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

[BC Break] trimHeaderValues in 1.6 Release #282

Closed
solverat opened this issue Jul 1, 2019 · 9 comments
Closed

[BC Break] trimHeaderValues in 1.6 Release #282

solverat opened this issue Jul 1, 2019 · 9 comments

Comments

@solverat
Copy link

@solverat solverat commented Jul 1, 2019

With the new release, all headers are getting asserted by its type. Having said that, this introduces a BC break. Symfony's Browserkit Client for example, passes the https value as a boolean type:

https://github.com/symfony/symfony/blob/38b9b951a42b908fd7e7e9651e41209bc7e620d1/src/Symfony/Component/BrowserKit/AbstractBrowser.php#L385

After updating guzzlehttp/psr7 to 1.6, this request will fail.

@Tobion
Copy link
Member

@Tobion Tobion commented Jul 1, 2019

Symfony BrowserKit does not use psr7. How does the boolean https trigger the error? Do you use ServerRequest::fromGlobals? Please provide a reproducer.

@svandervlugt
Copy link

@svandervlugt svandervlugt commented Jul 1, 2019

I think I hit the same problem.

In 1.6 new GuzzleHttp\Psr7\Response(200, ['Authorization' => null], '') throws an InvalidArgumentException while in 1.5.2 it generates the same as
new \GuzzleHttp\Psr7\Response(200, ['Authorization' => ''], '')

@sagikazarmark
Copy link
Member

@sagikazarmark sagikazarmark commented Jul 1, 2019

According to PSR-7 header values must be strings or arrays of strings. 1.6.0 enforces that. While it might be a break from a library point of view, this library is a PSR-7 implementation. Relying on undocumented features is never a good idea.

@Tobion IMHO we should add a warning to the release section saying that this release might break for users not following the PSR-7 specification.

@Tobion
Copy link
Member

@Tobion Tobion commented Jul 1, 2019

Fix in #283

@sagikazarmark
Copy link
Member

@sagikazarmark sagikazarmark commented Jul 1, 2019

IMHO that should only be the solution, if we add back the assertions to 2.x

@Tobion
Copy link
Member

@Tobion Tobion commented Jul 1, 2019

Yes that's a possibility. But let's not do it in 1.6 as it has only been released for 1 day and several people are affected. So more people will follow otherwise.

@GrahamCampbell
Copy link
Member

@GrahamCampbell GrahamCampbell commented Jul 1, 2019

I disagree it's a BC break. Because the param type was string[], the specification says the behaviour is undefined if you pass something else, and changing the behaviour of something outside of the specification shouldn't be regarded as a BC break.

That said, for reasons of practicality, it doesn't make sense to break the Symfony code when there's a simple way to get the old behaviour back. ;)

@sagikazarmark
Copy link
Member

@sagikazarmark sagikazarmark commented Jul 1, 2019

☝️

@sagikazarmark
Copy link
Member

@sagikazarmark sagikazarmark commented Jul 1, 2019

Fixed in #283

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants