r91642 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r91641‎ | r91642 | r91643 >
Date:14:56, 7 July 2011
Author:mah
Status:reverted (Comments)
Tags:
Comment:
Bug #29755: Apply patch from Vitaliy Filippov so that MW's HTTP client
respects no_proxy env setting
Modified paths:
  • /trunk/phase3/includes/HttpFunctions.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/HttpFunctions.php
@@ -273,12 +273,61 @@
274274 $this->proxy = 'http://localhost:80/';
275275 } elseif ( $wgHTTPProxy ) {
276276 $this->proxy = $wgHTTPProxy ;
277 - } elseif ( getenv( "http_proxy" ) ) {
278 - $this->proxy = getenv( "http_proxy" );
 277+ } elseif ( $this->useProxy( $this->url ) ) {
 278+ $this->proxy = $this->useProxy( $this->url );
279279 }
280280 }
281281
282282 /**
 283+ * Determine HTTP proxy from environment settings respecting
 284+ * 'http_proxy' and 'no_proxy' environment variables
 285+ */
 286+ public static function useProxy( $url ) {
 287+ if ( $proxy = getenv( "http_proxy" ) ) {
 288+ $useproxy = true;
 289+ if ( $url && ( $noproxy = preg_split( "#\s*,\s*#is", getenv( "no_proxy" ) ) ) ) {
 290+ foreach ( $noproxy as $n ) {
 291+ if ( preg_match('#(\d+)\.(\d+)\.(\d+)\.(\d+)/(\d+)#s', $n, $m) &&
 292+ preg_match('#^[a-z0-9_]+://(?:[^/]*:[^/]*@)?([^/@]+)(?:/|$|\?)#is', $url, $ip) ) {
 293+ $mask = array(
 294+ max( 0x100 - ( 1 << max( 8-$m[5], 0 ) ), 0 ),
 295+ max( 0x100 - ( 1 << max( 16-$m[5], 0 ) ), 0 ),
 296+ max( 0x100 - ( 1 << max( 24-$m[5], 0 ) ), 0 ),
 297+ max( 0x100 - ( 1 << max( 32-$m[5], 0 ) ), 0 ),
 298+ );
 299+ $ip = @gethostbyname( $ip[1] );
 300+ if ( preg_match( '#(\d+)\.(\d+)\.(\d+)\.(\d+)#s', $ip, $ipm ) &&
 301+ ( intval( $ipm[1] ) & $mask[0] ) == intval( $m[1] ) &&
 302+ ( intval( $ipm[2] ) & $mask[1] ) == intval( $m[2] ) &&
 303+ ( intval( $ipm[3] ) & $mask[2] ) == intval( $m[3] ) &&
 304+ ( intval( $ipm[4] ) & $mask[3] ) == intval( $m[4] ) ) {
 305+ $useproxy = false;
 306+ break;
 307+ }
 308+ } else {
 309+ $n = preg_replace( '/#.*$/is', '', $n );
 310+ $n = preg_quote( $n );
 311+ $n = str_replace( '\\*', '.*', $n );
 312+ if ( preg_match( '#'.$n.'#is', $url ) ) {
 313+ $useproxy = false;
 314+ break;
 315+ }
 316+ }
 317+ }
 318+ }
 319+ if ( $useproxy ) {
 320+ $proxy = preg_replace( '#^http://#is', '', $proxy );
 321+ $proxy = preg_replace( '#/*$#is', '', $proxy );
 322+ }
 323+ else {
 324+ $proxy = null;
 325+ }
 326+ return $proxy;
 327+ }
 328+ return null;
 329+ }
 330+
 331+ /**
283332 * Set the refererer header
284333 */
285334 public function setReferer( $url ) {

Follow-up revisions

RevisionCommit summaryAuthorDate
r91738Reverting r91642 per CR and pending review need-patch/patch in bug 29755krinkle17:45, 8 July 2011

Comments

#Comment by 😂 (talk | contribs)   15:08, 7 July 2011

No tests? :(

#Comment by Bryan (talk | contribs)   15:19, 7 July 2011
  • Error suppression operator. Should never be used in new code.
  • Variable names do not match MediaWiki code conventions. Use $lowerCamelCase.
  • The absence of code comments, makes it almost impossible to review this. The regexes suggest that you are looking to IPv4 IP addresses. You should use the functions in the IP class for this, and also make this working with IPv6.
#Comment by MarkAHershberger (talk | contribs)   16:34, 7 July 2011

You should've seen the patch I didn't submit! Good points, though.

#Comment by VitaliyFilippov (talk | contribs)   20:15, 7 July 2011

Thanks for review! I didn't expect it in trunk that fast :)

What's the IP class?

#Comment by Bryan (talk | contribs)   20:16, 7 July 2011

The IP class in includes/IP.php defines helper functions, which should be mostly compatible with both IPv4 and IPv6.

#Comment by Hashar (talk | contribs)   20:23, 7 July 2011

You can look it up on http://svn.wikimedia.org/doc/ or use the link:

http://svn.wikimedia.org/doc/classIP.html

IP::isIPAddress() should do it.

#Comment by Hashar (talk | contribs)   16:31, 7 July 2011

I believe mark just applied the patch to get the bug closed. Lets just rewrite the above code :)

#Comment by 😂 (talk | contribs)   16:37, 7 July 2011

Well if we're just going to start blindly applying patches I've got 351 bugs in core we can go ahead and resolve ;-)

#Comment by Nikerabbit (talk | contribs)   16:38, 7 July 2011

Seems to get better review this way :o

#Comment by MarkAHershberger (talk | contribs)   18:17, 7 July 2011

I didn't look as thoroughly as I should have and missed a few basic things as Bryan pointed out. However, Nikerabbit is right: the code review process is missing from Bugzilla patches. This one looked especially good -- at least the functionality of it. And Code Review sprang into action soon after I made the commit.

#Comment by Krinkle (talk | contribs)   17:45, 8 July 2011

Reopening bug 29755, patch wasn't reviewed yet. (or rather, it wasn't a patch/diff at all).

I think it was a good experience to trigger code review by committing it (which may have been a coincidence), but I don't think it's something that should be a acceptable way of getting code review. It will only stress developers into maintaining trunk.

In a time where pre-commit review is being considered (perhaps even github-like forks and merges, there even are github projects of which the history solely consists of merges! that result from pull-requests that have been reviewed first), this doesn't look good.

It's probably easy to turn this into a patch and make it happy with the conventions but to keep the patch simple I've reverted it for now.

Status & tagging log