@bx80 @michalkleiner Feel free to already have a look at this one and give feedback if you have any ideas how to change or improve that. Most stuff should already be finished. But I need to have another look through the plugins to check if any might use some vendor lib that isn't proxied yet.

That certainly is a big change, will look through it.

I already have two questions:

• Did you use any tooling to do the change across the code base such as Rector or any other tokenizer/replacer? Just so we could document the process to be replicable and also if someone wanted to do the same, to be able to compare and confirm all the usages are replaced.
• If we're trying to prevent plugins from using dependencies that might sometimes be in a lower version than what we need due to composer resolution (or other reasons), how is this avoiding the issue if the classes (mostly) proxy the calls to the original dependencies anyway?

If we're trying to prevent plugins from using dependencies that might sometimes be in a lower version than what we need due to composer resolution (or other reasons), how is this avoiding the issue if the classes (mostly) proxy the calls to the original dependencies anyway?

Matomo plugins will need to work with the versions of dependencies used by Matomo core. When Matomo is being used in a WordPress plugin, where other WordPress plugins may have loaded different version of the dependencies, then Matomo core can use dependency prefixing to avoid conflicts.

Some Wordpress plugin -> php-di/php-di  v2.0.0
Some Matomo plugin    -> matomo core:piwik/di -> prefixed-for-matomo/php-di/php-di v6.0.0

This change isn't adding the prefixing, but is a first step to make sure that all Matomo plugins are using a vendor proxy class in core to access dependencies. This is the breaking change as Matomo plugins will need to be updated and so needs to be part of a major release, the actual prefixing change can be done later after the major release. Prefixing could also just be done for Matomo for WordPress and not for normal installs.

(that's my understanding anyway! 🙂 )

The proxy classes should prevent that plugins need to use any class (or namespaced method) from a dependency directly. The requirement is more to be able to prefix the vendor libs if needed, without breaking plugins, than fully hiding our dependencies.
Fully proxying the dependencies might actually be better, so the plugin would not even be able to access anything, but that would be a lot more effort I guess and a bit out of scope for now.

And I didn't use any tool for the changes. Some of the stuff can be easily replaced with regexes, but some stuff needs to be looked at manually I guess.

The proxy classes should prevent that plugins need to use any class (or namespaced method) from a dependency directly.

Yes, though ultimately that dependency still need to be installed by Composer and the same 'version resolution'-related issue could occur if I understood the problem right.

Would we need to add any Composer dependencies to core while moving them from the plugins? Or it is basically limited to dependencies that we already have/know about? This might surface once the core/maintained plugins are updated to use the proxy classes.

And I didn't use any tool for the changes. Some of the stuff can be easily replaced with regexes, but some stuff needs to be looked at manually I guess.

All good, I might try a quick Rector run to see what it produces, just to gauge the effort and compare the outcomes. That shouldn't hold this PR though.

@michalkleiner Matomo itself uses a fixed set a composer dependencies. We have even committed the composer.lock to ensure it always installs the same version.
Therefor we are updating to every new version of a vendor lib "manually" and don't need to handle any other version of a vendor library.

Plugins are still allowed to have their own dependencies. But from Matomo 5 on we will recommend to use php-scoper to prefix all vendors of a plugin to ensure they can't conflict with another wordpress plugin or even with core if we decide to add new vendor libs.

Thanks for explaining and having patience with me, @sgiehl!

@michalkleiner @bx80 The changes in the PR should now be complete. It would be good if one or even both of you would do an intense review of this PR. As it contains a lot (breaking) changes it's important to check that everything still works. Especially some commands could be broken now. We unfortunately don't have many tests for our commands, so we might need to test some of them manually to prove they still work or maybe add some tests if important commands don't have any.

If you both think the changes are fine we can merge this PR so we can finish the required adjustments in the plugins.
If you're unsure maybe @tsteur could also have a rough final look...

Note: As some of the plugins are included as submodules in core, the PRs for those plugins need to be merged first, so we can update the submodule references here. Otherwise it can happen that we end up with invalid references as the submodules in this PR target certain branches that might get deleted after their PRs are merged.
The plugins team might possibly be able to help you with reviewing the submodule plugins. Tests will likely fail there, as they run against 5.x-dev, but they also run in core tests. So as long as core passes, there shouldn't be an issue in the submodule plugins tests and we can rerun them there once this PR was merged.

I also didn't have enough time to check if any plugin might use another core dependency. But we can add additional proxy classes later if that might be required.

In the end a quick summary of the todos:

• Review this PR and apply required changes if needed
• Review the PRs for submodule plugins and merge them
• Merge the PRs in submodule plugins
• Update the submodule plugins in this PR to latest 5.x-dev
• If tests are passing, merge this PR
• Notify plugins team to finish the pending PRs

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers