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

Namespace external package dependencies to avoid conflicts with other plugins/theme using the same package but different version. #3005

Closed
hellofromtonya opened this issue Aug 18, 2020 · 6 comments · Fixed by #3008
Labels
priority: 🔥critical The plugin or the website are unusable, it affects a significant number of customers
Milestone

Comments

@hellofromtonya
Copy link
Contributor

hellofromtonya commented Aug 18, 2020

As we experienced with the WooCommerce container conflict #3003 and here, when other plugins/themes use the same external composer dependencies but with a different version, the risk is high for a fatal error conflict. We can mitigate this risk by encapsulating each production-level dependency with a namespace.

There are multiple ways to accomplish this including:

  • Pulling dependencies into the Engine as we did with the Container
  • Using namespacing tools such as Daniel points out here

Let's be proactive to guard Rocket and our customers from this happening again.

@DanielRuf
Copy link

DanielRuf commented Aug 18, 2020

@DanielRuf
Copy link

Pulling dependencies into the Engine as we did with the Container

Probably more work to maintain and update in this case (just my opinion).

@iCaspar
Copy link
Contributor

iCaspar commented Aug 18, 2020

On a quick review of those tools mentioned/linked, I think our support of PHP 5.6 will rule out both php-scoper and imposter, as they both require at least 7.2. Mozart seems like it could work, as it requires 7.2 to run on composer, but can handle packages using < 7, BUT it is still an experimental package with it's own warning about "wearing a hard hat for use in production". Not good with a million sites relying on stability. :(

I think our best solution (for now) is likely to re-namespace our other dependencies into Engine, as we did for league/container. This requires some additional maintenance overhead, as we'd have to track the packages ourselves, but as we move into the new roadmap and get significant parts of the plugin out of the plugin :) it should be less cumbersome over time, until we can either drop 5.6 or Mozart stabilizes.

@hellofromtonya
Copy link
Contributor Author

@DanielRuf Your point is valid. It does add work for releases as we have to manually check and update each dependency. Definitely not optimal.

Mozart seems like it could work, as it requires 7.2 to run on composer, but can handle packages using < 7, BUT it is still an experimental package with it's own warning about "wearing a hard hat for use in production". Not good with a million sites relying on stability.

We're using Mozart on our standalone LazyLoad plugin. We have support tickets with a similar League Container fatal error. At the moment, we're not sure yet of the root cause.

However, @iCaspar your point is valid. "Experimental" is not stable or robust enough for our customers.

This is why we pulled the League Container into Rocket's Engine as it's a critical component to Rocket.

@DanielRuf
Copy link

DanielRuf commented Aug 18, 2020

Not sure but to me it looks like the dependencies are twice in there.

In the default vendor folder with the autoloader:
https://plugins.trac.wordpress.org/browser/rocket-lazy-load/trunk/vendor

In src/Dependencies:
https://plugins.trac.wordpress.org/browser/rocket-lazy-load/trunk/src/Dependencies

And I guess depending on the used loader it is a different path:

https://plugins.trac.wordpress.org/browser/rocket-lazy-load/trunk/vendor/composer/autoload_real.php

https://plugins.trac.wordpress.org/browser/rocket-lazy-load/trunk/vendor/composer/autoload_classmap.php

https://plugins.trac.wordpress.org/browser/rocket-lazy-load/trunk/vendor/composer/autoload_static.php

This is just an assumption as I've never used mozart and only used php-scoper in projects.

$vendorDir might be wrong in autoload_classmap.php. See line 26 of autoload_real.php.

@hellofromtonya
Copy link
Contributor Author

@DanielRuf Great catch. You pointed us in the right direction from which I created this issue. Thank you!

@hellofromtonya hellofromtonya added this to the 3.7 milestone Aug 18, 2020
@hellofromtonya hellofromtonya added the priority: 🔥critical The plugin or the website are unusable, it affects a significant number of customers label Aug 18, 2020
@remyperona remyperona mentioned this issue Aug 26, 2020
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: 🔥critical The plugin or the website are unusable, it affects a significant number of customers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants