@mattmary opened this Pull Request on January 10th 2022 Contributor

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

@sgiehl commented on January 11th 2022 Member

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 commented on January 11th 2022 Member

I guess the reason is this part:

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 commented on January 11th 2022 Member

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 https://github.com/matomo-org/plugin-CustomDimensions/commit/ddcf3278db875cab1eabcc07c91f14141e69d3e6), so it might actually be not needed anymore.

@tsteur commented on January 11th 2022 Member

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.


This Pull Request was closed on January 12th 2022
Powered by GitHub Issue Mirror