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

Conflict with a theme based on composer using a more recent Twig version #583

Closed
MikeNGarrett opened this issue Mar 25, 2020 · 17 comments
Closed
Labels

Comments

@MikeNGarrett
Copy link

We're using Timber, a theme that allows integration of Twig and other libraries which are installed via Composer. Installing PublishPress plugins causes a conflict with our theme's composer libraries. PublishPress uses an older version of Twig which is the primary evidence that there's an issue. PublishPress loads in composer libraries regardless of whether or not they already exist.

I understand that it would be helpful if WordPress had tools to manage this from a central composer.json or some other form of dependency management.

It would be helpful for compatibility's sake to load these libraries in a namespace for PublishPress to prevent conflicts.

Here's an example: https://jtreminio.com/blog/composer-namespaces-in-5-minutes/#bonus-round-custom-namespaces

@andergmartins
Copy link
Collaborator

andergmartins commented Mar 26, 2020

Hi @MikeNGarrett thanks for reporting the issue.

Thanks for the suggestion regarding the custom namespace. We use the custom namespace in the composer file, but that works only for our own libraries because PHP requires de namespace to be set on the top of every file. Unfortunately that would require to fork the Twig library and refactor the namespace on every file. But we are thinking about a solution for this.

@andergmartins andergmartins changed the title Namespaced composer dependencies Conflict with a theme based on composer using a more recent Twig version Mar 26, 2020
@andergmartins andergmartins self-assigned this Mar 26, 2020
@MikeNGarrett
Copy link
Author

Hi Ander,
Thank you for responding.

You're right that it would require refactoring the plugins. That's a big task. It wouldn't require forking twig. You can see the example I linked to for composer namespaces gives a great example for how to add namespaces in the autoloader without forking anything.

@andergmartins
Copy link
Collaborator

Hi Mike,

Sure thing, thank you for collaborating.

Yeah, thanks, I checked the post again, but I think there is a chance we are talking about different things.

My point is that if the conflict is related to different versions of the Twig library are being loaded at the same time, both will use the same namespace. If I got it right, that is the conflict you are reporting.

Since we load Twig from composer, it has its own composer.json file and its own namespace (please, see its reference here: /~https://github.com/twigphp/Twig/blob/1.x/composer.json#L38). That namespace is only used to make the composer's autoload acknowledge the existence of the namespace and map it to the twig's internal folder: src. Please, note that all the files will have the namespace specified in the top of the file, like here: /~https://github.com/twigphp/Twig/blob/1.x/src/Environment.php#L12. That is the part we can't change.

If I got your suggestion right, you are suggesting make our plugin load Twig using a different namespace, like: \PublishPress\Twig\. Yes, we could add a custom namespace to our own composer.json file mapping that namespace to the vendor/twig/twig/src folder, but that won't work as expected because the autoloader won't find that namespace on any file coming from twig. All the files have: namespace Twig; So any class we try to load won't be found by PHP. The files are there, but the class implementation wouldn't be compatible with the new namespace we are trying to specify.

That composer's feature is intended to use to specify existent namespaces or create new ons for classes you own (so you can make the namespace fit it on every file). Not for overriding namespaces already specified.

Even if we were able to do that, there is no way (as far as I know) to stop composer of loading the original namespace Twig is declaring. So the conflict still exists.

When we talk about the multiple instances of a library being loaded we are talking about a conflict detected by PHP's runtime. And the fact it that it doesn't care if you are using an autoloader (from composer or not) or if you are importing the classes manually using require, require_once, include etc. Once the file is loaded, the class will be identified by PHP based on the namespace declaration you have as the first statement on the beginning of each file.

You can find more information about namespaces here:
https://www.php.net/manual/en/language.namespaces.rationale.php
https://www.php.net/manual/en/language.namespaces.definition.php

This issue is something we do have on our plate, since we are starting to use composer more widely. We are going to look into it for sure. Feel free to let us know if you have any question.

@MikeNGarrett
Copy link
Author

Thank you for the in-depth explanation. I read through this potential solution too fast and didn't notice the important specification detail you pointed out: this is intended for libraries you have control over. I really appreciate the time you took to respond.

@andergmartins
Copy link
Collaborator

@MikeNGarrett sure no problem, you're more than welcome.

We do appreciate your collaboration.
Feel free to let us know if you have any question or suggestions.

@MikeNGarrett
Copy link
Author

Good morning!
We ran into this issue with another plugin on this project, so I started doing a bit of digging to see if I could find a solution.

There's a library called php-scoper that's suited for exactly this situation. A number of other plugins have followed this method recently.

Read more: matomo-org/matomo-for-wordpress#233 and mailpoet/mailpoet#1911 and google/site-kit-wp#612

It sounds like this is a very simple method for prefixing, but it still requires updating any references in your codebase to use the new prefix which is a big task.

Hope this helps!

@andergmartins
Copy link
Collaborator

Hi @MikeNGarrett, thanks for the suggestion. At a first glance that is the best option for now but we need to make some tests before use that.

Some days ago I was trying to solving a similar issue with some internal libraries we have and wrote a prototype for a smarter loader for WordPress plugin that only loads the most recent version of the library. But while navigating in the links you suggested I found out that Jetpack has a "similar" but more robust solution: /~https://github.com/Automattic/jetpack-autoloader.

Thanks for collaborating. We are going to start some testing on the next days.

@andergmartins
Copy link
Collaborator

andergmartins commented Apr 14, 2020

Tools or options for prefixing the namespaces of our dependencies:

Options for fixing/improving the issue in the autoloader

Things we need to think about:

@agapetry, please, we need to talk about this on the next days...

  1. Thinking about consistency, do we need to prefix this for every plugin we have or we should do that only once for all the PublishPress plugin ecosystem - maybe pushing them to specific repositories so we can reuse it internally?
    If 3 of our plugins are using Twig and considering they are all activated, do we really need to have 3 twig (or other common dependency) - plus any 3rd party theme/plugin using another version - libraries loaded? Today we have 6 plugins. So we could do it in a way we can reuse the "prefixed" libraries could be reused internally.
    If we do that, I suggest we use the Smart Loader, or try the Jetpack autoload plugin for making sure we always have the most recent version loaded.
    This way we end up with "PublishPress-Authors\Twig" + "PublishPress-Revisions\Twig" + etc. but only "PublishPress\Twig".

  2. If we do this for libraries that register WordPress hooks: actions or filters, do we have a problem calling an action that will be executed by each library, or a filter that will run multiple times for each library? Do we need to prefix the hooks as well? Maybe this is just an issue with our own internal libraries for now, but would be solved if we do the proposed on the item above (1.).

@agapetry
Copy link
Contributor

@andergmartins I'll work my way up to this after discussing a separate vendor library issue with you...

@andergmartins
Copy link
Collaborator

Something that just happened to my mind was the fact that some libraries will have other dependencies, and all that will be loaded multiple times (if those had the namespaces prefixed) - i know... it is probably obvious. Just something we need to check, if those libraries are prefixed by the tools we will be testing.

@agapetry
Copy link
Contributor

Here's an initial reply before I move on to a couple support troubleshooting issues. Guidelines I suggest:

  • We prefix our vendor library classes (and each of their dependencies) with a PublishPress_[pplibversion] prefix. So when running the latest release of all our plugins, only one copy of our vendor library is loaded.

  • Retain control of which version of third party vendor libraries (Twig, Pimple) will be used. So "our stable" is not necessarily "their stable". We have already had trouble with third party changes breaking our implementation in the past.

  • When an older version of Revisions is active alongside a newer version of Authors (for example), each will continue to use the vendor code that it was released with. If the vendor library changed, that means we have an extra copy of each being loaded. That performance hit can be resolved by simply upgrading to the latest release - much preferable to the risk of fatal error or unpredictable behavior due to an unforeseen vendor library change.

@agapetry
Copy link
Contributor

I should add that my concept is not that older versions of our plugins will always trigger a separate copy of the vendor library to be loaded. That would only be the case for plugin versions which were released with a different vendor library version.

@andergmartins
Copy link
Collaborator

@agapetry Thanks, those suggestions makes sense to me. I will think a little more about that.

@stevejburge
Copy link
Member

stevejburge commented Nov 19, 2020

I know others are following along, so I'll post @andergmartins thoughts:

We have 2 main issues related to Composer:

  1. Pro plugins (those using Free as Composer’s dependency) can’t be handled/installed throw Composer on sites that have the vendor folder not public. On that case, all the assets (css, img, js) that are used for the Free plugin won’t be accessible to the user, because they are inside the protected vendor folder.
  2. Conflict with different versions of 3rd party libraries like Twig. If another plugin is using a newer or older version of Twig, we can have some compatibility issues because a site can’t run 2 different versions of the same library. Using composer or not. The point is: 2 libraries in different versions. Only one will be loaded, and at least one of the plugins will have some compatibility issues because it was written for another version of the library.

Composer is a dependency manager. For 2, using Composer or not is not the problem, because if we loaded an older version of Twig directly, without using Composer, that conflict, or a similar one would still happen. Composer is actually designed to solve those cases of conflicts. But for having that advantage, it should be used to handle all the site, including all plugins. That is the link with the issue 1.

If 1 is fixed, 2 won’t happen on sites using Composer for handling all the site. They would see a warning in the terminal and wouldn’t be able to install the plugin (if the plugins are properly configured).

But since almost any site uses Composer, we can fix 2 implementing the solution we started discussing on the issue #583.

I started looking into that a few months ago, refactoring the namespaces of the libraries we use as dependency, but regarding the fact that there are some scripts available to make it more automatic, at a first glance, it requires to change some stuff by hand. At least, it required me to check everything by hand. Might take some time.

For now, I see a 2 step solution:

  1. Move from Composer to Git Submodules to handle the Free code inside the Pro. The Free plugin would be in another subfolder, out of the vendor folder, fixing the issue with the assets. Some paths, or assets urls should be refactored to match the new destination of the Free code. It will be a repository inside another repository. No changes in the Free code should be done inside the Pro repo.
  2. Refactoring the 3rd party libraries we use for having a custom namespace. This will make it more difficult to us for updating those libraries in future updates of our plugins, because the refactoring we previously did will need to be redone on the new code.

@andergmartins
Copy link
Collaborator

We received another report for a similar issue:

Hi, getting this error when I go to PublishPress > Roles. I also have the Capabilities plugin installed, if that is relevant.

pp-error-3ae2a11706204bf6eab29eb510bd177d

@stevejburge
Copy link
Member

@andergmartins Let's retire the Roles feature in PublishPress. Not needed

@andergmartins andergmartins removed their assignment Apr 15, 2022
@andergmartins
Copy link
Collaborator

We are removing Twig on #1125, please feel free to let us know if you still have this issue with any other library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants