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

Prefix four dependencies during composer install/update (symfony/console, twig, monolog & php-di) #19712

Closed
wants to merge 144 commits into from

Conversation

diosmosis
Copy link
Member

@diosmosis diosmosis commented Sep 7, 2022

Fixes #16905
Refs #8568

In environments when Matomo runs alongside other PHP software, such as in Matomo for WordPress, hard to debug issues caused by conflicting dependencies can arise. Given there is no way to know exactly which other versions of dependencies might
end up being used in a user's setup, dealing with them on a version-by-version basis is undesirable (if at all possible).

This PR provides the ability to prefix dependencies in core and in plugins as part of the composer workflow, making sure the
dependencies we use do not conflict with different versions in other software.

twig & monolog are currently configured to be prefixed in this PR. An example of prefixing composer dependencies in a plugin can be found here: matomo-org/plugin-GoogleAnalyticsImporter#279

Adding support for dependency prefixing

Prefixing is accomplished via a new composer:prefix-dependency command. It is added as a post install and post update script in composer.json files, eg:

    "scripts": {
        "post-install-cmd": [
            "@php ./console composer:prefix-dependency"
        ],
        "post-update-cmd": [
            "@php ./console composer:prefix-dependency"
        ]
    }

or

    "scripts": {
        "post-install-cmd": [
            "@php ../../console composer:prefix-dependency --plugin=MyPlugin"
        ],
        "post-update-cmd": [
            "@php ../../console composer:prefix-dependency --plugin=MyPlugin"
        ]
    }

For plugins, the dependencies that you want prefixed are listed in the plugin.json file, in the prefixedDependencies property. For core, the list is hardcoded in the Prefixer class.

Prefixing a dependency

Once the command is added to composer.json, you can add dependencies to prefixedDependencies. However, this alone may not be enough. If the dependency you want to prefix dynamically references classes and functions, php-scoper, the tool used to prefix the namespaces, may not be able to detect and change those uses. In that case, you need to manually replace code via a patcher that you define in scoper.inc.php. An example of this is the extra replaces done for twig in the scoper.inc.php file for Matomo core, and https://github.com/matomo-org/plugin-GoogleAnalyticsImporter/blob/a7f6b48bec018338bb815bdf9bfa2e17223aa5f7/scoper.inc.php for google's compiled PHP protobuf code.

How prefixing is accomplished

Prefixing is accomplished using the php-scoper project. php-scoper processes a folder, adding a prefix to all namespace/use
statements, and anything else it recognizes. It works pretty well, but doesn't replace more dynamic usages. Those can be
replaced manually, however, by adding code to a scoper.inc.php file (as mentioned above).

The entire process, encapsulated by the composer:prefix-dependency command and the Piwik\Dependency\Prefixer class is as follows:

  • if php-scoper does not exist locally and a custom path was not given, the php-scoper phar is downloaded from a hardcoded link
  • determine what dependencies should be scoped. for core, it's hardcoded in a constant. for plugins, it's in the prefixedDependencies property in the plugin's plugin.json file.
  • child dependencies are recursively gathered. child dependencies are prefixed as well to avoid dependency conflicts with them in addition to the root dependency.
    • at this point we also read the composer.json file of every dependency to prefix and gather the namespaces of those dependencies (via the psr-4 autoload configuration).
  • if prefixing for a plugin and the plugin does not have a scoper.inc.php file, an empty one is generated
  • php-scoper is run to prefix the main namespaces of the dependency. The prefix is either Matomo\Dependencies for core or Matomo\Dependencies\{PluginName} for a plugin.
    • the dependencies to prefix are made available to the scoper.inc.php file via the MATOMO_DEPENDENCIES_TO_PREFIX environment variable. scoper.inc.php uses a Finder to limit the processed files to those dependencies' directories.
    • the namespaces to prefix are made available to the scoper.inc.php file via the MATOMO_NAMESPACES_TO_PREFIX environment variable. scoper.inc.php specifies those namespaces in a include_namespaces configuration for php-scoper (NOTE: this option is actually added via a fork on my personal github, more info below.) This lets us limit prefixing to just the dependencies we want to prefix.
    • the prefixed files are output to a vendor/prefixed directory
  • if prefixing for a plugin, a second php-scoper pass is done, this time with Matomo\Dependencies as the prefix and the namespace just for prefixed core dependencies. So in core, since we prefix monolog, this part of the process will replace uses of Monolog in plugin dependencies with Matomo\Dependencies\Monolog, for instance.
    • the output here goes to a prefixed2 directory, which is then swapped w/ prefixed so the result of the second pass is in the prefixed/ folder.
  • the prefix-dependency command now uses composer to dump an autoload file for the prefixed/ directory, if it exists.
  • the dependencies that were prefixed are now removed from the vendor/ directory.
  • then we run the dump-autoload command again for the root vendor/ directory, so it no longer references dependencies that are now in prefixed/.
  • finally, a new autoload.php file is generated which loads the vendor/ autoloader, and, if it exists, the vendor/prefixed/ autoloader.

The process is slightly different if prefixing for a plugin that has an empty prefixedDependencies property. In this case, since we don't need to prefix anything, but still need to replace references to prefixed core dependencies, only the second php-scoper pass is done.

include_namespaces

Currently php-scoper only has exclude_*** configuration options. This is pretty inconvenient when we want to prefix X and exclude everything else, so I added an include_namespaces feature in https://github.com/diosmosis/php-scoper/tree/included-namespaces. An official fork in the matomo-org organization will need to be made eventually. No other changes to php-scoper are needed, and we probably won't need to ever update it. I'll try to get the changes accepted upstream after this is reviewed.

Review

