php-http / message

HTTP Message related tools
http://php-http.org
MIT License
1.3k stars 42 forks source link

GuzzleStreamFactory not compatible with cache-plugin v2 #160

Closed ruudk closed 8 months ago

ruudk commented 8 months ago

When upgrading CachePlugin to v2, I noticed that the GuzzleStreamFactory fails:

TypeError: Http\Client\Common\Plugin\CachePlugin::__construct(): Argument #2 ($streamFactory) must be of type Psr\Http\Message\StreamFactoryInterface, Http\Message\StreamFactory\GuzzleStreamFactory given

https://github.com/php-http/message/blob/2311491d26df850f0da910ca2c3a03b236d12cd5/src/StreamFactory/GuzzleStreamFactory.php#L19

dbu commented 8 months ago

cache plugin 2 switched from our php-http interfaces to the PSR-17 interfaces.

when you upgrade to cache plugin 2, you should switch to the psr17 stream factory provided by guzzle itself, rather than use the deprecated stream factory provided in this repository. (see the note on the GuzzleStreamFactory: * @deprecated This will be removed in php-http/message2.0. Consider using the official Guzzle PSR-17 factory

ruudk commented 8 months ago

I didn't choose this, it's happening automatically because of this: https://github.com/php-http/discovery/blob/664ded67cc61abcaa5b77d27581777483d1c843d/src/Strategy/CommonClassesStrategy.php#L63

Not sure how to solve this.

ruudk commented 8 months ago

Manually setting this in my HTTPlug config seems to fix it:

'classes' => [
                'stream_factory' => HttpFactory::class,
            ],

I feel this should be default strategy?

dbu commented 8 months ago

the line you point to is the discovery for getting a Http\Message\StreamFactory, which is the old thing. it is supposed to return the old thing, and returning a psr factory would be incorrect. to get the psr version, use discovery for Psr\Http\Message\StreamFactoryInterface (as defined in the psr17 strategy)

is there something automagic that asks for the legacy stream factory?

ruudk commented 8 months ago

Well, the point is, I don't configure anything in the HTTPlug bundle. So there must be more users having this issue.

The CommonPsr17ClassesStrategy strategy is chosen after CommonClassesStrategy https://github.com/php-http/discovery/blob/664ded67cc61abcaa5b77d27581777483d1c843d/src/ClassDiscovery.php#L24-L30

That means that it always selects the deprecated one, instead of the PSR17 one.

dbu commented 8 months ago

ah, this is about the symfony bundle? i think the reason for what happens is https://github.com/php-http/HttplugBundle/blob/22dae44d4c47bf6ef73241a96816183428af7b9e/src/DependencyInjection/Configuration.php#L858 - that should look at the cache plugin version to determine what kind of factory it is looking for. the correct workaround for now would be to configure the stream_factory option in the httplug configuration of symfony.

i will try to have a look into the bundle default configuration soonish.

(btw, the strategies provide discovery for different things, so the order should not matter.)

dbu commented 8 months ago

ok, had some time to hopefully fix it in https://github.com/php-http/HttplugBundle/pull/448

can you please try with the latest dev of httplug bundle 1.x if thats fine? if it fixes the issue, i will tag a release.

ruudk commented 8 months ago

Awesome! Will test it out tomorrow and report back

ruudk commented 8 months ago

Just tested it and it works without issues. Let's tag it! :shipit:

dbu commented 8 months ago

thanks for testing it. released https://github.com/php-http/HttplugBundle/releases/tag/1.33.0