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

Browserkit client https fix #32293

Closed
wants to merge 3 commits into from

Conversation

solverat
Copy link

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? (in progress)
Fixed tickets #32292
License MIT
Doc PR --

This PR fixes the wrong server header type HTTPS (currently boolean) which should be not available if not true or on if request is in https mode.

@solverat
Copy link
Author

I‘m still not sure about this. Maybe we should keep the boolean and also just pass „on“ to the request object if the HTTPS value ist „true“ (less changes). On the other hand: It’s a wrong value and we also pass other server values as they are - so the current pr is more consistent.

@@ -302,7 +302,9 @@ public function request($method, $uri, array $parameters = [], array $files = []
$server['HTTP_HOST'] = $this->extractHost($uri);
}

$server['HTTPS'] = 'https' == parse_url($uri, PHP_URL_SCHEME);
Copy link
Member

Choose a reason for hiding this comment

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

I think we should do the smallest possible change.
What about this?
$server['HTTPS'] = (int) ('https' == parse_url($uri, PHP_URL_SCHEME));

Copy link
Member

Choose a reason for hiding this comment

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

Isn't it a BC break?

Copy link
Author

Choose a reason for hiding this comment

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

@fabpot i just rechecked the whole thing. In fact, the "https" value always gets redefined in the client object, no matter which value $server is passing. So i think it's not a real bc break. You could pass any https value in $server => request(), it always checks against the url scheme via parse_url:

$server['HTTPS'] = 'https' == parse_url($uri, PHP_URL_SCHEME);

@solverat solverat closed this Jul 1, 2019
@solverat solverat force-pushed the browserkit_client_https_fix branch from fad6f34 to 081c601 Compare July 1, 2019 07:26
@solverat solverat reopened this Jul 1, 2019
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Jul 4, 2019
@nicolas-grekas
Copy link
Member

Thanks for reporting, now that this is fixed on Guzzle's side, no need to change anything here.

@gmponos
Copy link
Contributor

gmponos commented Jul 4, 2019

@nicolas-grekas you might want to reconsider since this might be the behavior of guzzle psr7 on version 2

@nicolas-grekas
Copy link
Member

But then the issue would lie in the code bridging this class (which doesn't know anything about Guzzle) and Guzzle.

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

Successfully merging this pull request may close these issues.

None yet

5 participants