Make sure required plugins are loaded in correct order #11678
Conversation
I noticed a bug when a plugin requires another plugin. If eg a plugin named `Acc` requires a plugin named `Cli`, plugin `Acc` will be not activated because `Acc` will be always loaded before `Cli`. See https://github.com/piwik/piwik/blob/3.0.4-b2/core/Application/Kernel/PluginList.php#L113-L114 we always sort custom plugins alphabetically. However, it should respect a required plugin and make sure to load a required plugin first. In this case `Cli` before `Acc`. In #11676 I implemented similar solution but is slower I would say. This solution should be quite a bit faster when having a few plugins installed. @sgiehl @mattab maybe you guys have a better idea on how to fix it? There is still an issue that `plugins/$pluginName/config/config.php` in each plugin is loaded alphabetically. It is to be discussed if a `config.php` of a required plugin should have less of a priority so that a plugin that requires another plugin, can overwrite a DI setting of the required plugin. Meaning ideally, the config for `Cli` would be loaded first, then `Acc` because `Acc` requires `Cli`. This way `Acc` could overwrite any DI setting from `Cli`. Currently, `config.php` files are loaded as they are defined in `config.ini.php` which is usually alphabetically because of https://github.com/piwik/piwik/blob/3.0.4-b2/core/Plugin/Manager.php#L209-L212 (if config file is writable and not changed manually).
tsteur
added
Bug
|
|
||
| $requirements = $newPlugin->getMissingDependencies(); | ||
|
|
||
| if (!empty($requirements)) { |
There was a problem hiding this comment.
This is triggered when a required plugin's version could not be found. This should be similar to the statement below if ($newPlugin->hasMissingDependencies()) {. Without testing, it looks like this block would result in posting events to required plugins only in the case where the required plugin had some incompatibility (ie. different version).
-> does this PR result in posting events to required plugins all the time (including when the plugin requiring other plugins have all dependencies met)?
There was a problem hiding this comment.
Yes, that should still work the same. The logic should be the same, we only load another plugin first under circumstances. I just also debugged and events are postponed to all plugins and if there is a missing dependency it will postpone events to a dependency before the plugin that requires this dependency. Which is good as then a plugin that requires a different plugin could overwrite an action etc.
👍 it seems it would make sense to load config.php in the correct order. as you say this PR is faster than the other one, it would be desired to use this one. from looking at the code only I wasn't sure yet if this is fully working as expected, see my comment. if it is working, feel free to merge 👍 |
|
@tsteur feel free to merge this if/when you're done |
|
FYI: Created an issue for config problem: #11681 |
I noticed a bug when a plugin requires another plugin.
If eg a plugin named
Accrequires a plugin namedCli, pluginAccwill be not activated becauseAccwill be always loaded beforeCli. See https://github.com/piwik/piwik/blob/3.0.4-b2/core/Application/Kernel/PluginList.php#L113-L114 we always sort custom plugins alphabetically. However, it should respect a required plugin and make sure to load a required plugin first. In this caseClibeforeAcc.In #11676 I implemented similar solution but is slower I would say. This solution should be quite a bit faster when having a few plugins installed. Also this plugin should make sure to load plugins alphabetically if no plugin requires anything which means the tests won't randomly fail.
@sgiehl @mattab maybe you guys have a better idea on how to fix it?
There is still an issue that
plugins/$pluginName/config/config.phpin each plugin is loaded alphabetically. It is to be discussed if aconfig.phpof a required plugin should have less of a priority so that a plugin that requires another plugin, can overwrite a DI setting of the required plugin. Meaning ideally, the config forCliwould be loaded first, thenAccbecauseAccrequiresCli. This wayAcccould overwrite any DI setting fromCli. Currently,config.phpfiles are loaded as they are defined inconfig.ini.phpwhich is usually alphabetically because of https://github.com/piwik/piwik/blob/3.0.4-b2/core/Plugin/Manager.php#L209-L212 (if config file is writable and not changed manually).