Fix broken HttpMethodsClient with PSR RequestFactory #202
Conversation
src/HttpMethodsClient.php
Outdated
| { | ||
| if (!$requestFactory instanceof RequestFactory && !$requestFactory instanceof RequestFactoryInterface) { | ||
| throw new \TypeError( | ||
| sprintf('%s::__construct(): Argument #2 ($requestFactory) must be of type %s|%s, %s given', self::class, RequestFactory::class, RequestFactoryInterface::class, get_debug_type($requestFactory)) | ||
| ); | ||
| } | ||
|
|
||
| if (!$requestFactory instanceof RequestFactory && null === $streamFactory) { | ||
| @trigger_error(sprintf('Passing a %s without a %s to %s::__construct() is deprecated as of version 2.3 and will be disallowed in version 3.0. A stream factory is required to create a request with a string body.', RequestFactoryInterface::class, StreamFactoryInterface::class, self::class)); |
There was a problem hiding this comment.
i assume that in next major, we might want to only support the psr factory, to move forward? then we could simply make the stream factory a required argument... (no need to change anything here, just a thought ;-) )
There was a problem hiding this comment.
I think so too. I am thinking once the old interfaces are all marked as deprecated in the future (https://github.com/php-http/message-factory/blob/master/src/ResponseFactory.php doesn't have a deprecation annotation yet), we can come back here to this constructor and also add a futher deprecation warning for if a psr request factory is not passed. Then yeh, in the next major version make all prams typed with psr interfaces, and all required.
| $headers, | ||
| $body | ||
| )); | ||
| if (!is_string($uri) && !$uri instanceof UriInterface) { |
There was a problem hiding this comment.
Note that this is not a BC break, even if the caller didn't have strict typing and expected stuff to be cast to a string at runtime, because this file itself has strict typing and was previously forwarding the parameter as is, so the result would still be a type error later, but just a more obscure error message. Including an early guard here will help callers correct their error, and also checking the uri and body are the correct types here mean our logic later on within the private createRequest function is a little simpler than it would have otherwise been.
|
Good call about the empty string. I've allowed that case through without setting the body. :) |
|
After this is merged, don't tag a release yet. I am going to add phpstan to the CI in a separate PR, and see if I can detect any further bugs. phpstan would have prevented this bug, for example. |
|
thanks! |
|
phpstan is a good idea for sure. i don't know if it would have spotted this particular issue, because the factory can have one or the other type, and the call was correct for one type. |
|
It would have warned us that we could possibly be passing too many parameters, if we had the analysis level turned up high enough (actually, level 2 or higher). It'd have insisted we did an instanceof check:
|
This PR was merged into the 3.0.x-dev branch. Discussion ---------- ~Depends on php-http/client-common#202 EDIT: merged and released. Commits ------- d934db0 Fix the HTTP methods client
The PSR request factory doesn't know anything about headers or body, and these parameters are silently thrown away!
In order to avoid these, we must set them on the request afterwards. Note that by doing this we have revealed a hidden dependency on a stream factory. I have allowed for this in BC-preserving manner.