Merge "Introduce UploadStashFile hook, improve API handling of stash errors"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Thu, 11 Aug 2016 04:31:27 +0000 (04:31 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Thu, 11 Aug 2016 04:31:27 +0000 (04:31 +0000)
docs/hooks.txt
includes/api/ApiUpload.php
includes/specials/SpecialUpload.php
includes/upload/UploadBase.php
includes/upload/UploadFromChunks.php
includes/upload/UploadFromStash.php
languages/i18n/en.json
languages/i18n/qqq.json

index 8fa3793..5cf8ffe 100644 (file)
@@ -3373,6 +3373,18 @@ added to the descriptor
 &$radio: Boolean, if source type should be shown as radio button
 $selectedSourceType: The selected source type
 
+'UploadStashFile': Before a file is stashed (uploaded to stash).
+Note that code which has not been updated for MediaWiki 1.28 may not call this
+hook. If your extension absolutely, positively must prevent some files from
+being uploaded, use UploadVerifyFile or UploadVerifyUpload.
+$upload: (object) An instance of UploadBase, with all info about the upload
+$user: (object) An instance of User, the user uploading this file
+$props: (array) File properties, as returned by FSFile::getPropsFromPath()
+&$error: output: If the file stashing should be prevented, set this to the reason
+  in the form of array( messagename, param1, param2, ... ) or a MessageSpecifier
+  instance (you might want to use ApiMessage to provide machine-readable details
+  for the API).
+
 'UploadVerification': DEPRECATED! Use UploadVerifyFile instead.
 Additional chances to reject an uploaded file.
 $saveName: (string) destination file name
index 95659e5..89d9af8 100644 (file)
@@ -64,7 +64,8 @@ class ApiUpload extends ApiBase {
                                $this->dieUsage( 'No upload module set', 'nomodule' );
                        }
                } catch ( UploadStashException $e ) { // XXX: don't spam exception log
-                       $this->handleStashException( $e );
+                       list( $msg, $code ) = $this->handleStashException( get_class( $e ), $e->getMessage() );
+                       $this->dieUsage( $msg, $code );
                }
 
                // First check permission to upload
@@ -112,7 +113,8 @@ class ApiUpload extends ApiBase {
                                $result['imageinfo'] = $this->mUpload->getImageInfo( $this->getResult() );
                        }
                } catch ( UploadStashException $e ) { // XXX: don't spam exception log
-                       $this->handleStashException( $e );
+                       list( $msg, $code ) = $this->handleStashException( get_class( $e ), $e->getMessage() );
+                       $this->dieUsage( $msg, $code );
                }
 
                $this->getResult()->addValue( null, $this->getModuleName(), $result );
@@ -156,20 +158,13 @@ class ApiUpload extends ApiBase {
         */
        private function getStashResult( $warnings ) {
                $result = [];
+               $result['result'] = 'Success';
+               if ( $warnings && count( $warnings ) > 0 ) {
+                       $result['warnings'] = $warnings;
+               }
                // Some uploads can request they be stashed, so as not to publish them immediately.
                // In this case, a failure to stash ought to be fatal
-               try {
-                       $result['result'] = 'Success';
-                       $result['filekey'] = $this->performStash();
-                       $result['sessionkey'] = $result['filekey']; // backwards compatibility
-                       if ( $warnings && count( $warnings ) > 0 ) {
-                               $result['warnings'] = $warnings;
-                       }
-               } catch ( UploadStashException $e ) {
-                       $this->handleStashException( $e );
-               } catch ( Exception $e ) {
-                       $this->dieUsage( $e->getMessage(), 'stashfailed' );
-               }
+               $this->performStash( 'critical', $result );
 
                return $result;
        }
@@ -185,12 +180,7 @@ class ApiUpload extends ApiBase {
                $result['warnings'] = $warnings;
                // in case the warnings can be fixed with some further user action, let's stash this upload
                // and return a key they can use to restart it
-               try {
-                       $result['filekey'] = $this->performStash();
-                       $result['sessionkey'] = $result['filekey']; // backwards compatibility
-               } catch ( Exception $e ) {
-                       $result['warnings']['stashfailed'] = $e->getMessage();
-               }
+               $this->performStash( 'optional', $result );
 
                return $result;
        }
@@ -228,14 +218,7 @@ class ApiUpload extends ApiBase {
                }
 
                if ( $this->mParams['offset'] == 0 ) {
-                       try {
-                               $filekey = $this->performStash();
-                       } catch ( UploadStashException $e ) {
-                               $this->handleStashException( $e );
-                       } catch ( Exception $e ) {
-                               // FIXME: Error handling here is wrong/different from rest of this
-                               $this->dieUsage( $e->getMessage(), 'stashfailed' );
-                       }
+                       $filekey = $this->performStash( 'critical' );
                } else {
                        $filekey = $this->mParams['filekey'];
 
@@ -320,27 +303,57 @@ class ApiUpload extends ApiBase {
        }
 
        /**
-        * Stash the file and return the file key
-        * Also re-raises exceptions with slightly more informative message strings (useful for API)
-        * @throws MWException
-        * @return string File key
+        * Stash the file and add the file key, or error information if it fails, to the data.
+        *
+        * @param string $failureMode What to do on failure to stash:
+        *   - When 'critical', use dieStatus() to produce an error response and throw an exception.
+        *     Use this when stashing the file was the primary purpose of the API request.
+        *   - When 'optional', only add a 'stashfailed' key to the data and return null.
+        *     Use this when some error happened for a non-stash upload and we're stashing the file
+        *     only to save the client the trouble of re-uploading it.
+        * @param array &$data API result to which to add the information
+        * @return string|null File key
         */
-       private function performStash() {
+       private function performStash( $failureMode, &$data = null ) {
                try {
-                       $stashFile = $this->mUpload->stashFile( $this->getUser() );
+                       $status = $this->mUpload->tryStashFile( $this->getUser() );
 
-                       if ( !$stashFile ) {
-                               throw new MWException( 'Invalid stashed file' );
+                       if ( $status->isGood() && !$status->getValue() ) {
+                               // Not actually a 'good' status...
+                               $status->fatal( new ApiRawMessage( 'Invalid stashed file', 'stashfailed' ) );
                        }
-                       $fileKey = $stashFile->getFileKey();
                } catch ( Exception $e ) {
-                       $message = 'Stashing temporary file failed: ' . get_class( $e ) . ' ' . $e->getMessage();
-                       wfDebug( __METHOD__ . ' ' . $message . "\n" );
-                       $className = get_class( $e );
-                       throw new $className( $message );
+                       $debugMessage = 'Stashing temporary file failed: ' . get_class( $e ) . ' ' . $e->getMessage();
+                       wfDebug( __METHOD__ . ' ' . $debugMessage . "\n" );
+                       $status = Status::newFatal( new ApiRawMessage( $e->getMessage(), 'stashfailed' ) );
+               }
+
+               if ( $status->isGood() ) {
+                       $stashFile = $status->getValue();
+                       $data['filekey'] = $stashFile->getFileKey();
+                       // Backwards compatibility
+                       $data['sessionkey'] = $data['filekey'];
+                       return $data['filekey'];
+               }
+
+               if ( $status->getMessage()->getKey() === 'uploadstash-exception' ) {
+                       // The exceptions thrown by upload stash code and pretty silly and UploadBase returns poor
+                       // Statuses for it. Just extract the exception details and parse them ourselves.
+                       list( $exceptionType, $message ) = $status->getMessage()->getParams();
+                       $debugMessage = 'Stashing temporary file failed: ' . $exceptionType . ' ' . $message;
+                       wfDebug( __METHOD__ . ' ' . $debugMessage . "\n" );
+                       list( $msg, $code ) = $this->handleStashException( $exceptionType, $message );
+                       $status = Status::newFatal( new ApiRawMessage( $msg, $code ) );
                }
 
-               return $fileKey;
+               // Bad status
+               if ( $failureMode !== 'optional' ) {
+                       $this->dieStatus( $status );
+               } else {
+                       list( $code, $msg ) = $this->getErrorFromStatus( $status );
+                       $data['stashfailed'] = $msg;
+                       return null;
+               }
        }
 
        /**
@@ -354,12 +367,7 @@ class ApiUpload extends ApiBase {
         * @throws UsageException
         */
        private function dieRecoverableError( $error, $parameter, $data = [], $code = 'unknownerror' ) {
-               try {
-                       $data['filekey'] = $this->performStash();
-                       $data['sessionkey'] = $data['filekey'];
-               } catch ( Exception $e ) {
-                       $data['stashfailed'] = $e->getMessage();
-               }
+               $this->performStash( 'optional', $data );
                $data['invalidparameter'] = $parameter;
 
                $parsed = $this->parseMsg( $error );
@@ -653,49 +661,41 @@ class ApiUpload extends ApiBase {
 
        /**
         * Handles a stash exception, giving a useful error to the user.
-        * @param Exception $e The exception we encountered.
+        * @param string $exceptionType Class name of the exception we encountered.
+        * @param string $message Message of the exception we encountered.
+        * @return array Array of message and code, suitable for passing to dieUsage()
         */
-       protected function handleStashException( $e ) {
-               $exceptionType = get_class( $e );
-
+       protected function handleStashException( $exceptionType, $message ) {
                switch ( $exceptionType ) {
                        case 'UploadStashFileNotFoundException':
-                               $this->dieUsage(
-                                       'Could not find the file in the stash: ' . $e->getMessage(),
+                               return [
+                                       'Could not find the file in the stash: ' . $message,
                                        'stashedfilenotfound'
-                               );
-                               break;
+                               ];
                        case 'UploadStashBadPathException':
-                               $this->dieUsage(
-                                       'File key of improper format or otherwise invalid: ' . $e->getMessage(),
+                               return [
+                                       'File key of improper format or otherwise invalid: ' . $message,
                                        'stashpathinvalid'
-                               );
-                               break;
+                               ];
                        case 'UploadStashFileException':
-                               $this->dieUsage(
-                                       'Could not store upload in the stash: ' . $e->getMessage(),
+                               return [
+                                       'Could not store upload in the stash: ' . $message,
                                        'stashfilestorage'
-                               );
-                               break;
+                               ];
                        case 'UploadStashZeroLengthFileException':
-                               $this->dieUsage(
+                               return [
                                        'File is of zero length, and could not be stored in the stash: ' .
-                                               $e->getMessage(),
+                                               $message,
                                        'stashzerolength'
-                               );
-                               break;
+                               ];
                        case 'UploadStashNotLoggedInException':
-                               $this->dieUsage( 'Not logged in: ' . $e->getMessage(), 'stashnotloggedin' );
-                               break;
+                               return [ 'Not logged in: ' . $message, 'stashnotloggedin' ];
                        case 'UploadStashWrongOwnerException':
-                               $this->dieUsage( 'Wrong owner: ' . $e->getMessage(), 'stashwrongowner' );
-                               break;
+                               return [ 'Wrong owner: ' . $message, 'stashwrongowner' ];
                        case 'UploadStashNoSuchKeyException':
-                               $this->dieUsage( 'No such filekey: ' . $e->getMessage(), 'stashnosuchfilekey' );
-                               break;
+                               return [ 'No such filekey: ' . $message, 'stashnosuchfilekey' ];
                        default:
-                               $this->dieUsage( $exceptionType . ': ' . $e->getMessage(), 'stasherror' );
-                               break;
+                               return [ $exceptionType . ': ' . $message, 'stasherror' ];
                }
        }
 
index bfb52e8..6ca1100 100644 (file)
@@ -339,7 +339,13 @@ class SpecialUpload extends SpecialPage {
         * @param string $message HTML message to be passed to mainUploadForm
         */
        protected function showRecoverableUploadError( $message ) {
-               $sessionKey = $this->mUpload->stashFile()->getFileKey();
+               $stashStatus = $this->mUpload->tryStashFile( $this->getUser() );
+               if ( $stashStatus->isGood() ) {
+                       $sessionKey = $stashStatus->getValue()->getFileKey();
+               } else {
+                       $sessionKey = null;
+                       // TODO Add a warning message about the failure to stash here?
+               }
                $message = '<h2>' . $this->msg( 'uploaderror' )->escaped() . "</h2>\n" .
                        '<div class="error">' . $message . "</div>\n";
 
@@ -368,7 +374,13 @@ class SpecialUpload extends SpecialPage {
                        return false;
                }
 
-               $sessionKey = $this->mUpload->stashFile()->getFileKey();
+               $stashStatus = $this->mUpload->tryStashFile( $this->getUser() );
+               if ( $stashStatus->isGood() ) {
+                       $sessionKey = $stashStatus->getValue()->getFileKey();
+               } else {
+                       $sessionKey = null;
+                       // TODO Add a warning message about the failure to stash here?
+               }
 
                // Add styles for the warning, reused from the live preview
                $this->getOutput()->addModuleStyles( 'mediawiki.special.upload.styles' );
index b620132..03c864a 100644 (file)
@@ -944,6 +944,33 @@ abstract class UploadBase {
                return $this->mLocalFile;
        }
 
+       /**
+        * Like stashFile(), but respects extensions' wishes to prevent the stashing.
+        *
+        * Upload stash exceptions are also caught and converted to an error status.
+        *
+        * @since 1.28
+        * @param User $user
+        * @return Status If successful, value is an UploadStashFile instance
+        */
+       public function tryStashFile( User $user ) {
+               $props = $this->mFileProps;
+               $error = null;
+               Hooks::run( 'UploadStashFile', [ $this, $user, $props, &$error ] );
+               if ( $error ) {
+                       if ( !is_array( $error ) ) {
+                               $error = [ $error ];
+                       }
+                       return call_user_func_array( 'Status::newFatal', $error );
+               }
+               try {
+                       $file = $this->doStashFile( $user );
+                       return Status::newGood( $file );
+               } catch ( UploadStashException $e ) {
+                       return Status::newFatal( 'uploadstash-exception', get_class( $e ), $e->getMessage() );
+               }
+       }
+
        /**
         * If the user does not supply all necessary information in the first upload
         * form submission (either by accident or by design) then we may want to
@@ -956,6 +983,7 @@ abstract class UploadBase {
         * which can be passed through a form or API request to find this stashed
         * file again.
         *
+        * @deprecated since 1.28 Use tryStashFile() instead
         * @param User $user
         * @return UploadStashFile Stashed file
         * @throws UploadStashBadPathException
@@ -963,6 +991,16 @@ abstract class UploadBase {
         * @throws UploadStashNotLoggedInException
         */
        public function stashFile( User $user = null ) {
+               return $this->doStashFile( $user );
+       }
+
+       /**
+        * Implementation for stashFile() and tryStashFile().
+        *
+        * @param User $user
+        * @return UploadStashFile Stashed file
+        */
+       protected function doStashFile( User $user = null ) {
                $stash = RepoGroup::singleton()->getLocalRepo()->getUploadStash( $user );
                $file = $stash->stashFile( $this->mTempPath, $this->getSourceType() );
                $this->mLocalFile = $file;
@@ -979,7 +1017,7 @@ abstract class UploadBase {
         */
        public function stashFileGetKey() {
                wfDeprecated( __METHOD__, '1.28' );
-               return $this->stashFile()->getFileKey();
+               return $this->doStashFile()->getFileKey();
        }
 
        /**
@@ -990,7 +1028,7 @@ abstract class UploadBase {
         */
        public function stashSession() {
                wfDeprecated( __METHOD__, '1.28' );
-               return $this->stashFile()->getFileKey();
+               return $this->doStashFile()->getFileKey();
        }
 
        /**
index 0323b68..cc1d698 100644 (file)
@@ -65,19 +65,19 @@ class UploadFromChunks extends UploadFromFile {
        }
 
        /**
-        * Calls the parent stashFile and updates the uploadsession table to handle "chunks"
+        * Calls the parent doStashFile and updates the uploadsession table to handle "chunks"
         *
         * @param User|null $user
         * @return UploadStashFile Stashed file
         */
-       public function stashFile( User $user = null ) {
+       protected function doStashFile( User $user = null ) {
                // Stash file is the called on creating a new chunk session:
                $this->mChunkIndex = 0;
                $this->mOffset = 0;
 
                $this->verifyChunk();
                // Create a local stash target
-               $this->mLocalFile = parent::stashFile( $user );
+               $this->mLocalFile = parent::doStashFile( $user );
                // Update the initial file offset (based on file size)
                $this->mOffset = $this->mLocalFile->getSize();
                $this->mFileKey = $this->mLocalFile->getFileKey();
@@ -162,7 +162,7 @@ class UploadFromChunks extends UploadFromFile {
                // Update the mTempPath and mLocalFile
                // (for FileUpload or normal Stash to take over)
                $tStart = microtime( true );
-               $this->mLocalFile = parent::stashFile( $this->user );
+               $this->mLocalFile = parent::doStashFile( $this->user );
                $tAmount = microtime( true ) - $tStart;
                $this->mLocalFile->setLocalReference( $tmpFile ); // reuse (e.g. for getImageInfo())
                wfDebugLog( 'fileconcatenate', "Stashed combined file ($i chunks) in $tAmount seconds." );
index 908c8aa..50bcbc4 100644 (file)
@@ -153,10 +153,10 @@ class UploadFromStash extends UploadBase {
         * @param User $user
         * @return UploadStashFile
         */
-       public function stashFile( User $user = null ) {
+       protected function doStashFile( User $user = null ) {
                // replace mLocalFile with an instance of UploadStashFile, which adds some methods
                // that are useful for stashed files.
-               $this->mLocalFile = parent::stashFile( $user );
+               $this->mLocalFile = parent::doStashFile( $user );
 
                return $this->mLocalFile;
        }
index 839e081..b55cb76 100644 (file)
        "uploadstash-errclear": "Clearing the files failed.",
        "uploadstash-refresh": "Refresh the list of files",
        "uploadstash-thumbnail": "view thumbnail",
+       "uploadstash-exception": "Could not store upload in the stash ($1): \"$2\".",
        "invalid-chunk-offset": "Invalid chunk offset",
        "img-auth-accessdenied": "Access denied",
        "img-auth-nopathinfo": "Missing PATH_INFO.\nYour server is not set up to pass this information.\nIt may be CGI-based and cannot support img_auth.\nSee https://www.mediawiki.org/wiki/Special:MyLanguage/Manual:Image_Authorization.",
index 77d863d..953ebbb 100644 (file)
        "uploadstash-errclear": "Used as error message in [[Special:UploadStash]].",
        "uploadstash-refresh": "Used as link text in [[Special:UploadStash]].",
        "uploadstash-thumbnail": "Used as link text in [[Special:UploadStash]].",
+       "uploadstash-exception": "Error message shown when an action related to the upload stash fails unexpectedly.\n\nParameters:\n* $1 - exception name, e.g. 'UploadStashFileNotFoundException'\n* $2 - exceptions details (always in English), e.g. 'cannot find path, or not a plain file'",
        "invalid-chunk-offset": "Error that can happen if chunks get uploaded out of order.\nAs a result of this error, clients can continue from an offset provided or restart the upload.\nUsed on [[Special:UploadWizard]].",
        "img-auth-accessdenied": "[[mw:Manual:Image Authorization|Manual:Image Authorization]]: Access Denied\n{{Identical|Access denied}}",
        "img-auth-nopathinfo": "[[mw:Manual:Image Authorization|Manual:Image Authorization]]: Missing PATH_INFO - see english description\n{{Doc-important|This is plain text. Do not use any wiki syntax.}}",