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

Deactivating a plugin: clear the caches before unloading the plugin #8016

Merged
merged 2 commits into from Jun 5, 2015

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented May 28, 2015

Fixes #8007

The plugin instance is used for clearing the caches, so it needs to be unloaded after the caches are cleared. I'm opening for review because I am not certain what I'm doing isn't affecting other parts :/ (and maybe there's a better solution)

For the record it started to show the warning after a20e71c fixed a bug (unloading plugins wasn't really unloading before) which made this bug more visible (so it may have been partially broken for some time before).

The plugin is used for clearing the caches, so it needs to be unloaded after.
@mnapoli mnapoli added Bug For errors / faults / flaws / inconsistencies etc. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels May 28, 2015
@mnapoli mnapoli added this to the 2.14.0 milestone May 28, 2015
@diosmosis
Copy link
Member

Can you trigger the bug in a test?

@diosmosis
Copy link
Member

I think these two things would help in checking that there are no side effects:

  1. Check whether Manager::removePluginFromConfig(), Manager::unloadPluginFromMemory(), Manager::executePluginDeactivate() inadvertently accesses one of the caches that are cleared. If so, maybe the cache will be recreated before the plugin is removed from the Plugin\Manager list?
  2. Access the caches in a plugin's Plugin::deactivate() and make sure the recreated cache doesn't cause problems.

This is just from looking at the existing code, though. I haven't worked w/ this code in a while, so I don't know of any specific issues.

@tsteur
Copy link
Member

tsteur commented Jun 1, 2015

LGTM. Not merging directly since there were some other comments

@mnapoli
Copy link
Contributor Author

mnapoli commented Jun 5, 2015

I've added a test covering that and had a look at the code, couldn't find usage of the cache.

mnapoli added a commit that referenced this pull request Jun 5, 2015
Deactivating a plugin: clear the caches before unloading the plugin
@mnapoli mnapoli merged commit 447ff12 into master Jun 5, 2015
@mnapoli mnapoli deleted the fix/deactivating-plugins branch June 5, 2015 15:27
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. Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants