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

[7.x] Fix Http::withCookies() #31745

Closed
wants to merge 5 commits into from
Closed

[7.x] Fix Http::withCookies() #31745

wants to merge 5 commits into from

Conversation

SjorsO
Copy link
Contributor

@SjorsO SjorsO commented Mar 4, 2020

Currently, Http::withCookies() has an array typehint. If you give this method an array, it will throw this error: cookies must be an instance of GuzzleHttp\Cookie\CookieJarInterface

For reference, Zttp's withCookies method doesn't have a typehint.

Fixes #31750

@SjorsO
Copy link
Contributor Author

SjorsO commented Mar 4, 2020

The CI fails because --prefer-lowest installs Guzzle 6.3.0, which contains a bug that was fixed in 6.3.1: guzzle/guzzle#1999

@driesvints
Copy link
Member

@SjorsO it only fails with this PR so we'd have to update the minimum version in this pr.

@SjorsO
Copy link
Contributor Author

SjorsO commented Mar 4, 2020

@SjorsO it only fails with this PR so we'd have to update the minimum version in this pr.

I'm not very familiar with version constraints, would changing "guzzlehttp/guzzle": "^6.3|^7.0", to "guzzlehttp/guzzle": "^6.3.1|^7.0", fix it?

@GrahamCampbell
Copy link
Member

I'm not very familiar with version constraints, would changing "guzzlehttp/guzzle": "^6.3|^7.0", to "guzzlehttp/guzzle": "^6.3.1|^7.0", fix it?

Yes.

@GrahamCampbell
Copy link
Member

GrahamCampbell commented Mar 4, 2020

Please update the suggest blocks too (in all composer.json files).

@SjorsO
Copy link
Contributor Author

SjorsO commented Mar 4, 2020

Please update the suggest blocks too (in all composer.json files).

I've updated the suggest block for the HTTP client.

I haven't updated the Console and Mail suggest blocks, because technically they would work with ^6.3.0. Should I update them to ^6.3.1 too?

@GrahamCampbell
Copy link
Member

Please update all the suggest blocks, so the framework suggests the same version, across all the components.

@SjorsO
Copy link
Contributor Author

SjorsO commented Mar 4, 2020

Please update all the suggest blocks, so the framework suggests the same version, across all the components.

I've bumped all the ones I could find. Thanks for your help!

Copy link
Member

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now. 👍

@taylorotwell
Copy link
Member

Uh, I don't want people to have to use a "CookieJar" - I just want to be able to pass an array.

@SjorsO
Copy link
Contributor Author

SjorsO commented Mar 5, 2020

@taylorotwell If you give this method an array Guzzle will throw an exception. Which means that with the current type-hint, this method is unusable.

ZTTP uses this method to re-use cookies between requests. $response->cookies() returns a CookieJar.

@gocanto
Copy link
Contributor

gocanto commented Mar 5, 2020

@SjorsO you might want to accept the array as an argument and build the jar within the given method. :)

ref: https://github.com/guzzle/guzzle/blob/master/src/Cookie/CookieJar.php#L25

@SjorsO
Copy link
Contributor Author

SjorsO commented Mar 5, 2020

@SjorsO you might want to accept the array as an argument and build the jar within the given method. :)

ref: https://github.com/guzzle/guzzle/blob/master/src/Cookie/CookieJar.php#L25

I thought about using CookieJar::fromArray($array, $domain), but you don't have the $domain when this method is called, so it gets kind of tricky.

@driesvints
Copy link
Member

Taylor pushed a new commit for this: 36d783c

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

Successfully merging this pull request may close these issues.

Problem with HTTP facade and withCookies() function
5 participants