r92009 MediaWiki - Code Review archive

Don't use 'smallint' for things like width/height; there are plenty of formats allowing larger images than 32k x 32k... and we can expect to be the ones to get them. ;)

It looks like there's no actual need to change the API responses either; this should be entirely an internally-facing change that clients need not do anything about.

Fixed the smallint thing in r92035.

Regarding the API and sessionkey referring to an upload session, here's what I wrote in the email thread on wikitech-l (pasted here because it makes more sense here):

Neilk and I talked about that, but it seemed misleading. While it could be argued that the UploadWizard implements the concept of an upload session, it doesn't really carry through to the API. Each key is linked to a specific file, whereas I'd expect an upload session to be an abstraction that could contain multiple files. Previously, sessionkey was really referring to the file key that was stored in the session.

We decided that changing it made sense since it was possible to deprecate the old name without removing it, which gave us both clarity and functionality. Had that been impossible, leaving it unchanged was the next-best option.

+       public function __construct( $stash = false, $repo = false ) {
+               if( !$this->repo ) {
+                       $this->repo = RepoGroup::singleton()->getLocalRepo();
+               }

This is not what you meant I think. You want if ( $repo ) { $this->repo = $repo } else { $this->repo = RepoGroup::singleton()->getLocalRepo(); }

       public function getFile( $key ) {
+               global $wgUser;
<pre>
We should avoid new uses of $wgUser. Pass a user object as parameter to getFile().

<pre>
+                               $lag = $dbr->getLag();

$dbr is undefined at this point.

+                       'us_timestamp' => wfTimestamp( TS_MW )

This breaks postgres, use $dbw->timestamp()

+               $dbw = wfGetDB( DB_MASTER );
+               $dbw->insert(
+                       'uploadstash',
+                       $this->fileMetadata[$key],
+                       __METHOD__
+               );

The uploadstash table is repobound, so you should use $repo->getMasterDb().

+               $dbw->delete(
+                       'uploadstash',
+                       array( 'us_key' => $key, 'us_user' => $userId ),
+                       __METHOD__
+               );

us_key is a UNIQUE INDEX, so you don't need the condition on us_user.

+               while( $row = $dbr->fetchRow( $res ) ) {
+                       array_push( $keys, $row['us_key'] );
+               }

fetchRow returns objects, not arrays iirc. You should not use while {}, but iterate over $res with foreach.

+       us_media_type varchar(255),

This is an enum in image, so for consistency this should also be an enum, but on the other hand enums suck. Hmmm...

Thanks for the info. I'm now pulling user info from ApiBase::createContext (though if it's truly deprecated, Manual:$wgUser should probably be updated). I'm still using $wgUser when called from Filerepo::getUploadStash, since Filerepo doesn't seem to be aware of the current user.

I went ahead and changed the type of us_media_type for consistency... I figure if it ever changes to a not-enum, it won't be hard to do it in both places.

Timestamp generation is corrected (and I'm glad to see the move to a function that might make datetime columns possible someday). However, Manual:WfTimestamp should probably mention that this is the right way to do things. I'd make the documentation changes myself, but I'm still a bit unsure about what the correct conventions are at this point.

Updates are in r92081

Basic documentation added at Manual:Uploadstash_table

In includes/upload/UploadStash.php line 189 onwards, I wish you hadn't changed $status to $storeResult -- it's a hint that this is a Status object (includes/Status.php). If you felt the status needed a more descriptive name, maybe $storeStatus ?

includes/upload/UploadStash.php#230:

  wfDebugCallstack(); -- we shouldn't have this in production...

In includes/api/ApiQueryStashImageInfo.php#63 you added a TODO for some new exceptions -- fix that?

• UploadStashNotLoggedInException
• UploadStashMaxLagExceededException

(others?)

In general this change was too big, and it incorporates a name change to one API parameter, along with more substantive changes. It would be easier to review if those (at least) were separate.

The rename from mwe-upwiz-api-error-invalid-session-key to mwe-upwiz-api-error-invalid-file-key is incomplete: in the i18n file, you only changed the message name for Finnish (fi) and left all other languages, including English (!!) alone.

You are totally right. So much for search/replace, I guess. Fixed in r94526

+		return self::isValidKey( $request->getText( 'wpFileKey' ) || $request->getText( 'wpSessionKey' ) );
...
+		$fileKey = $request->getText( 'wpFileKey' ) || $request->getText( 'wpSessionKey' );
...
+			$desiredDestName = $request->getText( 'wpUploadFile' ) || $request->getText( 'filename' );

This doesn't work. The || operator in PHP doesn't behave like the one in JavaScript or Python or whatever, but more like the one in C:

> $request = new FauxRequest(array('wpFileKey' => 'foo', 'wpSessionKey' => 'bar'));

> var_dump($request->getText( 'wpSessionKey' ) || $request->getText( 'wpSessionKey' ))
bool(true)

Instead, what you want to use is the second parameter to getText() (and most of the other getters in WebRequest), which is a default value that is returned if the key you asked for isn't set. So something like $request->getText( 'wpFileKey', $request->getText( 'wpSessionKey', 'defaultValueIfNeitherIsSet,DefaultsToEmptyString' ) ) would do what you want.

I'm confused, though: I don't have a very good understanding of the code, but it seems to me like this accidental creation of booleans that then get converted back to strings must break something, somewhere. Is there some devious reason why this doesn't break things, or at least doesn't appear to during basic testing?

+		if ( !empty( $this->mLocalFile ) ) {

This looks a lot like an inappropriate use of empty().

The waiting-for-lag behavior is unnecessarily complex, scary and fragile. If your data doesn't appear due to lag, fetch it from the master instead. This probably involves adding a parameter to fetchFileMetadata() to use the master instead of the slave.

	public function listFiles() {
...
			array( 'us_key' => $key ),

$key is undefined, and this condition probably breaks listFiles() entirely.

+		$this->fileMetadata[$key] = array(
+			'us_user' => $row->us_user,
+			'us_key' => $row->us_key,
+			'us_orig_path' => $row->us_orig_path,
+			'us_path' => $row->us_path,
+			'us_size' => $row->us_size,
+			'us_sha1' => $row->us_sha1,
+			'us_mime' => $row->us_mime,
+			'us_media_type' => $row->us_media_type,
+			'us_image_width' => $row->us_image_width,
+			'us_image_height' => $row->us_image_height,
+			'us_image_bits' => $row->us_image_bits,
+			'us_source_type' => $row->us_source_type,
+			'us_timestamp' => $row->us_timestamp,
+			'us_status' => $row->us_status
+		);

You can also use $this->fileMetadata[$key] = (array)$row; .

In UploadStash.php line 50, the function is returning a boolean value. So that's correct to use ||.

The other cases (line 77,81) I agree. That is perplexing. Not sure why this even works now.

You're misreading the parentheses on line 50. The || is used to compute the parameter to the function instead of being applied to the return value.

You're right, my fault.

Ian - you can check if the listFiles() function is broken by uploading a couple of files, and then not proceeding to describe them. In another tab open: http://yoursite/wiki/Special:UploadStash

I think I understand the problem with initializeFromRequest. That code path isn't even touched by UploadWizard, since it uses the API. Only Special:Upload uses it. To trigger it, you have to craft a problem with the file that won't be caught by Special:Upload, which will cause it to be stashed. An example might be to try a file like SANY_12345.JPG, because (if you copied the Commons config) that's a Blacklisted title but isn't caught by any hardcoded checks. Then, even if you fix the stash, nothing seems to happen; you get sent back to an "empty" upload form.

re: || operator, I have no idea where I got the notion that such a thing would work. Looking at it myself, I'm like, "WTF?" Fixed. I'm not entirely sure why this didn't cause serious problems either, but I've added some tests to cover this code since we needed them anyway.

regarding usage of empty(): I was just duplicating the behavior that was there before (Reedy's, I believe--all I changed is the name of the variable). However, I think you're right, and I've changed it to !.

re: listfiles(), it should be checking against the user. Fixed.

Regarding lag, I based my decision on the following statement in Database Access/Working with lag:

"To avoid swamping the master every time the slaves lag, use of this approach should be kept to a minimum. In most cases you should just read from the slave and let the user deal with the delay."

UploadWizard could generate a page that requires a select for each of many images. I'm concerned about the load that could place on the master. I'll totally change it if you think it's best, but are you sure?

Anyway, this is all implemented in r94536 except the lag stuff.

What you did is not what we meant with "let the user deal with the delay". You're supposed to either present the user with a page with the caveat that it might be out of date, or read from the slave first and if you find the data is out of date in a nasty way (like, something doesn't exist yet but you suspect it should), read from the master.

Got it. Fixed in r94592.