Closed bezumkin closed 4 months ago
Why this package has bootstrap.php that overrides Guzzle and PSR classes on every core initialization?
It doesn't override them - it forces them to be loaded into memory. The reason that is still useful in MODX3 is because it typically forces them all to come from the same place (i.e. core/vendor) and not another location that may also have those same files (like an extra that includes it as a dependency) that is initialised later on.
Not only is it not needed in MODX 3, but it can also cause a conflict with other dependencies installed with core composer.json.
This package's autoloader is not required when it detects the classes are already loaded (ie in MODX3). See the bootstrap.php that ends early if the classes are already there.
If you post the full stack trace of your conflict error, I'm pretty sure you'll find a path somewhere in there that is not guzzle7 and not core...
If an extra requires MODX3+, it can indeed just use the core provided services as long as it doesn't require guzzle through some dependency. Lots of SDK-type packages sneakily introduce it and this does still help with conflicts with the "preloading".
@Mark-H
It doesn't override them... See the bootstrap.php that ends early if the classes are already there.
Sorry, but it doesn't work as you say, according to XDebug:
When you load your own composer autoloader after MODX core autoloader - it replace classes. This is the only explanation why I see the fatal error with code from your package dependency, but not from modx.
As you can see, there is 2 Psr\Http\Message\UriInterface
in the system, first is from MODX core without return type, and another is from Guzzle7 package with return type. And because the second one replaces the first - I see the fatal error.
Guzzle7 package should not be installed in MODX 3, because it is just not needed if we already have Guzzle as core dependency.
You don't need to load anything manually, you don't need to be sure that something is already in memory - that is why Composer was invented for. If I remove this dependency from UpgradeMODX, it continue to just work as it should, no errors.
Lots of SDK-type packages sneakily introduce it and this does still help with conflicts with the "preloading".
Right now I see the very weird situation with all this bootstrap.php
files of extras that brings their own small copies of vendor
directory with nobody knows versions of packages.
And MODX loads all of them on every core init. This is madness!
That xdebug screenshot suggests that the class_exist() checks are failing? I.e. it can't find the classes on your installation. $skip defaults to true, and is only set to false (which will load the guzzle7 autoloader) if the classes can't be found.
You don't need to load anything manually, you don't need to be sure that something is already in memory - that is why Composer was invented for. Right now I see the very weird situation with all this bootstrap.php files of extras that brings their own small copies of vendor directory with nobody knows versions of packages. And MODX loads all of them on every core init. This is madness!
Agreed, it's a mess.
This package offers a pragmatic fix by 1) providing a single set of the very common guzzle dependency that extra developers can incorporate for 2.x (with compatibility for 3.x too, but agreed, on 3.x it is largely unnecessary) and 2) pre-loading classes which helps avoid conflicts with extras that don't use this package but ship their own guzzle version.
The problem 2) fixes is when package 1 (or core) provides guzzle 7 but package 2 has guzzle 6, and suddenly the autoloading in PHP gets all confused mixing classes from the different versions. It would cause errors like the one you see. Pre-loading these classes earlier on avoids that because, hopefully, that is loading the class before the next autoloader is initialised.
The question is why on your installation, the bootstrap isn't detecting the guzzle classes are already there.
@Mark-H Yep. it doesn't see the \GuzzleHttp\ClientInterface
, because it should be checked with interface_exists()
Here is how it is now:
https://github.com/modmore/guzzle7/assets/1257284/ee33661d-32dd-4367-9e45-f441cec8c3f5
And here is how it could be fixed:
https://github.com/modmore/guzzle7/assets/1257284/a3c7e94b-2a0e-49a3-a91d-c267db3c0deb
But I strongly against installing this in MODX 3.
Thanks for figuring that out, fixed in v7.8.1. Should now behave as intended.
While not required for extras that only support 3.0, it still provides the benefit of loading those core-provided classes into memory which helps avoid conflicts when there is another extra that does ship with the guzzle classes and uses guzzle version detection to determine what type of wrapper to use. That's something that I've seen done a lot in the world of SDKs and the pre-loading through the class/interface checks helps those determine the right version shipped by core or this extra.
I still strongly recommend this for extras supporting both 2.x and 3.x.
@Mark-H Thank you for the new version with fix!
The only way to avoid dependencies conflict in MODX 3 is install all extras with Composer. All other ways are just different workarounds.
Too bad there's no such thing that supports users on MODX 2 and 3 yet. ;)
Very curious to check out what you worked on recently, just been too busy putting out fires. Would've loved to catch your presentation about it in Belgrade. If there's any English info on it yet, I'd love to hear and I'm sure others in the community would too.
@Mark-H Here is my Belgrade presentation
And today I started my career as a video blogger - you can try to watch this video with some AI translator or autogenerated subtitles.
MODX 3 already has
guzzlehttp/guzzle
in dependencies since the beginning of using Composer.Why this package has bootstrap.php that overrides Guzzle and PSR classes on every core initialization?
Not only is it not needed in MODX 3, but it can also cause a conflict with other dependencies installed with core
composer.json
.We found out this situation when tested mmx/forms on site with installed UpgradeMODX that requires Guzzle7.
PR to fix this requirement in UpgradeMODX is already made.