Bartosz Dziewoński has uploaded a new change for review.
https://gerrit.wikimedia.org/r/244512
Change subject: mw.Upload: Refactor error handling for the umpteenth time
......................................................................
mw.Upload: Refactor error handling for the umpteenth time
When I started, I just wanted mw.Upload.BookletLayout to be able to
display more information about errors than just the default message
(like it can do for warnings). And down the rabbit hole I went...
mediawiki/api/upload.js:
* Simply throw errors when our methods are called with bad parameters,
rather than return a rejected promise.
* Always call .notify( 1 ) when upload is complete, regardless of
whether it succeeded or failed.
* Reject promises with error code and error details, for consistency
with api.js. Previous behavior meant that we did not let callers
know the details in some cases. It was also problematic when we
passed-through promises rejected in api.js (which had different
parameters given).
* Made some effort to return sane codes when something intricate
fails in iframe upload, but no guarantee that this works well. The
codes are inspired by what api.js returns in similar circumstances.
* When rejecting because of warnings, use the first warning's key as
error code.
* Always ignore the warnings when uploading to stash and 'filekey'
is present in response, never ignore when uploading directly.
* When the upload succeeds, never check for 'result.upload.error'
(which just isn't a thing) nor for 'result.error' (which api.js
detects and rejects the promise before we get to it). We only need
to check for 'result.upload.warnings'.
mediawiki.Upload.js:
* Update for the above changes in mediawiki/api/upload.js.
* More reliably distinguish warnings from errors in all cases, not
only when finishing a stash upload.
* Store machine-readable error codes, not mw.Message objects. This
lets callers do something sensible when we encounter an unknown
error (especially one that has no corresponding message).
* Store full result as state details for warnings, as well as errors.
mediawiki.Upload.BookletLayout.js:
* Update for the above changes in mediawiki.Upload.js.
* Give errors/warnings generated during upload to stash the same
loving treatment as errors/warnings during publishing.
* Extract the code to a new method getErrorMessageForStateDetails().
* Handle 'stashfailed' warning (which is really an error).
* Handle unknown errors, now that mw.Upload lets us do something
sensible with them. (See, this is the thing I set out to do.)
Bug: T114940
Change-Id: I4c0f619a4e483cca296c2fa2907ed1f81a99fdd6
---
M resources/Resources.php
M resources/src/mediawiki/api/upload.js
M resources/src/mediawiki/mediawiki.Upload.BookletLayout.js
M resources/src/mediawiki/mediawiki.Upload.js
4 files changed, 223 insertions(+), 172 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core
refs/changes/12/244512/1
diff --git a/resources/Resources.php b/resources/Resources.php
index c635f0e..4d1b5ef 100644
--- a/resources/Resources.php
+++ b/resources/Resources.php
@@ -1135,54 +1135,6 @@
'dom-level2-shim',
'mediawiki.api.upload',
),
- 'messages' => array(
- 'api-error-badaccess-groups',
- 'api-error-badtoken',
- 'api-error-copyuploaddisabled',
- 'api-error-duplicate',
- 'api-error-duplicate-archive',
- 'api-error-empty-file',
- 'api-error-emptypage',
- 'api-error-fetchfileerror',
- 'api-error-fileexists-forbidden',
- 'api-error-fileexists-shared-forbidden',
- 'api-error-file-too-large',
- 'api-error-filename-tooshort',
- 'api-error-filetype-banned',
- 'api-error-filetype-banned-type',
- 'api-error-filetype-missing',
- 'api-error-hookaborted',
- 'api-error-http',
- 'api-error-illegal-filename',
- 'api-error-internal-error',
- 'api-error-invalid-file-key',
- 'api-error-missingparam',
- 'api-error-missingresult',
- 'api-error-mustbeloggedin',
- 'api-error-mustbeposted',
- 'api-error-noimageinfo',
- 'api-error-nomodule',
- 'api-error-ok-but-empty',
- 'api-error-overwrite',
- 'api-error-stashfailed',
- 'api-error-publishfailed',
- 'api-error-stasherror',
- 'api-error-stashedfilenotfound',
- 'api-error-stashpathinvalid',
- 'api-error-stashfilestorage',
- 'api-error-stashzerolength',
- 'api-error-stashnotloggedin',
- 'api-error-stashwrongowner',
- 'api-error-stashnosuchfilekey',
- 'api-error-timeout',
- 'api-error-unclassified',
- 'api-error-unknown-code',
- 'api-error-unknown-error',
- 'api-error-unknown-warning',
- 'api-error-unknownerror',
- 'api-error-uploaddisabled',
- 'api-error-verification-error',
- ),
),
'mediawiki.ForeignUpload' => array(
'scripts' =>
'resources/src/mediawiki/mediawiki.ForeignUpload.js',
@@ -1231,6 +1183,52 @@
'upload-form-label-infoform-description',
'upload-form-label-usage-title',
'upload-form-label-usage-filename',
+ 'api-error-unknownerror',
+ 'api-error-unknown-warning',
+ 'api-error-badaccess-groups',
+ 'api-error-badtoken',
+ 'api-error-copyuploaddisabled',
+ 'api-error-duplicate',
+ 'api-error-duplicate-archive',
+ 'api-error-empty-file',
+ 'api-error-emptypage',
+ 'api-error-fetchfileerror',
+ 'api-error-fileexists-forbidden',
+ 'api-error-fileexists-shared-forbidden',
+ 'api-error-file-too-large',
+ 'api-error-filename-tooshort',
+ 'api-error-filetype-banned',
+ 'api-error-filetype-banned-type',
+ 'api-error-filetype-missing',
+ 'api-error-hookaborted',
+ 'api-error-http',
+ 'api-error-illegal-filename',
+ 'api-error-internal-error',
+ 'api-error-invalid-file-key',
+ 'api-error-missingparam',
+ 'api-error-missingresult',
+ 'api-error-mustbeloggedin',
+ 'api-error-mustbeposted',
+ 'api-error-noimageinfo',
+ 'api-error-nomodule',
+ 'api-error-ok-but-empty',
+ 'api-error-overwrite',
+ 'api-error-stashfailed',
+ 'api-error-publishfailed',
+ 'api-error-stasherror',
+ 'api-error-stashedfilenotfound',
+ 'api-error-stashpathinvalid',
+ 'api-error-stashfilestorage',
+ 'api-error-stashzerolength',
+ 'api-error-stashnotloggedin',
+ 'api-error-stashwrongowner',
+ 'api-error-stashnosuchfilekey',
+ 'api-error-timeout',
+ 'api-error-unclassified',
+ 'api-error-unknown-code',
+ 'api-error-unknown-error',
+ 'api-error-uploaddisabled',
+ 'api-error-verification-error',
'fileexists',
'filepageexists',
'filename-bad-prefix',
diff --git a/resources/src/mediawiki/api/upload.js
b/resources/src/mediawiki/api/upload.js
index 4d6b34c..614c001 100644
--- a/resources/src/mediawiki/api/upload.js
+++ b/resources/src/mediawiki/api/upload.js
@@ -28,6 +28,21 @@
/**
* @private
+ * Given a non-empty object, return one of its keys.
+ *
+ * @param {Object} obj
+ * @return {string}
+ */
+ function getFirstKey( obj ) {
+ for ( var key in obj ) {
+ if ( obj.hasOwnProperty( key ) ) {
+ return key;
+ }
+ }
+ }
+
+ /**
+ * @private
* Get new iframe object for an upload.
*
* @return {HTMLIframeElement}
@@ -116,13 +131,13 @@
}
if ( !file ) {
- return $.Deferred().reject( 'No file' );
+ throw new Error( 'No file' );
}
canUseFormData = formDataAvailable() && file instanceof
window.File;
if ( !isFileInput && !canUseFormData ) {
- return $.Deferred().reject( 'Unsupported
argument type passed to mw.Api.upload' );
+ throw new Error( 'Unsupported argument type
passed to mw.Api.upload' );
}
if ( canUseFormData ) {
@@ -182,17 +197,19 @@
$iframe.one( 'load', function () {
$iframe.one( 'load', function () {
var result = processIframeResult(
iframe );
+ deferred.notify( 1 );
if ( !result ) {
- deferred.reject( 'No response
from API on upload attempt.' );
- } else if ( result.error ||
result.warnings ) {
- if ( result.error &&
result.error.code === 'badtoken' ) {
+ deferred.reject(
'ok-but-empty', 'No response from API on upload attempt.' );
+ } else if ( result.error ) {
+ if ( result.error.code ===
'badtoken' ) {
api.badToken( 'edit' );
}
- deferred.reject( result.error
|| result.warnings );
+ deferred.reject(
result.error.code, result );
+ } else if ( result.upload &&
result.upload.warnings ) {
+ deferred.reject( getFirstKey(
result.upload.warnings ), result );
} else {
- deferred.notify( 1 );
deferred.resolve( result );
}
} );
@@ -202,7 +219,7 @@
} );
$iframe.error( function ( error ) {
- deferred.reject( 'iframe failed to load: ' +
error );
+ deferred.reject( 'http', error );
} );
$iframe.prop( 'src', 'about:blank' ).hide();
@@ -214,7 +231,7 @@
} );
if ( !data.filename && !data.stash ) {
- return $.Deferred().reject( 'Filename not
included in file data.' );
+ throw new Error( 'Filename not included in file
data.' );
}
if ( this.needToken() ) {
@@ -258,7 +275,7 @@
data.file = file;
if ( !data.filename && !data.stash ) {
- return $.Deferred().reject( 'Filename not
included in file data.' );
+ throw new Error( 'Filename not included in file
data.' );
}
// Use this.postWithEditToken() or this.post()
@@ -281,15 +298,16 @@
}
} )
.done( function ( result ) {
- if ( result.error || result.warnings ) {
- deferred.reject( result.error
|| result.warnings );
+ deferred.notify( 1 );
+ if ( result.upload &&
result.upload.warnings ) {
+ deferred.reject( getFirstKey(
result.upload.warnings ), result );
} else {
- deferred.notify( 1 );
deferred.resolve( result );
}
} )
- .fail( function ( result ) {
- deferred.reject( result );
+ .fail( function ( errorCode, result ) {
+ deferred.notify( 1 );
+ deferred.reject( errorCode, result );
} );
return deferred.promise();
@@ -324,7 +342,7 @@
api = this;
if ( !data.filename ) {
- return $.Deferred().reject( 'Filename not
included in file data.' );
+ throw new Error( 'Filename not included in file
data.' );
}
function finishUpload( moreData ) {
@@ -334,26 +352,31 @@
data.format = 'json';
if ( !data.filename ) {
- return $.Deferred().reject( 'Filename
not included in file data.' );
+ throw new Error( 'Filename not included
in file data.' );
}
return api.postWithEditToken( data ).then(
function ( result ) {
- if ( result.upload && (
result.upload.error || result.upload.warnings ) ) {
- return $.Deferred().reject(
result.upload.error || result.upload.warnings ).promise();
+ if ( result.upload &&
result.upload.warnings ) {
+ return $.Deferred().reject(
getFirstKey( result.upload.warnings ), result ).promise();
}
return result;
} );
}
- return this.upload( file, { stash: true, filename:
data.filename } ).then( function ( result ) {
- if ( result && result.upload &&
result.upload.filekey ) {
+ return this.upload( file, { stash: true, filename:
data.filename } ).then(
+ function ( result ) {
filekey = result.upload.filekey;
- } else if ( result && ( result.error ||
result.warning ) ) {
- return $.Deferred().reject( result );
+ return finishUpload;
+ },
+ function ( errorCode, result ) {
+ if ( result && result.upload &&
result.upload.filekey ) {
+ // Ignore any warnings if
'filekey' was returned, that's all we care about
+ filekey = result.upload.filekey;
+ return $.Deferred().resolve(
finishUpload );
+ }
+ return $.Deferred().reject( errorCode,
result );
}
-
- return finishUpload;
- } );
+ );
},
needToken: function () {
diff --git a/resources/src/mediawiki/mediawiki.Upload.BookletLayout.js
b/resources/src/mediawiki/mediawiki.Upload.BookletLayout.js
index c4b8c09..1a748da 100644
--- a/resources/src/mediawiki/mediawiki.Upload.BookletLayout.js
+++ b/resources/src/mediawiki/mediawiki.Upload.BookletLayout.js
@@ -191,19 +191,15 @@
deferred.resolve();
layout.emit( 'fileUploaded' );
}, function () {
- // These errors will be thrown while the user is on the
info page
- if ( layout.upload.getState() === mw.Upload.State.ERROR
) {
- deferred.reject( new OO.ui.Error(
layout.upload.getStateDetails(), {
- recoverable: false
- } ) );
- return false;
- }
- if ( layout.upload.getState() ===
mw.Upload.State.WARNING ) {
- deferred.reject( new OO.ui.Error(
layout.upload.getStateDetails(), {
- recoverable: false
- } ) );
- return false;
- }
+ var
+ state = layout.upload.getState(),
+ stateDetails = layout.upload.getStateDetails(),
+ errorMessage =
layout.getErrorMessageForStateDetails( state, stateDetails );
+
+ // These errors will be thrown while the user is on the
info page.
+ // Pretty sure it's impossible to get a warning other
than 'stashfailed' here, which should
+ // really be an error...
+ deferred.reject( errorMessage );
} );
// If there is an error in uploading, come back to the upload
page
@@ -244,80 +240,112 @@
deferred.resolve();
layout.emit( 'fileSaved' );
}, function () {
- var stateDetails =
layout.upload.getStateDetails();
+ var
+ state = layout.upload.getState(),
+ stateDetails =
layout.upload.getStateDetails(),
+ errorMessage =
layout.getErrorMessageForStateDetails( state, stateDetails );
- if ( layout.upload.getState() ===
mw.Upload.State.ERROR ) {
- deferred.reject( new OO.ui.Error(
stateDetails, {
- recoverable: false
- } ) );
- return false;
- }
-
- if ( layout.upload.getState() ===
mw.Upload.State.WARNING ) {
- // We could get more than one of these
errors, these are in order
- // of importance. For example fixing
the thumbnail like file name
- // won't help the fact that the file
already exists.
- if ( stateDetails.exists !== undefined
) {
- deferred.reject( new
OO.ui.Error(
- $( '<p>' ).html(
- mw.message(
'filepageexists', stateDetails.exists ).parse()
- ),
- { recoverable: false }
- ) );
- } else if ( stateDetails.duplicate !==
undefined ) {
- deferred.reject( new
OO.ui.Error(
- $( '<p>' ).html(
- mw.message(
'fileexists', stateDetails.duplicate[ 0 ] ).parse()
- ),
- { recoverable: false }
- ) );
- } else if ( stateDetails[ 'thumb-name'
] !== undefined ) {
- deferred.reject( new
OO.ui.Error(
- $( '<p>' ).html(
- mw.message(
'filename-thumb-name' ).parse()
- ),
- { recoverable: false }
- ) );
- } else if ( stateDetails[ 'bad-prefix'
] !== undefined ) {
- deferred.reject( new
OO.ui.Error(
- $( '<p>' ).html(
- mw.message(
'filename-bad-prefix', stateDetails[ 'bad-prefix' ] ).parse()
- ),
- { recoverable: false }
- ) );
- } else if ( stateDetails[
'duplicate-archive' ] !== undefined ) {
- deferred.reject( new
OO.ui.Error(
- $( '<p>' ).html(
- mw.message(
'api-error-duplicate-archive', stateDetails[ 'duplicate-archive' ] ).parse()
- ),
- { recoverable: false }
- ) );
- } else if ( stateDetails.badfilename
!== undefined ) {
- // Change the name if the
current name isn't acceptable
- layout.filenameWidget.setValue(
stateDetails.badfilename );
- deferred.reject( new
OO.ui.Error(
- $( '<p>' ).html(
- mw.message(
'badfilename', stateDetails.badfilename ).parse()
- )
- ) );
- } else {
- deferred.reject( new
OO.ui.Error(
- $( '<p>' ).html(
- // Let's get
all the help we can if we can't pin point the error
- mw.message(
'api-error-unknown-warning', JSON.stringify( stateDetails ) ).parse()
- ),
- { recoverable: false }
- ) );
- }
-
- return false;
- }
+ deferred.reject( errorMessage );
} );
} );
return deferred.promise();
};
+ /**
+ * Get an error message (as OO.ui.Error object) that should be
displayed to the user for the given
+ * state and state details.
+ *
+ * @protected
+ * @param {mw.Upload.State} state mw.Upload.State.ERROR or
mw.Upload.State.WARNING
+ * @param {Object} stateDetails
+ * @returns {OO.ui.Error} Error to display for given state and details.
+ */
+ mw.Upload.BookletLayout.prototype.getErrorMessageForStateDetails =
function ( state, stateDetails ) {
+ var message,
+ error = stateDetails.error,
+ warnings = stateDetails.upload &&
stateDetails.upload.warnings;
+
+ if ( state === mw.Upload.State.ERROR ) {
+ message = mw.message( 'api-error-' + error.code );
+ if ( !message.exists() ) {
+ message = mw.message( 'api-error-unknownerror',
JSON.stringify( stateDetails ) );
+ }
+ return new OO.ui.Error(
+ $( '<p>' ).html(
+ message.parse()
+ ),
+ { recoverable: false }
+ );
+ }
+
+ if ( state === mw.Upload.State.WARNING ) {
+ // We could get more than one of these errors, these
are in order
+ // of importance. For example fixing the thumbnail like
file name
+ // won't help the fact that the file already exists.
+ if ( warnings.stashfailed !== undefined ) {
+ return new OO.ui.Error(
+ $( '<p>' ).html(
+ mw.message(
'api-error-stashfailed' ).parse()
+ ),
+ { recoverable: false }
+ );
+ } else if ( warnings.exists !== undefined ) {
+ return new OO.ui.Error(
+ $( '<p>' ).html(
+ mw.message( 'filepageexists',
warnings.exists ).parse()
+ ),
+ { recoverable: false }
+ );
+ } else if ( warnings.duplicate !== undefined ) {
+ return new OO.ui.Error(
+ $( '<p>' ).html(
+ mw.message( 'fileexists',
warnings.duplicate[ 0 ] ).parse()
+ ),
+ { recoverable: false }
+ );
+ } else if ( warnings[ 'thumb-name' ] !== undefined ) {
+ return new OO.ui.Error(
+ $( '<p>' ).html(
+ mw.message(
'filename-thumb-name' ).parse()
+ ),
+ { recoverable: false }
+ );
+ } else if ( warnings[ 'bad-prefix' ] !== undefined ) {
+ return new OO.ui.Error(
+ $( '<p>' ).html(
+ mw.message(
'filename-bad-prefix', warnings[ 'bad-prefix' ] ).parse()
+ ),
+ { recoverable: false }
+ );
+ } else if ( warnings[ 'duplicate-archive' ] !==
undefined ) {
+ return new OO.ui.Error(
+ $( '<p>' ).html(
+ mw.message(
'api-error-duplicate-archive', warnings[ 'duplicate-archive' ] ).parse()
+ ),
+ { recoverable: false }
+ );
+ } else if ( warnings.badfilename !== undefined ) {
+ // Change the name if the current name isn't
acceptable
+ // TODO This might not really be the best place
to do this
+ this.filenameWidget.setValue(
warnings.badfilename );
+ return new OO.ui.Error(
+ $( '<p>' ).html(
+ mw.message( 'badfilename',
warnings.badfilename ).parse()
+ )
+ );
+ } else {
+ return new OO.ui.Error(
+ $( '<p>' ).html(
+ // Let's get all the help we
can if we can't pin point the error
+ mw.message(
'api-error-unknown-warning', JSON.stringify( stateDetails ) ).parse()
+ ),
+ { recoverable: false }
+ );
+ }
+ }
+ };
+
/* Form renderers */
/**
diff --git a/resources/src/mediawiki/mediawiki.Upload.js
b/resources/src/mediawiki/mediawiki.Upload.js
index 007c855..1432912 100644
--- a/resources/src/mediawiki/mediawiki.Upload.js
+++ b/resources/src/mediawiki/mediawiki.Upload.js
@@ -191,7 +191,7 @@
* Sets the state and state details (if any) of the upload.
*
* @param {mw.Upload.State} state
- * @param {string|Object} stateDetails
+ * @param {Object} stateDetails
*/
UP.setState = function ( state, stateDetails ) {
this.state = state;
@@ -254,8 +254,13 @@
upload.setState( Upload.State.UPLOADED );
upload.imageinfo = result.upload.imageinfo;
return result;
- }, function () {
- upload.setState( Upload.State.ERROR );
+ }, function ( errorCode, result ) {
+ if ( result && result.upload && result.upload.warnings
) {
+ upload.setState( Upload.State.WARNING, result );
+ } else {
+ upload.setState( Upload.State.ERROR, result );
+ }
+ return $.Deferred().reject( errorCode, result );
} );
};
@@ -282,8 +287,13 @@
} ).then( function ( finishStash ) {
upload.setState( Upload.State.STASHED );
return finishStash;
- }, function ( result ) {
- upload.setState( Upload.State.ERROR, mw.message(
'api-error-' + result ) );
+ }, function ( errorCode, result ) {
+ if ( result && result.upload && result.upload.warnings
) {
+ upload.setState( Upload.State.WARNING, result );
+ } else {
+ upload.setState( Upload.State.ERROR, result );
+ }
+ return $.Deferred().reject( errorCode, result );
} );
return this.stashPromise;
@@ -313,21 +323,13 @@
upload.setState( Upload.State.UPLOADED );
upload.imageinfo = result.upload.imageinfo;
return result;
- }, function ( result ) {
- // Errors are strings that can be used to get
error message
- if ( typeof result === 'string' ) {
- upload.setState( Upload.State.ERROR,
mw.message( 'api-error-' + result ) );
- return;
- }
-
- // Warnings come in the form of objects
- if ( $.isPlainObject( result ) ) {
+ }, function ( errorCode, result ) {
+ if ( result && result.upload &&
result.upload.warnings ) {
upload.setState( Upload.State.WARNING,
result );
- return;
+ } else {
+ upload.setState( Upload.State.ERROR,
result );
}
-
- // Throw an empty error if we can't figure it
out
- upload.setState( Upload.State.ERROR );
+ return $.Deferred().reject( errorCode, result );
} );
} );
};
--
To view, visit https://gerrit.wikimedia.org/r/244512
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4c0f619a4e483cca296c2fa2907ed1f81a99fdd6
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Bartosz Dziewoński <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
