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

CorePluginsAdmin.setSystemSettings always saves the plugin settings for all plugins #12826

Closed
tsteur opened this issue May 6, 2018 · 1 comment · Fixed by #12827
Closed
Assignees
Labels
Bug For errors / faults / flaws / inconsistencies etc. c: Performance For when we could improve the performance / speed of Matomo. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself.

Comments

@tsteur
Copy link
Member

tsteur commented May 6, 2018

Instead, it should only save the plugins for the plugin(s) that is/are being updated. Through the UI, always only one plugin is being updated. Through the API in theory multiple plugins can be updated.

The payload may look like this:

image

So we should only update the plugin settings for plugins that are in the payload. Ideally, we would check if any value was changed for a plugin but this may be quite hard. Instead, it should be fine though to check the plugin names that are updated and then only save those.

This is to prevent possible data loss, not doing unneeded actions that may happen in a "save" method such as here https://github.com/matomo-org/plugin-QueuedTracking/blob/master/SystemSettings.php#L413-L429 and in other plugins.

It is not 100% bug, but classifying it as a bug as it could cause problems.

Same problem should exist for the method setUserSettings

@tsteur tsteur added Bug For errors / faults / flaws / inconsistencies etc. c: Performance For when we could improve the performance / speed of Matomo. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. labels May 6, 2018
@tsteur tsteur self-assigned this May 6, 2018
@mattab
Copy link
Member

mattab commented May 7, 2018

Good find 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. c: Performance For when we could improve the performance / speed of Matomo. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants