UploadBase: Stop mLocalFile doubling as stashed file

"I've a great idea", they said. "You know what would be cool? If I
made this boring getter, getLocalFile(), return something completely
different after the file was stashed. This will be a nice surprise for
someone in the future to discover", they added gleefully.

I am pretty sure everything still works, but I never could get async
upload publishing to work locally, so I'd appreciate some testing.

Change-Id: I11dcf2ed89e4f1dd8ddf081af521da005efdbf39

diff --git a/includes/api/ApiUpload.php b/includes/api/ApiUpload.php
index fc2fd59..227278c 100644 (file)
--- a/includes/api/ApiUpload.php
+++ b/includes/api/ApiUpload.php
@@ -278,7 +278,7 @@ class ApiUpload extends ApiBase {
                                // the old filekey and fetch the new one.
                                UploadBase::setSessionStatus( $this->getUser(), $filekey, false );
                                $this->mUpload->stash->removeFile( $filekey );
-                               $filekey = $this->mUpload->getLocalFile()->getFileKey();
+                               $filekey = $this->mUpload->getStashFile()->getFileKey();
 
                                $result['result'] = 'Success';
                        }
@@ -735,7 +735,7 @@ class ApiUpload extends ApiBase {
                        $this->mParams['text'] = $this->mParams['comment'];
                }
 
-               /** @var $file File */
+               /** @var $file LocalFile */
                $file = $this->mUpload->getLocalFile();
 
                // For preferences mode, we want to watch if 'watchdefault' is set,

diff --git a/includes/jobqueue/jobs/AssembleUploadChunksJob.php b/includes/jobqueue/jobs/AssembleUploadChunksJob.php
index 1e804c4..f3fcadf 100644 (file)
--- a/includes/jobqueue/jobs/AssembleUploadChunksJob.php
+++ b/includes/jobqueue/jobs/AssembleUploadChunksJob.php
@@ -74,7 +74,7 @@ class AssembleUploadChunksJob extends Job {
                        }
 
                        // We have a new filekey for the fully concatenated file
-                       $newFileKey = $upload->getLocalFile()->getFileKey();
+                       $newFileKey = $upload->getStashFile()->getFileKey();
 
                        // Remove the old stash file row and first chunk file
                        $upload->stash->removeFileNoAuth( $this->params['filekey'] );

diff --git a/includes/upload/UploadBase.php b/includes/upload/UploadBase.php
index ae16f70..e2f7763 100644 (file)
--- a/includes/upload/UploadBase.php
+++ b/includes/upload/UploadBase.php
@@ -44,7 +44,7 @@ abstract class UploadBase {
        protected $mDesiredDestName, $mDestName, $mRemoveTempFile, $mSourceType;
        protected $mTitle = false, $mTitleError = 0;
        protected $mFilteredName, $mFinalExtension;
-       protected $mLocalFile, $mFileSize, $mFileProps;
+       protected $mLocalFile, $mStashFile, $mFileSize, $mFileProps;
        protected $mBlackListedExtensions;
        protected $mJavaDetected, $mSVGNSError;
 
@@ -912,7 +912,7 @@ abstract class UploadBase {
        /**
         * Return the local file and initializes if necessary.
         *
-        * @return LocalFile|UploadStashFile|null
+        * @return LocalFile|null
         */
        public function getLocalFile() {
                if ( is_null( $this->mLocalFile ) ) {
@@ -923,6 +923,13 @@ abstract class UploadBase {
                return $this->mLocalFile;
        }
 
+       /**
+        * @return UploadStashFile|null
+        */
+       public function getStashFile() {
+               return $this->mStashFile;
+       }
+
        /**
         * Like stashFile(), but respects extensions' wishes to prevent the stashing. verifyUpload() must
         * be called before calling this method (unless $isPartial is true).
@@ -997,7 +1004,7 @@ abstract class UploadBase {
        protected function doStashFile( User $user = null ) {
                $stash = RepoGroup::singleton()->getLocalRepo()->getUploadStash( $user );
                $file = $stash->stashFile( $this->mTempPath, $this->getSourceType() );
-               $this->mLocalFile = $file;
+               $this->mStashFile = $file;
 
                return $file;
        }
@@ -1975,18 +1982,16 @@ abstract class UploadBase {
         * @return array Image info
         */
        public function getImageInfo( $result ) {
-               $file = $this->getLocalFile();
-               /** @todo This cries out for refactoring.
-                *  We really want to say $file->getAllInfo(); here.
-                * Perhaps "info" methods should be moved into files, and the API should
-                * just wrap them in queries.
-                */
-               if ( $file instanceof UploadStashFile ) {
+               $localFile = $this->getLocalFile();
+               $stashFile = $this->getStashFile();
+               // Calling a different API module depending on whether the file was stashed is less than optimal.
+               // In fact, calling API modules here at all is less than optimal. Maybe it should be refactored.
+               if ( $stashFile ) {
                        $imParam = ApiQueryStashImageInfo::getPropertyNames();
-                       $info = ApiQueryStashImageInfo::getInfo( $file, array_flip( $imParam ), $result );
+                       $info = ApiQueryStashImageInfo::getInfo( $stashFile, array_flip( $imParam ), $result );
                } else {
                        $imParam = ApiQueryImageInfo::getPropertyNames();
-                       $info = ApiQueryImageInfo::getInfo( $file, array_flip( $imParam ), $result );
+                       $info = ApiQueryImageInfo::getInfo( $localFile, array_flip( $imParam ), $result );
                }
 
                return $info;

diff --git a/includes/upload/UploadFromChunks.php b/includes/upload/UploadFromChunks.php
index 6368db8..9145a85 100644 (file)
--- a/includes/upload/UploadFromChunks.php
+++ b/includes/upload/UploadFromChunks.php
@@ -76,18 +76,18 @@ class UploadFromChunks extends UploadFromFile {
 
                $this->verifyChunk();
                // Create a local stash target
-               $this->mLocalFile = parent::doStashFile( $user );
+               $this->mStashFile = parent::doStashFile( $user );
                // Update the initial file offset (based on file size)
-               $this->mOffset = $this->mLocalFile->getSize();
-               $this->mFileKey = $this->mLocalFile->getFileKey();
+               $this->mOffset = $this->mStashFile->getSize();
+               $this->mFileKey = $this->mStashFile->getFileKey();
 
                // Output a copy of this first to chunk 0 location:
-               $this->outputChunk( $this->mLocalFile->getPath() );
+               $this->outputChunk( $this->mStashFile->getPath() );
 
                // Update db table to reflect initial "chunk" state
                $this->updateChunkStatus();
 
-               return $this->mLocalFile;
+               return $this->mStashFile;
        }
 
        /**
@@ -158,7 +158,7 @@ class UploadFromChunks extends UploadFromFile {
                        return $status;
                }
 
-               // Update the mTempPath and mLocalFile
+               // Update the mTempPath and mStashFile
                // (for FileUpload or normal Stash to take over)
                $tStart = microtime( true );
                // This is a re-implementation of UploadBase::tryStashFile(), we can't call it because we
@@ -169,14 +169,14 @@ class UploadFromChunks extends UploadFromFile {
                        return $status;
                }
                try {
-                       $this->mLocalFile = parent::doStashFile( $this->user );
+                       $this->mStashFile = parent::doStashFile( $this->user );
                } catch ( UploadStashException $e ) {
                        $status->fatal( 'uploadstash-exception', get_class( $e ), $e->getMessage() );
                        return $status;
                }
 
                $tAmount = microtime( true ) - $tStart;
-               $this->mLocalFile->setLocalReference( $tmpFile ); // reuse (e.g. for getImageInfo())
+               $this->mStashFile->setLocalReference( $tmpFile ); // reuse (e.g. for getImageInfo())
                wfDebugLog( 'fileconcatenate', "Stashed combined file ($i chunks) in $tAmount seconds." );
 
                return $status;