-
Notifications
You must be signed in to change notification settings - Fork 20
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
Comments
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. |
Hi Ander, 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. |
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 If I got your suggestion right, you are suggesting make our plugin load Twig using a different namespace, like: 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 You can find more information about namespaces here: 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. |
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. |
@MikeNGarrett sure no problem, you're more than welcome. We do appreciate your collaboration. |
Good morning! 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! |
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. |
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...
|
@andergmartins I'll work my way up to this after discussing a separate vendor library issue with you... |
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. |
Here's an initial reply before I move on to a couple support troubleshooting issues. Guidelines I suggest:
|
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. |
@agapetry Thanks, those suggestions makes sense to me. I will think a little more about that. |
I know others are following along, so I'll post @andergmartins thoughts:
|
@andergmartins Let's retire the Roles feature in PublishPress. Not needed |
We are removing Twig on #1125, please feel free to let us know if you still have this issue with any other library. |
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
The text was updated successfully, but these errors were encountered: