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

Make sure required plugins are loaded in correct order #11676

Closed
wants to merge 2 commits into from

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented May 8, 2017

I noticed a bug when requiring a 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 the diff changes, so far 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.

While this PR should work, it is maybe a bit slow to load the plugin.json of each custom plugin each time for each request additionally (we load it again later in Metadata loader where it is cached). In tracker we would under circumstances not need to load it at all.

@sgiehl @mattab maybe you guys have a better idea on how to fix it? AFAIK the DI container and environment is not set up at this time and caching this information is likely not really possible. Also we can often not write it directly into plugin.json as we may not have file permissions etc.

@tsteur tsteur added the Bug For errors / faults / flaws / inconsistencies etc. label May 8, 2017
@tsteur tsteur added this to the 3.0.4 milestone May 8, 2017
@tsteur
Copy link
Member Author

tsteur commented May 9, 2017

I'm thinking we could maybe enrich the logic here instead: https://github.com/piwik/piwik/blob/3.0.4-b2/core/Plugin/Manager.php#L849-L850

When there is a missing dependency, we could check if we are supposed to load the required dependency and if so, try to load that dependency first and then check for missing dependencies again. Any thoughts?

tsteur added a commit that referenced this pull request May 9, 2017
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 tsteur added the Needs Review PRs that need a code review label May 9, 2017
@tsteur
Copy link
Member Author

tsteur commented May 9, 2017

Issued alternative in #11678

@tsteur
Copy link
Member Author

tsteur commented May 9, 2017

Closing this one in favour of #11678

@tsteur tsteur closed this May 9, 2017
@tsteur tsteur deleted the requiredplugins branch May 9, 2017 20:56
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant