Last modified: 2014-08-20 19:28:22 UTC
Bug #44113 claims to address this very statement which now reads if ( $this->proxy || !$this->noProxy ) { return; To break this out: if ( # if $this->proxy # the proxy is already set || # or !$this->noProxy # noProxy is not set ) { return; Instead, it should read "If the proxy is already set or noProxy is set": if ( $this->proxy || $this->noProxy ) { return;
Change 141366 had a related patch set uploaded by devunt: Use correct condition for MWHttpRequest::proxySetup() function https://gerrit.wikimedia.org/r/141366
Change 141374 had a related patch set uploaded by MarkAHershberger: Use correct condition for MWHttpRequest::proxySetup() function https://gerrit.wikimedia.org/r/141374
Change 141375 had a related patch set uploaded by MarkAHershberger: Use correct condition for MWHttpRequest::proxySetup() function https://gerrit.wikimedia.org/r/141375
Change 141376 had a related patch set uploaded by MarkAHershberger: Use correct condition for MWHttpRequest::proxySetup() function https://gerrit.wikimedia.org/r/141376
I think this change affects the logic in a non-intended way. The two conditions produce different results in case a proxy is set and proxy is disabled. In the original logic, the statement results in "false", and the following code is executed. In this proposed change, the proxy is unset, even if it was set before. The change you are suggesting here leads to the proxy remaining set, if it was set before, even if we have the no proxy flag. As Aaron Schulz and T.Gries pointed out in bug 44113, the former behaviour is inteded to bypass default settings on local urls.
The code that this repairs simply doesn't work in my testing. Granted, I may not be testing correctly, but I'd like to be shown that. Set up a MW server that can only connect to the outside world via a proxy. Now, set $wgHTTPProxy so the MW can use the proxy. Call Http::get( "http://www.google.com" ) The call will fail because noProxy is false by default (meaning, use the proxy!) and so the condition if ( $this->proxy || !$this->noProxy ) { will be true and setupProxy() will return without setting $this->proxy to $wgHttpProxy.
Change 141376 abandoned by Legoktm: Use correct condition for MWHttpRequest::proxySetup() function Reason: Not merged into master yet https://gerrit.wikimedia.org/r/141376
Change 141375 abandoned by Legoktm: Use correct condition for MWHttpRequest::proxySetup() function Reason: Not merged into master yet https://gerrit.wikimedia.org/r/141375
Change 141374 abandoned by Legoktm: Use correct condition for MWHttpRequest::proxySetup() function Reason: Not merged into master yet https://gerrit.wikimedia.org/r/141374
Clearing backport flag since nothing has been merged into master yet, and the patch has a -2 on it.
