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

diff --git a/includes/api/ApiUpload.php b/includes/api/ApiUpload.php
index 6a5a1ac..3a8326f 100644 (file)
--- a/includes/api/ApiUpload.php
+++ b/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'] ) ) {

diff --git a/includes/upload/UploadFromStash.php b/includes/upload/UploadFromStash.php
index 7f3789c..1ee720a 100644 (file)
--- a/includes/upload/UploadFromStash.php
+++ b/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;
        }
        

diff --git a/includes/upload/UploadStash.php b/includes/upload/UploadStash.php
index 61c337a..9c526b4 100644 (file)
--- a/includes/upload/UploadStash.php
+++ b/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;

diff --git a/maintenance/archives/patch-uploadstash.sql b/maintenance/archives/patch-uploadstash.sql
index 2d29c3b..2512076 100644 (file)
--- a/maintenance/archives/patch-uploadstash.sql
+++ b/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);
\ No newline at end of file
+CREATE INDEX /*i*/us_timestamp ON /*_*/uploadstash (us_timestamp);

diff --git a/maintenance/cleanupUploadStash.php b/maintenance/cleanupUploadStash.php
index 763f556..4b47f39 100644 (file)
--- a/maintenance/cleanupUploadStash.php
+++ b/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 ) {

diff --git a/maintenance/tables.sql b/maintenance/tables.sql
index 48e0d7b..65c8a72 100644 (file)
--- a/maintenance/tables.sql
+++ b/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,