r61357 MediaWiki - Code Review archive

Thanks for that, it's a big improvement.

parse_url() still has no wfSuppressWarnings()/wfRestoreWarnings(). Also you don't check to see if it returns false, so it will produce lots of PHP warnings in that case in subsequent lines.

 		} catch ( Exception $e ) {
+			$this->status->fatal( $e->getMessage() ); /* need some l10n help */

If there actually was an exception, you could display the message to the user with something like:

$this->status->fatal( 'http-fopen-exception', $e->getMessage() );

and in MessagesEn.php

'http-fopen-exception' => 'Error opening HTTP stream: $1',

However, the issue won't come up, because fopen() doesn't throw exceptions. This is PHP not Python. It'll output a few warnings and then return false.

$ php -d allow_url_fopen=1 eval.php
> $f = fopen('http://blah/','r')
Warning: fopen(): php_network_getaddresses: getaddrinfo failed: Name or service not known in .../maintenance/eval.php(60) : eval()'d code on line 1
Warning: fopen(http://blah/): failed to open stream: php_network_getaddresses: getaddrinfo failed: Name or service not known in .../maintenance/eval.php(60) : eval()'d code on line 1

> var_dump($f)
bool(false)

You should remove the try/catch block and have it check if $this->fh is false instead. If it is, you can return a generic error message. Or if you're feeling really ambitious, you can try to trap the PHP warnings and return them to the user, like what Database::installErrorHandler() and Database::restoreErrorHandler() do.

For the fread() loop, I was more thinking of something like:

while ( !feof( $this->fh ) ) {
    $buf = fread( $this->fh, 8192 );
    if ( $buf === false ) {
        $this->status->fatal( 'http-read-error' );
        break;
    }
    if ( strlen( $buf ) ) {
        call_user_func( $this->callback, $this->fh, $buf );
    }
}

You can't use "if ( $contents )" because that will omit strings like "00000".