r62806 MediaWiki - Code Review archive

Seen at translatewiki:

PHP Fatal error: Declaration of FSRepo::append() must be compatible with that of FileRepo::append() in /www/w/includes/filerepo/FSRepo.php on line 8

Fixed with r62824 by demon. Thanks :)

fopen should open in wb mode for Windows compatibility.

Changing status back to "new" as long as this commit isn't reviewed properly.

In UploadFromChunksTest.php:

ini_set( 'file_loads', 1 );

Presumably this is meant to be file_uploads. Either way, it won't do anything in CLI mode.

In UploadFromChunks.php:

$this->status = Status::newFatal( 'chunk-init-error' );

I'm getting tired of saying this: this message does not exist, you need to add it to MessagesEn.php. When you write a line of code which uses a new message, like Status::newFatal(), do not make a mental note to add the message later. You need to either add the message immediately, or make a note in some more reliable location than your head stating that it needs to be done later. Then when you are ready to commit, generate a diff and review it. For each new message reference in the diff, check it against the message additions in MessagesEn.php.

Mental notes are for poor people who can't afford real notes.

$this->setupChunkSession();

This is a PHP warning, not enough parameters.

• When a file is uploaded via chunk upload, when done=1 and there are warnings, the file isn't stashed properly. Only the last chunk is stashed.

 		// testing against null because we don't want to cause obscure
 		// bugs when $sessionKey is full of "0"
-		if ( $sessionKey === null ) {
-			return;
-		}

Comment above doesn't make sense anymore

ApiUpload.php:

"Verify that Status::getErrorsArray() (at least where it is used in ApiUpload::execute()) returns an array that we can pass to dieUsageMessage()"

What are you talking about? Due to a fragile accident, it produces an output which looks vaguely like an appropriate internal error, but not without issuing a PHP warning "Illegal offset type in isset or empty in .../includes/api/ApiBase.php on line 959". If you want it to work beyond the next minor refactor, you should follow the documentation.

FSRepo.php:

+		if ( $flags & self::DELETE_SOURCE ) {
 		unlink( $toAppendPath );
+		}

Good, but you didn't update the caller in UploadFromChunks::appendToUploadFile(), so the temporary file won't be deleted anymore (except by PHP on request shutdown but it's OK to delete it here as well).

After looking at the Firefogg protocol in more detail, I'm pretty convinced that it doesn't belong in action=upload. The Firefogg protocol is idiosyncratic and has several serious design flaws. I think the code here should be split out to an extension, in a separate API module (say action=firefogg). Since I want to release 1.16 within a day or two, pulling this code out of the 1.16 branch, and aiming for an extension that works against 1.17, seems like a good approach. All of the work you've done on it will still be applicable, there will just be a few tweaks to make it work as a standalone module.

The remaining API upload code has some issues as well, but maybe we can sort them out before a 1.16 release.

re: 'chunk-init-error' and 'invalid-session' (for which no key exists in MessagesEn.php)

The messages don't exist there because the resulting Status object is just passed to dieUsageMessage which uses a key from ApiBase.php to look up the (untranslated) message.

With the problem in dieUsageMsg() & getErrorsArray(), I will change this (when it is moved to an extension) so that error reporting from initialization doesn't use Status objects.

The code has been backed out and put into an extension that I'm working on. See r63638, r63621, r64425, and r64419.