Introduce UploadStashFile hook, improve API handling of stash errors
authorBartosz Dziewoński <matma.rex@gmail.com>
Wed, 3 Aug 2016 00:13:01 +0000 (02:13 +0200)
committerBartosz Dziewoński <matma.rex@gmail.com>
Tue, 9 Aug 2016 16:10:18 +0000 (18:10 +0200)
UploadBase:
* Introduce a new method, tryStashFile(), as a replacement for the
  now-soft-deprecated stashFile(). The method runs the new hook and
  returns a Status object, with an error (if the hook returned an
  error) or a value (if it didn't).
* Introduce a new hook, UploadStashFile, allowing extensions to
  prevent a file from being stashed. Note that code in extensions
  which has not been updated for MediaWiki 1.28 may still call
  stashFile() directly, and therefore not call this hook. For
  important checks (not just for UI), extension authors should use
  UploadVerifyFile or UploadVerifyUpload hooks.
* Extract common code of tryStashFile() and stashFile() to
  a new protected method doStashFile().

SpecialUpload:
* Use tryStashFile() when stashing a file after a warning or
  "recoverable error" was encountered.

ApiUpload:
* Refactor stashing code so that error handling only happens in one
  place, not four different ones. Use Status objects rather than
  exception throwing/catching for control flow.
* Simplify the error messages slightly (error codes are unchanged).
  Produce better ones by always using handleStashException().
  'stashfailed' is now always at root (not nested inside 'warnings'),
  behaving the same as 'filekey' does on success.
* Use tryStashFile() when stashing. Handle errors so as to allow
  custom API results passed via ApiMessage to be preserved.

Some API result changes for different requests are shown below.

  api.php?action=upload&format=json&filename=good.png&file=...&stash=1

    Before:
      {
        "error": {
          "code": "stashfilestorage",
          "info": "Could not store upload in the stash: Stashing temporary file failed: UploadStashFileException Error storing file in '/tmp/phpB32SRT': Could not create directory \"mwstore://local-backend/local-temp/3/3a\".",
          "*": "See http://localhost:3080/w/api.php for API usage"
        }
      }

    After:
      {
        "error": {
          "code": "stashfilestorage",
          "info": "Could not store upload in the stash: Error storing file in '/tmp/phpB32SRT': Could not create directory \"mwstore://local-backend/local-temp/3/3a\".",
          "*": "See http://localhost:3080/w/api.php for API usage"
        }
      }

  api.php?action=upload&format=json&filename=[bad].png&file=...

    Before:
      {
        "upload": {
          "result": "Warning",
          "warnings": {
            "badfilename": "-bad-.png",
            "stashfailed": "Stashing temporary file failed: UploadStashFileException Error storing file in '/tmp/phpB32SRT': Could not create directory \"mwstore://local-backend/local-temp/3/3a\"."
          }
        }
      }

    After:
      {
        "upload": {
          "result": "Warning",
          "stashfailed": "Could not store upload in the stash: Error storing file in '/tmp/phpB32SRT': Could not create directory \"mwstore://local-backend/local-temp/3/3a\"."
          "warnings": {
            "badfilename": "-bad-.png",
          }
        }
      }

Bug: T140521
Change-Id: I2f574b355cd33b2e9fa7ff8e1793503b257cce65

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 cddc30e..82fdee7 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 054ab42..1cca279 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.}}",