@sgiehl opened this Pull Request on December 10th 2020 Member

Description:

In some cases it might happen that a dimension file that should have been deleted with an update fails to be removed.
In that case Matomo might fail in various places, as it might try to use a database column that does not exist any longer.

By maintaining a list of removed dimensions, we can ensure those dimensions won't be used anymore, even if the still exist.

TODO: Need to check if any other dimension was removed in 4.0 and how to document that clearly or maybe a some kind of test to check if something was removed but isn't in the list

refs #16927

Review

  • [ ] Functional review done
  • [ ] 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
@sgiehl commented on December 16th 2020 Member

@tsteur The PR as is would actually be good to merge and would possibly solve some update issues where one of the dimension file couldn't be removed.

But I'm currently thinking on how to best check for removed dimensions, to avoid such issues in the future if the list isn't updated. But actually I don't see a good way for doing that. We could check the git history for removed files and check if one of those files was a dimension (using it's own database column). But actually I'm not sure if it's worth the effort.

Also I wonder if we actually shouldn't remove the dimension version in option table with an update that removes a dimension.
For people where the updater didn't remove the file, that might actually cause a update adding the column again. So might be not that good, but at least would avoid such issues 🤔

@tsteur commented on December 16th 2020 Member

@sgiehl we wouldn't want to look through git history.

Could we add something like https://github.com/matomo-org/matomo/blob/4.0.5/plugins/Widgetize/tests/System/WidgetTest.php#L57-L75 where we have a list of all available dimensionIds. When one dimension is removed, we'd expect the test to fail and would mention something like "if the dimension was removed on purpose you now need to add it to the list of removed dimensions" or similar. Basically we'd maintain a hard coded list of dimensions we have in the test. So we notice when one is added or removed.

Haven't thought this through but maybe that would work.

Re removing options for these columns: It be a problem indeed if it was to try to add the columns again and again. Maybe instead the above test could help in the future as well to not add the same column name again? I wouldn't spend too much time on it but might be worth a check like if we had a list of getDeprecatedDimensions() and getDeprecatedColumns() then we could have a test that makes sure we don't add the same ones again in the future so the option could still exist.

@sgiehl commented on December 17th 2020 Member

@tsteur I've updated the PR. For now the test only checks the dimensions holding a column definition. Guess those dimensions might make the most trouble when removed. Or do you think we should maintain a list of all dimensions?

@sgiehl commented on December 21st 2020 Member

@tsteur applied the feedback

This Pull Request was closed on December 21st 2020
Powered by GitHub Issue Mirror