@diosmosis
Copy link
Member Author

FYI @tsteur. Not sure if there's anyone else I should ping, apologies if I missed someone.

@tsteur
Copy link
Member

tsteur commented Sep 8, 2022

Nice, I just checked out the branch and ran the command and it renamed things nicely.

Ideally, when running composer in the future then it would indeed need to run php-scoper directly as part of the composer command.

It also allows using no prefixed dependencies, so Matomo can be distributed normally and any side effects of prefixing
can be limited to Matomo for WordPress, where the benefits of prefixing outweigh any potential issues.

Not fully understanding what is meant by this one @diosmosis ?

Would that mean that if someone uses Matomo for WordPress, and they install another Matomo plugin from the Marketplace in their WordPress, and this marketplace doesn't use the prefixed version, then it may cause issues that it could use a wrong vendor version from a completely different WordPress plugin?

I guess if we were to only replace Twig and Monolog this could be fine potentially as it's maybe usually not expected that these libraries would be used too often directly.

I guess from my perspective I just want to keep On-Premise and Matomo for WordPress as similar as possible to not have any unnoticed possible regressions in Matomo for WordPress because they run different code. Looking at the PR I think that doesn't seem to be the case though if I understand it right and would only apply to plugins.

@diosmosis
Copy link
Member Author

@tsteur

Would that mean that if someone uses Matomo for WordPress, and they install another Matomo plugin from the Marketplace in their WordPress, and this marketplace doesn't use the prefixed version, then it may cause issues that it could use a wrong vendor version from a completely different WordPress plugin?

Yes, that could be an issue. If scoping in the composer workflow, then I guess the plugin developer would be forced to use the prefixed namespace otherwise it wouldn't work for a normal Matomo, so this wouldn't happen.

I guess from my perspective I just want to keep On-Premise and Matomo for WordPress as similar as possible to not have any unnoticed possible regressions in Matomo for WordPress because they run different code. Looking at the PR I think that doesn't seem to be the case though if I understand it right and would only apply to plugins.

I understand. For a proof of concept I wanted to provide a solution the was as minimal as possible and show that if it turns out to be more complicated to prefix a dependency correctly and predictably, there was an easy way to go back. I also wanted to show its possible to externalize the mechanism and not have it in core if desired. All of this mostly because there was a lot of manual replacing required for twig, though it's probably an exception given it defines so many functions and generates php code that uses them.

Regardless, I can make it part of the composer workflow if desired, should I move forward with that? (I don't have a big opinion here, I think it's workable either way.)

@diosmosis
Copy link
Member Author

@tsteur I'm going to assume adding to the compost workflow is required. Can you tell me what release this is intended to be a part of or point me to someone who can?

@sgiehl
Copy link
Member

sgiehl commented Sep 8, 2022

We shouldn't do that before Matomo 5. As plugin developers should adjust their code, that imho shouldn't be done in a minor release. Also using the scoped classes in plugins would make those incompatible with older Matomo releases...

@diosmosis
Copy link
Member Author

Clarified on slack this should be targeting 5.x-dev.

@diosmosis diosmosis added this to the 5.0.0 milestone Sep 12, 2022
@tsteur
Copy link
Member

tsteur commented Sep 13, 2022

Sorry about the late reply

Regardless, I can make it part of the composer workflow if desired, should I move forward with that? (I don't have a big opinion here, I think it's workable either way.)

Yes, that be great

@diosmosis diosmosis changed the title Proof of Concept: allow scoping composer dependencies Prefix two dependencies during composer install/update (twig & monolog) Sep 13, 2022
diosmosis and others added 6 commits April 4, 2023 17:42
…dencies from autoload files, since dependencies of prefixed dependencies may not be included
…for prefixed dependencies so unprefixed dependencies of prefixed dependencies will have psr classmaps generated correctly
@diosmosis
Copy link
Member Author

@sgiehl OneClickUpdate tests are passing. The other failures look unrelated, let me know if otherwise.

@diosmosis diosmosis changed the title Prefix three dependencies during composer install/update (twig, monolog & php-di) Prefix four dependencies during composer install/update (symfony/console, twig, monolog & php-di) Apr 6, 2023
@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Apr 13, 2023
@sgiehl sgiehl removed this from the 5.0.0 milestone May 5, 2023
@sgiehl sgiehl removed Needs Review PRs that need a code review 5.0.0 labels May 5, 2023
@sgiehl sgiehl marked this pull request as draft May 12, 2023 12:35
@github-actions
Copy link
Contributor

This PR was last updated more than one month ago, maybe it's time to close it. Please check if there is anything we still can do or close this PR. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale for long The label used by the Close Stale Issues action label Jun 24, 2023
@michalkleiner michalkleiner removed Stale The label used by the Close Stale Issues action Stale for long The label used by the Close Stale Issues action labels Jun 27, 2023
@github-actions
Copy link
Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Jul 12, 2023
@michalkleiner michalkleiner removed the Stale The label used by the Close Stale Issues action label Jul 12, 2023
@github-actions
Copy link
Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Jul 27, 2023
@michalkleiner michalkleiner removed the Stale The label used by the Close Stale Issues action label Jul 27, 2023
@github-actions
Copy link
Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Aug 11, 2023
@michalkleiner michalkleiner added Do not close PRs with this label won't be marked as stale by the Close Stale Issues action and removed Stale The label used by the Close Stale Issues action labels Aug 15, 2023
@diosmosis diosmosis closed this Sep 10, 2023
@sgiehl sgiehl deleted the 16905-php-scoper branch January 9, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Do not close PRs with this label won't be marked as stale by the Close Stale Issues action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prefix all our vendor libs
6 participants