Skip to content
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

Merged
merged 6 commits into from Dec 21, 2020

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Dec 10, 2020

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 sgiehl added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Dec 10, 2020
@sgiehl sgiehl added this to the 4.0.x milestone Dec 10, 2020
@sgiehl
Copy link
Member Author

sgiehl commented Dec 16, 2020

@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
Copy link
Member

tsteur commented Dec 16, 2020

@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 sgiehl marked this pull request as ready for review December 17, 2020 09:31
@sgiehl
Copy link
Member Author

sgiehl commented Dec 17, 2020

@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 sgiehl added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Dec 17, 2020
core/Columns/Dimension.php Outdated Show resolved Hide resolved
core/Columns/Dimension.php Outdated Show resolved Hide resolved
@sgiehl
Copy link
Member Author

sgiehl commented Dec 21, 2020

@tsteur applied the feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants