Fix broken HttpMethodsClient with PSR RequestFactory

php-http / client-common

Common HTTP Client implementations and tools for HTTPlug

http://httplug.io

MIT License

1.01k stars 53 forks source link

Fix broken HttpMethodsClient with PSR RequestFactory #202

Closed GrahamCampbell closed 4 years ago

GrahamCampbell commented 4 years ago

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	yes
Related tickets	fixes #201
License	MIT

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.

GrahamCampbell commented 4 years ago

Good call about the empty string. I've allowed that case through without setting the body. :)

GrahamCampbell commented 4 years ago

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.

dbu commented 4 years ago

thanks!

dbu commented 4 years ago

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.

GrahamCampbell commented 4 years ago

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: