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

[8.0] Final for no proxy settings for all php versions #3164

Open
wants to merge 2 commits into
base: 8.0
Choose a base branch
from

Conversation

gergokee
Copy link
Contributor

@gergokee gergokee commented Aug 18, 2023

Fix for issue mentioned in #3163

@gergokee gergokee changed the title Fie Fix for https://github.com/guzzle/guzzle/issues/3163 Aug 18, 2023
@gergokee gergokee changed the title Fix for https://github.com/guzzle/guzzle/issues/3163 Final for for no proxy settings for all php versions Aug 18, 2023
@GrahamCampbell
Copy link
Member

Is this an issue with curl or PHP? I am reluctant to revert this and re-break the issue this was fixing. Do you know the exact last version of libcurl that has this issue, if this a PHP issue, the exact last version of PHP to have this issue?


I'll leave this PR on hold till we have that information.

@oostapov
Copy link

Is this an issue with curl or PHP? I am reluctant to revert this and re-break the issue this was fixing. Do you know the exact last version of libcurl that has this issue, if this a PHP issue, the exact last version of PHP to have this issue?

I'll leave this PR on hold till we have that information.

It is a curl issue, not php.
cURL version: 8.2.1 (hint: php -i | grep cURL) but it worked the same wrong on earlier 8.1 too

@GrahamCampbell
Copy link
Member

GrahamCampbell commented Aug 27, 2023

Ok, so what is the latest version of curl where this is broken, so we can decide if this is actually something we want to support? (not what is the latest version of curl)

@oostapov
Copy link

As I said it is broken on the 8.2.1 which is currently latest stable. It has been broken and worked this way for years. 7.29 is the oldest I can test with and since that version it has never worked correctly.

@oostapov
Copy link

I believe it has nothing to do with cURL as the cURL behaves quite predictable. It is an improper handling of the proxy settings by the guzzle. If we want to not use proxy for some domain we should force the curl having it in NO_PROXY setting instead of leaving it unset which leads to falling back to the ENV settings

@gergokee
Copy link
Contributor Author

gergokee commented Aug 28, 2023

@GrahamCampbell

In short: my old fix was not a real fix. :/

Explanation:

The reason is that first i added $conf[\CURLOPT_PROXY] = ''; . This should have been okay. But then tests failed, so i accidentally added this: unset($conf[\CURLOPT_PROXY]); . Which is not a real fix, since we wanted to specifically set CURLOPT_PROXY to be empty, unsetting it does nothing :/

The reason now i want both CURLOPT_PROXY ='' and CURLOPT_NOPROXY set to the added values cause in older php versions CURLOPT_NOPROXY is not working. So setting both is the ultimate solution.

@gergokee gergokee changed the title Final for for no proxy settings for all php versions Final for no proxy settings for all php versions Feb 28, 2024
@GrahamCampbell
Copy link
Member

I'm wondering if Guzzle 8.0 is the best place to fix this. I'm very conscious of breaking things for people.

@GrahamCampbell GrahamCampbell changed the title Final for no proxy settings for all php versions [8.0] Final for no proxy settings for all php versions Mar 31, 2024
@GrahamCampbell GrahamCampbell changed the base branch from 7.7 to 8.0 March 31, 2024 19:44
Comment on lines 452 to 454
} else {
$conf[\CURLOPT_PROXY] = $options['proxy'][$scheme];
}

Choose a reason for hiding this comment

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

Although this covers the case where our request matches the ['proxy']['no'] configuration (and we want it not to use the proxy), it misses an inverse case. :)

Image that in the environment variables there's NO_PROXY="foo.bar" / no_proxy="foo.bar", and we want to configure guzzle to override/ignore this (to proxy everything, or at least proxy requests to foo.bar).

If we set:

'proxy' => [
    'http' => 'http://example:8080',
    'https' => 'http://example:8080',
    # Send everything through the proxy
    'no' => []
]

Then the environment's behavior will still get used and requests to foo.bar will (unexpectedly) not go through the proxy.

Perhaps something like this in the else branch?

Suggested change
} else {
$conf[\CURLOPT_PROXY] = $options['proxy'][$scheme];
}
} else {
$conf[\CURLOPT_PROXY] = $options['proxy'][$scheme];
$conf[\CURLOPT_NOPROXY] = "";
}

(Related comment on the issue: #3163 (comment))

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.

None yet

4 participants