r111588 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r111587‎ | r111588 | r111589 >
Date:22:40, 15 February 2012
Author:wikinaut
Status:ok (Comments)
Tags:
Comment:
important fix inside HttpFunctions.php. I noticed that when you call HttpRequest::factory with noProxy=true for example inside an intranet, this is ignored in (Curl)HttpRequest because the proxy url is not cleared. This commit makes sure at lowest level - namel in the public function proxySetup() - that a optional(true) value of noProxy is observed and that it (now) also clears the proxy url. (I have been told that I am allowed to commit this patch. Please carefully review, as usual).
Modified paths:
  • /trunk/phase3/includes/HttpFunctions.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/HttpFunctions.php
——@@ -20,8 +20,9 @@
2121 * - timeout Timeout length in seconds
2222 * - postData An array of key-value pairs or a url-encoded form data
2323 * - proxy The proxy to use.
24 - * Will use $wgHTTPProxy (if set) otherwise.
25 - * - noProxy Override $wgHTTPProxy (if set) and don't use any proxy at all.
 24+ * Otherwise it will use $wgHTTPProxy (if set)
 25+ * Otherwise it will use the environment variable "http_proxy" (if set)
 26+ * - noProxy Don't use any proxy at all. Takes precedence over proxy value(s).
2627 * - sslVerifyHost (curl only) Verify hostname against certificate
2728 * - sslVerifyCert (curl only) Verify SSL certificate
2829 * - caInfo (curl only) Provide CA information
——@@ -286,11 +287,11 @@
287288 public function proxySetup() {
288289 global $wgHTTPProxy;
289290
290 - if ( $this->proxy ) {
 291+ if ( $this->proxy && !$this->noProxy ) {
291292 return;
292293 }
293294
294 - if ( Http::isLocalURL( $this->url ) ) {
 295+ if ( Http::isLocalURL( $this->url ) || $this->noProxy ) {
295296 $this->proxy = '';
296297 } elseif ( $wgHTTPProxy ) {
297298 $this->proxy = $wgHTTPProxy ;
——@@ -401,9 +402,11 @@
402403 $this->setReferer( wfExpandUrl( $wgTitle->getFullURL(), PROTO_CURRENT ) );
403404 }
404405
405 - if ( !$this->noProxy ) {
406 - $this->proxySetup();
407 - }
 406+ /**
 407+ * Set up the proxy, but
 408+ * clear the proxy when $noProxy is set (takes precedence)
 409+ */
 410+ $this->proxySetup();
408411
409412 if ( !$this->callback ) {
410413 $this->setCallback( array( $this, 'read' ) );

Follow-up revisions

RevisionCommit summaryAuthorDate
r112663r111588: refactored proxy/noProxy code slightly for simplicityaaron00:36, 29 February 2012

Comments

#Comment by Wikinaut (talk | contribs)   22:51, 15 February 2012

(background: I found this problem when debugging Extension:RSS for fetching feeds from external servers using a proxy and from internal domains w/o proxy. E:RSS uses HttpRequest classes . It was however _not_ possible to fetch feeds from internal domains - where no proxy is allowed - even when setting noProxy = true as it was documented. Debugging then inside HttpRequst revealed, that the proxy url came through, but the noProyx switch value was lost - before curl was executed. Curl tried to fetch an internal url with proyx - which failed.)

#Comment by Wikinaut (talk | contribs)   22:52, 15 February 2012

In my opinion, the patch is justified; however, I do not know if it has unintended and unwanted side effects, I could not find any.

#Comment by Aaron Schulz (talk | contribs)   20:55, 17 February 2012 
+ if ( $this->proxy && !$this->noProxy ) {

Shouldn't this be an OR statement?

This code goes about this awkwardly. Instead of having to remember to check noProxy all the time, just nuke the proxy var in the constructor and where wgHTTPProxy is checked.

#Comment by Wikinaut (talk | contribs)   00:18, 18 February 2012

"+ if ( $this->proxy && !$this->noProxy ) Shouldn't this be an OR statement? "

No.

This was one of the core changes, as it makes sure that the processing _continues_ here in the case that $this->noProxy is true. Without that patch, processing stopped and a proxy value was not cleared.

In other words, if noProxy is set true, the function must not return but continue in order to clear any proxy value.

(I use this fix in our enterprise wiki since many days, and everything works very well - fetching feeds from the intranet without proxy and from the extranet with proxy.)

#Comment by Wikinaut (talk | contribs)   00:19, 18 February 2012

This part must be reached if noProxy is true:


if ( Http::isLocalURL( $this->url ) || $this->noProxy ) { $this->proxy = ;

#Comment by Wikinaut (talk | contribs)   00:20, 18 February 2012

[CORR] This part must be reached if noProxy is true: 

if ( Http::isLocalURL( $this->url ) || $this->noProxy ) {
   $this->proxy = ;
}
#Comment by Wikinaut (talk | contribs)   00:21, 18 February 2012

reset to NEW

#Comment by Aaron Schulz (talk | contribs)   00:27, 18 February 2012

The code is still awkward in constantly checking noProxy. That looks like an antipattern. That was the reason I tagged it.

#Comment by Wikinaut (talk | contribs)   00:32, 18 February 2012

Well, honestly, I could not find another method. Please, if you can correct it, this would be fine.

Status & tagging log