r61655 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r61654‎ | r61655 | r61656 >
Date:07:25, 29 January 2010
Author:mah
Status:resolved (Comments)
Tags:
Comment:
follow up r61357
* fix up fopen warnings — I was bedazzled by phpUnit's
set_error_handler that turned warnings into exceptions and Tim set
me right.
* Use Tim's read loop
* No need for private $fh member
* Fix some minor problems with PHP versions (no "timeout" param in
pre-5.2.1, POST issue in Hardy's PHP version where timeout is null.)
* Fix how tests are run on old versions of phpUnit.
Modified paths:
  • /trunk/phase3/includes/HttpFunctions.php (modified) (history)
  • /trunk/phase3/languages/messages/MessagesEn.php (modified) (history)
  • /trunk/phase3/tests/HttpTest.php (modified) (history)
  • /trunk/phase3/tests/bootstrap.php (modified) (history)

Diff [purge]

Index: trunk/phase3/tests/bootstrap.php
——@@ -13,12 +13,26 @@
1414
1515 define( 'MEDIAWIKI', true );
1616 define( 'MW_PHPUNIT_TEST', true );
17 -ini_set( 'include_path', "$IP:" .ini_get( 'include_path' ) );
1817
19 -require "$IP/includes/Defines.php";
20 -require "$IP/includes/AutoLoader.php";
21 -require "$IP/LocalSettings.php";
22 -
 18+require_once "$IP/includes/Defines.php";
 19+require_once "$IP/includes/AutoLoader.php";
 20+require_once "$IP/LocalSettings.php";
2321 require_once "$IP/includes/ProfilerStub.php";
2422 require_once "$IP/includes/GlobalFunctions.php";
2523 require_once "$IP/includes/Hooks.php";
 24+
 25+// for php versions before 5.2.1
 26+if ( !function_exists('sys_get_temp_dir')) {
 27+ function sys_get_temp_dir() {
 28+ if( $temp=getenv('TMP') ) return $temp;
 29+ if( $temp=getenv('TEMP') ) return $temp;
 30+ if( $temp=getenv('TMPDIR') ) return $temp;
 31+ $temp=tempnam(__FILE__,'');
 32+ if (file_exists($temp)) {
 33+ unlink($temp);
 34+ return dirname($temp);
 35+ }
 36+ return null;
 37+ }
 38+ }
 39+
Index: trunk/phase3/tests/HttpTest.php
——@@ -1,5 +1,9 @@
22 <?php
33
 4+if ( !defined( 'MW_PHPUNIT_TEST' ) ) {
 5+ require_once( 'bootstrap.php' );
 6+}
 7+
48 class HttpTest extends PhpUnit_Framework_TestCase {
59 static $content;
610 static $headers;
——@@ -25,7 +29,7 @@
2630 self::$has_curl = function_exists( 'curl_init' );
2731
2832 if ( !file_exists("/usr/bin/curl") ) {
29 - $this->markTestIncomplete("This test requires the curl binary at /usr/bin/curl. If you have curl, please file a bug on this test, or, better yet, provide a patch.");
 33+ $this->markTestIncomplete("This test requires the curl binary at /usr/bin/curl. If you have curl, please file a bug on this test, or, better yet, provide a patch.");
3034 }
3135
3236 $content = tempnam( sys_get_temp_dir(), "" );
——@@ -65,7 +69,7 @@
6670 function testInstantiation() {
6771 Http::$httpEngine = false;
6872
69 - $r = HttpRequest::factory("http://www.example.com/");
 73+ $r = HttpRequest::factory("http://www.example.com/");
7074 if ( self::$has_curl ) {
7175 $this->assertThat($r, $this->isInstanceOf( 'CurlHttpRequest' ));
7276 } else {
——@@ -120,7 +124,7 @@
121125 }
122126
123127 /* ./phase3/includes/Import.php:1108: $data = Http::request( $method, $url ); */
124 - /* ./includes/Import.php:1124: $link = Title::newFromText( "$interwiki:Special:Export/$page" ); */
 128+ /* ./includes/Import.php:1124: $link = Title::newFromText( "$interwiki:Special:Export/$page" ); */
125129 /* ./includes/Import.php:1134: return ImportStreamSource::newFromURL( $url, "POST" ); */
126130 function runHTTPRequests($proxy=null) {
127131 $opt = array();
——@@ -160,8 +164,8 @@
161165 /* ./extensions/BookInformation/drivers/IsbnDb.php:24: if( ( $xml = Http::get( $uri ) ) !== false ) { */
162166 /* ./extensions/BookInformation/drivers/Amazon.php:23: if( ( $xml = Http::get( $uri ) ) !== false ) { */
163167 /* ./extensions/TitleBlacklist/TitleBlacklist.list.php:217: $result = Http::get( $url ); */
164 - /* ./extensions/TSPoll/TSPoll.php:68: $get_server = Http::get( 'http://toolserver.org/~jan/poll/dev/main.php?page=wiki_output&id='.$id ); */
165 - /* ./extensions/TSPoll/TSPoll.php:70: $get_server = Http::get( 'http://toolserver.org/~jan/poll/main.php?page=wiki_output&id='.$id ); */
 168+ /* ./extensions/TSPoll/TSPoll.php:68: $get_server = Http::get( 'http://toolserver.org/~jan/poll/dev/main.php?page=wiki_output&id='.$id ); */
 169+ /* ./extensions/TSPoll/TSPoll.php:70: $get_server = Http::get( 'http://toolserver.org/~jan/poll/main.php?page=wiki_output&id='.$id ); */
166170 /* ./extensions/DoubleWiki/DoubleWiki.php:56: $translation = Http::get( $url.$sep.'action=render' ); */
167171 /* ./extensions/ExternalPages/ExternalPages_body.php:177: $serializedText = Http::get( $this->mPageURL ); */
168172 /* ./extensions/Translate/utils/TranslationHelpers.php:143: $suggestions = Http::get( $url, $timeout ); */
——@@ -198,7 +202,7 @@
199203 /* ./extensions/LocalisationUpdate/LocalisationUpdate.class.php:172: $basefilecontents = Http::get( $basefile ); */
200204 /* ./extensions/APC/SpecialAPC.php:245: $rss = Http::get( 'http://pecl.php.net/feeds/pkg_apc.rss' ); */
201205 /* ./extensions/Interlanguage/Interlanguage.php:56: $a = Http::get( $url ); */
202 - /* ./extensions/MWSearch/MWSearch_body.php:492: $data = Http::get( $searchUrl, $wgLuceneSearchTimeout, $httpOpts); */
 206+ /* ./extensions/MWSearch/MWSearch_body.php:492: $data = Http::get( $searchUrl, $wgLuceneSearchTimeout, $httpOpts); */
203207 function runHTTPGets($proxy=null) {
204208 $opt = array();
205209
Index: trunk/phase3/includes/HttpFunctions.php
——@@ -184,7 +184,7 @@
185185 case 'php':
186186 if ( !wfIniGetBool( 'allow_url_fopen' ) ) {
187187 throw new MWException( __METHOD__.': allow_url_fopen needs to be enabled for pure PHP http requests to work. '.
188 - 'If possible, curl should be used instead. See http://php.net/curl.' );
 188+ 'If possible, curl should be used instead. See http://php.net/curl.' );
189189 }
190190 return new PhpHttpRequest( $url, $options );
191191 default:
——@@ -365,8 +365,6 @@
366366 }
367367
368368 class PhpHttpRequest extends HttpRequest {
369 - private $fh;
370 -
371369 protected function urlToTcp( $url ) {
372370 $parsedUrl = parse_url( $url );
373371
——@@ -376,7 +374,7 @@
377375 public function execute() {
378376 if ( $this->parsedUrl['scheme'] != 'http' ) {
379377 $this->status->fatal( 'http-invalid-scheme', $this->parsedURL['scheme'] );
380 - }
 378+ }
381379
382380 parent::execute();
383381 if ( !$this->status->isOK() ) {
——@@ -399,26 +397,33 @@
400398 $options['method'] = $this->method;
401399 $options['timeout'] = $this->timeout;
402400 $options['header'] = implode("\r\n", $this->getHeaderList());
403 - // FOR NOW: Force everyone to HTTP 1.0
404 - /* if ( version_compare( "5.3.0", phpversion(), ">" ) ) { */
405 - $options['protocol_version'] = "1.0";
406 - /* } else { */
407 - /* $options['protocol_version'] = "1.1"; */
408 - /* } */
 401+ // Note that at some future point we may want to support
 402+ // HTTP/1.1, but we'd have to write support for chunking
 403+ // in version of PHP < 5.3.1
 404+ $options['protocol_version'] = "1.0";
409405
410406 if ( $this->postData ) {
411407 $options['content'] = $this->postData;
412408 }
413409
 410+ $oldTimeout = false;
 411+ if ( version_compare( '5.2.1', phpversion(), '>' ) ) {
 412+ $oldTimeout = ini_set('default_socket_timeout', $this->timeout);
 413+ }
 414+
414415 $context = stream_context_create( array( 'http' => $options ) );
415 - try {
416 - $this->fh = fopen( $this->url, "r", false, $context );
417 - } catch ( Exception $e ) {
418 - $this->status->fatal( $e->getMessage() ); /* need some l10n help */
 416+ wfSuppressWarnings();
 417+ $fh = fopen( $this->url, "r", false, $context );
 418+ wfRestoreWarnings();
 419+ if ( $oldTimeout !== false ) {
 420+ ini_set('default_socket_timeout', $oldTimeout);
 421+ }
 422+ if ( $fh === false ) {
 423+ $this->status->fatal( 'http-request-error' );
419424 return $this->status;
420425 }
421426
422 - $result = stream_get_meta_data( $this->fh );
 427+ $result = stream_get_meta_data( $fh );
423428 if ( $result['timed_out'] ) {
424429 $this->status->fatal( 'http-timed-out', $this->url );
425430 return $this->status;
——@@ -426,16 +431,17 @@
427432
428433 $this->headers = $result['wrapper_data'];
429434
430 - $end = false;
431 - while ( !$end ) {
432 - $contents = fread( $this->fh, 8192 );
433 - $size = 0;
434 - if ( $contents ) {
435 - $size = call_user_func_array( $this->callback, array( $this->fh, $contents ) );
 435+ while ( !feof( $fh ) ) {
 436+ $buf = fread( $fh, 8192 );
 437+ if ( $buf === false ) {
 438+ $this->status->fatal( 'http-read-error' );
 439+ break;
436440 }
437 - $end = ( $size == 0 ) || feof( $this->fh );
 441+ if ( strlen( $buf ) ) {
 442+ call_user_func( $this->callback, $fh, $buf );
 443+ }
438444 }
439 - fclose( $this->fh );
 445+ fclose( $fh );
440446
441447 return $this->status;
442448 }
Index: trunk/phase3/languages/messages/MessagesEn.php
——@@ -2154,7 +2154,7 @@
21552155 # HTTP errors
21562156 'http-invalid-url' => 'Invalid URL: $1',
21572157 'http-invalid-scheme' => 'URLs with the "$1" scheme are not supported',
2158 -'http-request-error' => 'Error sending request:',
 2158+'http-request-error' => 'Unknown error sending request.',
21592159
21602160 # Some likely curl errors. More could be added from <http://curl.haxx.se/libcurl/c/libcurl-errors.html>
21612161 'upload-curl-error6' => 'Could not reach URL',

Follow-up revisions

RevisionCommit summaryAuthorDate
r61675follow up r61655...mah19:18, 29 January 2010
r61698Followup r61655, add sys_get_temp_dir() support to wfTempDir(), use this in H...demon23:52, 29 January 2010
r61911follow-up r61655 fill out the rest of the missing messages...mah06:19, 3 February 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r61357Follow up 61352, address TimStarling's comments.mah07:50, 22 January 2010

Comments

#Comment by Nikerabbit (talk | contribs)   13:04, 29 January 2010

+'http-request-error' => 'Unknown error sending request.',

I find this message hard to understand. How about something along HTTP request failed due to unknown error.?

#Comment by Tim Starling (talk | contribs)   01:53, 3 February 2010

You forgot to add http-read-error to MessagesEn.php. And while we're on the subject of messages:

  • http-timed-out is missing
  • Constructing message names of the form "upload-curl-error$code" is a lazy shortcut and will lead to a missing message error when curl returns an unexpected code. It also requires message duplication, one for curl and one for PHP. The robust way to do it is very simple:
class CurlHttpRequest {
    static $curlMessageMap = array( 
        6 => 'http-host-unreachable',
        28 => 'http-timed-out'
    );

...

        if ( isset( self::$curlMessageMap[$code] ) ) {
            $status->fatal( self::$curlMessageMap[$code] );
        } else {
            $status->fatal( 'http-curl-error', curl_error( $curlHandle ) );
        }

...

    'http-curl-error' => 'Error fetching URL: $1',

Then you get an error message in English if there's an unlocalised curl error code, instead of an incomprehensible thing like "<upload-curl-error4>".

Otherwise it's looking good.

#Comment by MarkAHershberger (talk | contribs)   16:15, 3 February 2010

addressed in r61911

Status & tagging log