Added us_status column for future expansion re Bryan's suggestion

Implemented changes suggested in code review on r92009:
 constructor bug handling passed repo/stash
 up-to-date timestamp generation
 fetching db handle from repo
 iterating over select results according to convention
 changed uploadstash.us_media_type to enum to mirror image.img_media_type
 removed (most) new references to $wgUser, instead using ApiBase::createContext to find the user

includes/api/ApiUpload.php

@@ -220,7 +220,9 @@ class ApiUpload extends ApiBase {
 				$this->dieUsageMsg( 'invalid-file-key' );
 			}
 
-			$this->mUpload = new UploadFromStash();
+			// context allows access to the current user without creating new $wgUser references
+			$context = $this->createContext();
+			$this->mUpload = new UploadFromStash( $context->getUser() );
 			$this->mUpload->initialize( $this->mParams['filekey'], $this->mParams['filename'] );
 
 		} elseif ( isset( $this->mParams['file'] ) ) {

includes/upload/UploadFromStash.php

@@ -16,15 +16,23 @@ class UploadFromStash extends UploadBase {
 	//LocalFile repo
 	private $repo;
 	
-	public function __construct( $stash = false, $repo = false ) {
-		if( !$this->repo ) {
+	public function __construct( $user = false, $stash = false, $repo = false ) {
+		// user object. sometimes this won't exist, as when running from cron.
+		$this->user = $user;
+
+		if( $repo ) {
+			$this->repo = $repo;
+		} else {
 			$this->repo = RepoGroup::singleton()->getLocalRepo();
 		}
 
-		if( !$this->stash ) {
-			$this->stash = new UploadStash( $this->repo );
+		if( $stash ) {
+			$this->stash = $stash;
+		} else {
+			wfDebug( __METHOD__ . " creating new UploadStash instance for " . $user->getId() . "\n" );
+			$this->stash = new UploadStash( $this->repo, $this->user );
 		}
-		
+
 		return true;
 	}
 	

includes/upload/UploadStash.php

@@ -42,6 +42,9 @@ class UploadStash {
 	
 	// fileprops cache
 	protected $fileProps = array();
+	
+	// current user
+	protected $user, $userId, $isLoggedIn;
 
 	/**
 	 * Represents a temporary filestore, with metadata in the database.
@@ -49,16 +52,30 @@ class UploadStash {
 	 *
 	 * @param $repo FileRepo
 	 */
-	public function __construct( $repo ) {
+	public function __construct( $repo, $user = null ) {
 		// this might change based on wiki's configuration.
 		$this->repo = $repo;
+	
+		// if a user was passed, use it. otherwise, attempt to use the global.
+		// this keeps FileRepo from breaking when it creates an UploadStash object
+		if( $user ) {
+			$this->user = $user;
+		} else {
+			global $wgUser;
+			$this->user = $wgUser;
+		}
+		
+		if( is_object($this->user) ) {
+			$this->userId = $this->user->getId();
+			$this->isLoggedIn = $this->user->isLoggedIn();
+		}
 	}
 
 	/**
 	 * Get a file and its metadata from the stash.
 	 *
 	 * @param $key String: key under which file information is stored
-	 * @param $noauth Boolean (optional) Don't check authentication. Used by maintenance scripts.
+	 * @param $noAuth Boolean (optional) Don't check authentication. Used by maintenance scripts.
 	 * @throws UploadStashFileNotFoundException
 	 * @throws UploadStashNotLoggedInException
 	 * @throws UploadStashWrongOwnerException
@@ -66,19 +83,19 @@ class UploadStash {
 	 * @return UploadStashFile
 	 */
 	public function getFile( $key, $noAuth = false ) {
-		global $wgUser;
 		
 		if ( ! preg_match( self::KEY_FORMAT_REGEX, $key ) ) {
 			throw new UploadStashBadPathException( "key '$key' is not in a proper format" );
 		}
 		
 		if( !$noAuth ) {
-			$userId = $wgUser->getId();
-			if( !$userId ) {
-				throw new UploadStashNotLoggedInException( 'No user is logged in, files must belong to users' );
+			if( !$this->isLoggedIn ) {
+				throw new UploadStashNotLoggedInException( __METHOD__ . ' No user is logged in, files must belong to users' );
 			}
 		}
 		
+		$dbr = $this->repo->getSlaveDb();
+		
 		if ( !isset( $this->fileMetadata[$key] ) ) {
 			// try this first.  if it fails to find the row, check for lag, wait, try again. if its still missing, throw an exception.
 			// this more complex solution keeps things moving for page loads with many requests 
@@ -115,7 +132,7 @@ class UploadStash {
 		}
 		
 		if( !$noAuth ) {
-			if( $this->fileMetadata[$key]['us_user'] != $userId ) {
+			if( $this->fileMetadata[$key]['us_user'] != $this->userId ) {
 				throw new UploadStashWrongOwnerException( "This file ($key) doesn't belong to the current user." );
 			}
 		}
@@ -157,7 +174,6 @@ class UploadStash {
 	 * @return UploadStashFile: file, or null on failure
 	 */
 	public function stashFile( $path, $sourceType = null, $key = null ) {
-		global $wgUser;
 		if ( ! file_exists( $path ) ) {
 			wfDebug( __METHOD__ . " tried to stash file at '$path', but it doesn't exist\n" );
 			throw new UploadStashBadPathException( "path doesn't exist" );
@@ -214,13 +230,17 @@ class UploadStash {
 		$stashPath = $storeResult->value;
 		
 		// fetch the current user ID
-		$userId = $wgUser->getId();
-		if( !$userId ) {
-			throw new UploadStashNotLoggedInException( "No user is logged in, files must belong to users" );
+		if( !$this->isLoggedIn ) {
+			wfDebugCallstack();
+			throw new UploadStashNotLoggedInException( __METHOD__ . ' No user is logged in, files must belong to users' );
 		}
 
+		// insert the file metadata into the db.
+		wfDebug( __METHOD__ . " inserting $stashPath under $key\n" );
+		$dbw = $this->repo->getMasterDb();
+		
 		$this->fileMetadata[$key] = array(
-			'us_user' => $userId,
+			'us_user' => $this->userId,
 			'us_key' => $key,
 			'us_orig_path' => $path,
 			'us_path' => $stashPath,
@@ -232,12 +252,11 @@ class UploadStash {
 			'us_image_height' => $fileProps['height'],
 			'us_image_bits' => $fileProps['bits'],
 			'us_source_type' => $sourceType,
-			'us_timestamp' => wfTimestamp( TS_MW )
+			'us_timestamp' => $dbw->timestamp(),
+			'us_status' => 'finished'
 		);
 				
-		// insert the file metadata into the db.
-		wfDebug( __METHOD__ . " inserting $stashPath under $key\n" );
-		$dbw = wfGetDB( DB_MASTER );
+
 		$dbw->insert(
 			'uploadstash',
 			$this->fileMetadata[$key],
@@ -261,18 +280,15 @@ class UploadStash {
 	 * @return boolean: success
 	 */
 	public function clear() {
-		global $wgUser;
-		
-		$userId = $wgUser->getId();
-		if( !$userId ) {
-			throw new UploadStashNotLoggedInException( 'No user is logged in, files must belong to users' );
+		if( !$this->isLoggedIn ) {
+			throw new UploadStashNotLoggedInException( __METHOD__ . ' No user is logged in, files must belong to users' );
 		}
 		
 		wfDebug( __METHOD__ . " clearing all rows for user $userId\n" );
-		$dbw = wfGetDB( DB_MASTER );
+		$dbw = $this->repo->getMasterDb();
 		$dbw->delete(
 			'uploadstash',
-			array( 'us_user' => $userId ),
+			array( 'us_user' => $this->userId ),
 			__METHOD__	
 		);
 
@@ -291,14 +307,11 @@ class UploadStash {
 	 * @return boolean: success
 	 */
 	public function removeFile( $key ){
-		global $wgUser;
-		
-		$userId = $wgUser->getId();
-		if( !$userId ) {
-			throw new UploadStashNotLoggedInException( 'No user is logged in, files must belong to users' );
+		if( !$this->isLoggedIn ) {
+			throw new UploadStashNotLoggedInException( __METHOD__ . ' No user is logged in, files must belong to users' );
 		}
 		
-		$dbw = wfGetDB( DB_MASTER );
+		$dbw = $this->repo->getMasterDb();
 		
 		// this is a cheap query. it runs on the master so that this function still works when there's lag.
 		// it won't be called all that often.
@@ -309,7 +322,7 @@ class UploadStash {
 			__METHOD__
 		);
 		
-		if( $row->us_user != $userId ) {
+		if( $row->us_user != $this->userId ) {
 			throw new UploadStashWrongOwnerException( "Can't delete: the file ($key) doesn't belong to this user." );
 		}
 		
@@ -325,7 +338,7 @@ class UploadStash {
 	public function removeFileNoAuth( $key ) {
 		wfDebug( __METHOD__ . " clearing row $key\n" );
 
-		$dbw = wfGetDB( DB_MASTER );
+		$dbw = $this->repo->getMasterDb();
 		
 		// this gets its own transaction since it's called serially by the cleanupUploadStash maintenance script
 		$dbw->begin();
@@ -353,14 +366,11 @@ class UploadStash {
 	 * @return Array
 	 */
 	public function listFiles() {
-		global $wgUser;
-		
-		$userId = $wgUser->getId();
-		if( !$userId ) {
-			throw new UploadStashNotLoggedInException( 'No user is logged in, files must belong to users' );
+		if( !$this->isLoggedIn ) {
+			throw new UploadStashNotLoggedInException( __METHOD__ . ' No user is logged in, files must belong to users' );
 		}
 		
-		$dbw = wfGetDB( DB_SLAVE );
+		$dbr = $this->repo->getSlaveDb();
 		$res = $dbr->select(
 			'uploadstash',
 			'us_key',
@@ -368,14 +378,15 @@ class UploadStash {
 			__METHOD__
 		);
 
-		if( !is_object( $res ) ) {
-			// nothing there.
+		if( !is_object( $res ) || $res->numRows() == 0 ) {
+			// nothing to do.
 			return false;
 		}
 
+		// finish the read before starting writes.
 		$keys = array();
-		while( $row = $dbr->fetchRow( $res ) ) {
-			array_push( $keys, $row['us_key'] );
+		foreach($res as $row) {
+			array_push( $keys, $row->us_key );
 		}
 
 		return $keys;
@@ -419,7 +430,7 @@ class UploadStash {
 	 */
 	protected function fetchFileMetadata( $key ) {
 		// populate $fileMetadata[$key]
-		$dbr = wfGetDB( DB_SLAVE );
+		$dbr = $this->repo->getSlaveDb();
 		$row = $dbr->selectRow(
 			'uploadstash',
 			'*',
@@ -444,7 +455,9 @@ class UploadStash {
 			'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_source_type' => $row->us_source_type,
+			'us_timestamp' => $row->us_timestamp,
+			'us_status' => $row->us_status
 		);
 		
 		return true;

maintenance/archives/patch-uploadstash.sql

@@ -1,5 +1,5 @@
 --
--- Store information about newly uploaded files before they're
+-- Store information about newly uploaded files before they're 
 -- moved into the actual filestore
 --
 CREATE TABLE /*_*/uploadstash (
@@ -22,7 +22,9 @@ CREATE TABLE /*_*/uploadstash (
 	us_source_type varchar(50),
 	
 	-- the date/time on which the file was added
-	us_timestamp varchar(14) not null,
+	us_timestamp varbinary(14) not null,
+	
+	us_status varchar(50) not null,
 
 	-- file properties from File::getPropsFromPath.  these may prove unnecessary.
 	--
@@ -30,7 +32,8 @@ CREATE TABLE /*_*/uploadstash (
 	-- this hash comes from File::sha1Base36(), and is 31 characters
 	us_sha1 varchar(31) NOT NULL,
 	us_mime varchar(255),
-	us_media_type varchar(255),
+	-- Media type as defined by the MEDIATYPE_xxx constants, should duplicate definition in the image table
+  	us_media_type ENUM("UNKNOWN", "BITMAP", "DRAWING", "AUDIO", "VIDEO", "MULTIMEDIA", "OFFICE", "TEXT", "EXECUTABLE", "ARCHIVE") default NULL,
 	-- image-specific properties
 	us_image_width int unsigned,
 	us_image_height int unsigned,
@@ -43,4 +46,4 @@ CREATE INDEX /*i*/us_user ON /*_*/uploadstash (us_user);
 -- pick out files by key, enforce key uniqueness
 CREATE UNIQUE INDEX /*i*/us_key ON /*_*/uploadstash (us_key);
 -- the abandoned upload cleanup script needs this
-CREATE INDEX /*i*/us_timestamp ON /*_*/uploadstash (us_timestamp);
+CREATE INDEX /*i*/us_timestamp ON /*_*/uploadstash (us_timestamp);

maintenance/cleanupUploadStash.php

@@ -35,32 +35,33 @@ class UploadStashCleanup extends Maintenance {
   	}
 
   	public function execute() {
-		$dbr = wfGetDB( DB_SLAVE );
+		$repo = RepoGroup::singleton()->getLocalRepo();
+	
+		$dbr = $repo->getSlaveDb();
 		
 		$this->output( "Getting list of files to clean up...\n" );
 		$res = $dbr->select(
 			'uploadstash',
 			'us_key',
-			'us_timestamp < ' . wfTimestamp( TS_MW, time() - UploadStash::REPO_AGE * 3600 ),
+			'us_timestamp < ' . $dbr->timestamp( time() - UploadStash::REPO_AGE * 3600 ),
 			__METHOD__
 		);
 		
-		if( !is_object( $res ) ) {
+		if( !is_object( $res ) || $res->numRows() == 0 ) {
 			// nothing to do.
 			return false;
 		}
 
 		// finish the read before starting writes.
 		$keys = array();
-		while( $row = $dbr->fetchRow( $res ) ) {
-			array_push( $keys, $row['us_key'] );
+		foreach($res as $row) {
+			array_push( $keys, $row->us_key );
 		}
 		
 		$this->output( 'Removing ' . count($keys) . " file(s)...\n" );
 		// this could be done some other, more direct/efficient way, but using
 		// UploadStash's own methods means it's less likely to fall accidentally
 		// out-of-date someday
-		$repo = RepoGroup::singleton()->getLocalRepo();
 		$stash = new UploadStash( $repo );
 		
 		foreach( $keys as $key ) {

maintenance/tables.sql

@@ -962,7 +962,9 @@ CREATE TABLE /*_*/uploadstash (
 	us_source_type varchar(50),
 	
 	-- the date/time on which the file was added
-	us_timestamp varchar(14) not null,
+	us_timestamp varbinary(14) not null,
+	
+	us_status varchar(50) not null,
 
 	-- file properties from File::getPropsFromPath.  these may prove unnecessary.
 	--
@@ -970,7 +972,8 @@ CREATE TABLE /*_*/uploadstash (
 	-- this hash comes from File::sha1Base36(), and is 31 characters
 	us_sha1 varchar(31) NOT NULL,
 	us_mime varchar(255),
-	us_media_type varchar(255),
+	-- Media type as defined by the MEDIATYPE_xxx constants, should duplicate definition in the image table
+  	us_media_type ENUM("UNKNOWN", "BITMAP", "DRAWING", "AUDIO", "VIDEO", "MULTIMEDIA", "OFFICE", "TEXT", "EXECUTABLE", "ARCHIVE") default NULL,
 	-- image-specific properties
 	us_image_width int unsigned,
 	us_image_height int unsigned,