@flamisz opened this Pull Request on March 30th 2021 Contributor

Description:

fixes #17301

Review

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@flamisz commented on March 30th 2021 Contributor

Hi @tsteur and @diosmosis,

this is very WIP, but I'd like to know if I go in the right direction.

A couple of questions I have:

  • should I check if the hash column exists or the updater will solve it anyway (I mean it can not happen that the code updated but DB not?)
  • when we create the update file? whenever we need a migration? so this PR should contain a new update file?
  • how we fill the hash column for the existing segments in the db? does that go into the update file?
  • @tsteur mentioned in the issue, ideally we can remove the urlencodes from this PR: https://github.com/matomo-org/matomo/pull/17029/files. can we @diosmosis?

sorry I have too many questions, I just'd like to understand more and do properly. thanks

@diosmosis commented on March 30th 2021 Member

should I check if the hash column exists or the updater will solve it anyway (I mean it can not happen that the code updated but DB not?)

It shouldn't happen that the code is updated but DB is not updated. If it does happen, then it's because of a bug in the update process, or because of user error. So you shouldn't have to check if the added column exists. (There are exceptions in this because sometimes during the one click update, some new code will execute after it is written to the hard disk, while the old database hasn't been updated, but this shouldn't be one of those cases.)

when we create the update file? whenever we need a migration? so this PR should contain a new update file?

Yes :+1: and updates are tied to a version, so first you'd bump the version in Version.php (setting to a new beta or rc based on what's in there already).

how we fill the hash column for the existing segments in the db? does that go into the update file?

Yes :+1: it should be initialized to urlencode($segment['definition']) in the update file.

@tsteur mentioned in the issue, ideally we can remove the urlencodes from this PR: https://github.com/matomo-org/matomo/pull/17029/files. can we @diosmosis?

I think it depends on how this is implemented. To get rid of those urlencodes, in the Segment constructor you'd have to search through every stored segment in the db for a definition that matches, then if found, use the hash in the database.

sorry I have too many questions, I just'd like to understand more and do properly. thanks

from what matt & thomas said, questions are to be encouraged :+1:

@diosmosis commented on March 30th 2021 Member

I guess we don't have docs for writing updates/migrations, I'll put it on my list to try and write them (unless someone gets to them first).

@flamisz commented on March 31st 2021 Contributor

@tsteur and @diosmosis I'm wondering what else we need in this PR?
Can we get rid of those urlencode calls from the other PR? I checked that part of the code, and most of the time it gets the hash in the end of the process from the Segment, and now the getSegmentHash method does its best to find it (without anything, with urlencode and with urldecode as well).

@flamisz commented on April 11th 2021 Contributor

Hi @diosmosis, can you please review it again (hopefully last time) and we can merge it then?

@diosmosis commented on April 11th 2021 Member

When the tests pass, I'll take another look :+1:

@flamisz commented on April 14th 2021 Contributor

please note: this pr still contains the testdox="true" in the phunit.xml. Do we want to keep this? Sometimes it can be helpful to see the tests one by one, but we can always turn this on only when we need it.
@tsteur ?

@tsteur commented on April 14th 2021 Member

sounds very useful to me @flamisz 🚀 💯 I'd set this in a new PR though so the team can review it separately in case someone has a good reason for not having it

@tsteur commented on April 18th 2021 Member

@flamisz @diosmosis was wondering if there's much left in this issue to do? Asking as we'd like to have this issue included in the next release

@diosmosis commented on April 18th 2021 Member

@tsteur @flamisz merged it

This Pull Request was closed on April 18th 2021
Powered by GitHub Issue Mirror