New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ensure removed dimensions are not used even if they still exist #16934
Conversation
@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. |
@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 |
63a6383
to
5d51583
Compare
@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? |
c557e58
to
83c6951
Compare
@tsteur applied the feedback |
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