r61352 MediaWiki - Code Review archive

If striping the async download what is the plan to enable downloading files for more than php-execution time?

Why was this not included in part of the re-factor? Will this add even more delay to bug 20512?

Tim asked me to take it out. It will probably be re-done as something in the JobQueue.

Async download should be done with concurrency control, it shouldn't be a forkbomb exploitable by anyone with upload privileges. We have a system for this already: the job queue. For example, when EmailNotification needs to send emails to hundreds of users due to a change to a commonly-watched article, it does it by queueing EnotifNotifyJob jobs, which are then executed, with minimal delay, by dedicated runJobs.php threads which run with --type=enotifNotify. The same system could easily be used for async download.

However, async download is not a high priority compared to restoring basic MW 1.15 behaviour such as a working Http::get(), in preparation for a 1.16 release.

Since you're calling it a rewrite, I'll review the whole file as of r61353:

if ( !array_key_exists( 'timeout', $opts ) ) {

Use isset. Several other instances.

protected $no_proxy = false;
protected $parsed_url;

Use camel case not underscores.

if ( !ini_get( 'allow_url_fopen' ) ) {
	throw new MWException( 'allow_url_fopen needs to be enabled for http requests to work' );

That would be partially true if this code were in PhpHttpRequest, it's certainly wrong in the parent HttpRequest. allow_url_fopen is a security vulnerability and people should not be encouraged to enable it in their php.ini. The error message should promote curl as the preferred solution.

throw new MWException( 'Invalid URL' );

Exception messages should have enough context to allow programmers to work out what the problem is, when users report the error message only with backtraces disabled. That usually means something like:

throw new MWException( __METHOD__.': Invalid URL' );

However, if it's possible for the URL to be user input, then this error should be returned in the status instead of thrown as an exception.

function __construct( $url = null, $opt = array() ) {

It's a bit weird to have a default of null and then throw an exception if the user accepts that default. I think the URL sanity check should be moved to execute(), where a Status object with a fatal error message can be returned.

Usually array variable names use the plural, e.g. $options not $opt or $option.

if ( is_array( $this->postdata ) ) {

Use camel case, e.g. postData.

$this->initRequest();

Init functions are usually an indication of a fragile architecture with confused data flow. initRequest() is undocumented, and there is no documentation of what it initialises, or of what functions will break if they are called before it. I suggest using an accessor function CurlHttpRequest::getHandle() which initialises the member variable on the first request.

The calls of proxySetup() and setReferrer() should be moved to execute() so that initialisation of the curl handle can be deferred until then, and so that the user can do further configuration of the object using set*() functions before execute() is called.

	/**
	 * For backwards compatibility, we provide a __toString method so
	 * that any code that expects a string result from Http::Get()
	 * will see the content of the request.
	 */
	function __toString() {
		return $this->content;
	}

Http::get() should continue to return a string, since the implementation of __toString() in PHP has a few gaps where conversion is not done, and bugs can appear. This __toString() should be removed. Callers which wish to have an HttpRequest can construct one, using HttpRequest::factory().

To be continued.

throw new MWException( 'FIXME' );

Needs informative error message.

Another thing on this:

if ( !ini_get( 'allow_url_fopen' ) ) {

You need to use wfIniGetBool() to get boolean ini parameters, since ini_get() can return strings such as "off" which PHP will interpret as false.

$this->parsed_url = parse_url( $url );

parse_url() spews warnings if you give it malformed input, so if this is user input then it needs to be wrapped with wfSuppressWarnings()/wfRestoreWarnings(). One other instance.

if ( $this->headersOnly ) {
	curl_setopt( $this->curlHandle, CURLOPT_NOBODY, true );
	curl_setopt( $this->curlHandle, CURLOPT_HEADER, true );

It seems kind of weird to me that this is an option to Http::get() instead of being requested using Http::request('HEAD', ...). It's like the weird curl_setopt() interface is being copied by MW when it could be hidden.

$this->setCallback( array( $this, 'read' ) );

Theoretically you could use CURLOPT_RETURNTRANSFER instead of a callback if $this->curlCBSet is false, it might save a few lines of code and a few microseconds. It would also allow you to remove the condition from setCallback() so that calling code like this would work:

$req->setCallback( array( $this, 'foo' ) );
if ( $useBar ) {
    $req->setCallback( array( $this, 'bar' ) );
}

I see there's no documentation about the fact that this doesn't work presently.

$this->status->fatal( 'Error sending request (#$1): $2',

The first parameter to Status::fatal() is a message key, not English text. Several other instances.

try {
	if ( false === curl_exec( $this->curlHandle ) ) {
	...
} catch ( Exception $e ) {

I'm not sure what the try{} is for, very few PHP functions throw exceptions, and curl_exec() is not one of them. Maybe it could be used to catch errors thrown from the CURLOPT_WRITEFUNCTION callback, but if so, it lacks any method for getting an error message from the exception and passing it on to the user. And if it's meant to catch exceptions thrown from MW, it should be catching MWException instead of Exception.

CurlHttpRequest::getCode() seems to be broken, it doesn't return anything. If the aim is to set some member variable of $this->status then maybe it should be called something different.

while ( !$end ) {
	$contents = fread( $this->fh, 8192 );
	$size = call_user_func_array( $this->callback, array( $this->fh, $contents ) );
	$end = ( $size == 0 )  || feof( $this->fh );
}

fread() will return false if there is an error, in which case the callback should not be called, the loop should be exit immediately, and a meaningful error message should be returned to the user. fread() will return an empty string on EOF, although this is not a documented method of determining EOF, so it may do it in other situations as well. I suggest using feof() only to determine when the file has ended.

Handled in r61357