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

fix notice when generating documentation #18597

Merged
merged 3 commits into from Jan 12, 2022

Conversation

mattmary
Copy link
Contributor

fix a notice when generating the documentation when matomo is not installed.

@mattmary mattmary added the Needs Review PRs that need a code review label Jan 10, 2022
sgiehl
sgiehl previously approved these changes Jan 11, 2022
@sgiehl sgiehl added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Jan 11, 2022
@sgiehl sgiehl added this to the 4.7.0 milestone Jan 11, 2022
@sgiehl sgiehl removed the Needs Review PRs that need a code review label Jan 11, 2022
@sgiehl
Copy link
Member

sgiehl commented Jan 11, 2022

Hm... this is weird. Logically the change makes fully sense. But it causes a lot tests to fail. Will try to investigate the reason for this.

@sgiehl
Copy link
Member

sgiehl commented Jan 11, 2022

I guess the reason is this part:

if (!isset($this->pluginHooks[$pluginName])) {
$plugin = $manager->getLoadedPlugin($pluginName);
$this->pluginHooks[$pluginName] = $plugin->registerEvents();
}

Before the method was returning null, causing the events to be reloaded, once an event was posted. So guess in the tests the plugin is loaded too early and not loaded again, causing the events not to be registered again.

I guess we need to move the isInstalled check into each event method and return the event list in any case. Will try changing that.

@sgiehl
Copy link
Member

sgiehl commented Jan 11, 2022

Thinking more about that, I guess we could actually simply remove the isInstalled check. Plugin hooks should only be loaded for activated plugins nevertheless, and I don't think a plugin can be active, but uninstalled. The check has been added in a very early state of the CustomDimensions plugin (See matomo-org/plugin-CustomDimensions@ddcf327), so it might actually be not needed anymore.

@sgiehl sgiehl dismissed their stale review January 11, 2022 10:33

needs update

@sgiehl sgiehl added the Needs Review PRs that need a code review label Jan 11, 2022
@sgiehl sgiehl requested a review from tsteur January 11, 2022 12:35
@tsteur
Copy link
Member

tsteur commented Jan 11, 2022

I had a look for a whiel and I reckon this should be now fine to merge since we would install this plugin along with Matomo core. Versus before I think there could have been an issue that the log DB schema changes take some time (minutes, hours,days) to execute meaning it would have been in a state possibly where it's trying to activate and use the plugin but it's not finished installing yet. Then some of the events could have caused issues. This should no longer be an issue though since now the installation is always quick.

LGTM 👍

@sgiehl sgiehl merged commit 7681fc0 into 4.x-dev Jan 12, 2022
@sgiehl sgiehl deleted the fix_notice_developer_generation branch January 12, 2022 09:48
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 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