jenkins-bot has submitted this change and it was merged.
Change subject: UploadWizard: Don't throw JS errors; instead report them in UI
......................................................................
UploadWizard: Don't throw JS errors; instead report them in UI
Currently, Upload Wizard throws JavaScript errors towards the user's
JS-console when entering characters that are not valid in titles,
including square brackets, for instance.
After applying this patch, UploadWizard will properly report these
errors by leveraging the jQuery.validate plugin.
Further clean up:
- When constructing jQuery objects, either used valid HTML or a
single tag as the string argument.
- Combined multiple `var` statements.
- Avoid executing RegExps unnecessarily by passing the title as
arguments to subroutines.
- Using `.getMainText()` instead of `.toString()` to avoid unnecessary
API normalization.
- Make phpcs happy by breaking lines longer than 100 chars.
- Resolve a bug introduced while fixing Bug 32469: Do not clear errors
of validator that runs immediately from delayed method.
Bug: 64908
Change-Id: Ib54bf84497e37f8fba614e5c4dfa11550029b87d
---
M UploadWizardHooks.php
M i18n/en.json
M i18n/qqq.json
M resources/mw.DestinationChecker.js
M resources/mw.UploadWizardDetails.js
5 files changed, 90 insertions(+), 35 deletions(-)
Approvals:
Gilles: Looks good to me, approved
jenkins-bot: Verified
diff --git a/UploadWizardHooks.php b/UploadWizardHooks.php
index 764de17..394b26d 100644
--- a/UploadWizardHooks.php
+++ b/UploadWizardHooks.php
@@ -401,6 +401,7 @@
'mwe-upwiz-category-remove',
'mwe-upwiz-thumbnail-failed',
'mwe-upwiz-unparseable-filename',
+ 'mwe-upwiz-unparseable-title',
'mwe-upwiz-image-preview',
'mwe-upwiz-subhead-bugs',
'mwe-upwiz-subhead-alt-upload',
@@ -647,7 +648,10 @@
*/
public static function onIsUploadAllowedFromUrl( $url, &$allowed ) {
if ( $allowed ) {
- $flickrBlacklist = new UploadWizardFlickrBlacklist(
UploadWizardConfig::getConfig(), RequestContext::getMain() );
+ $flickrBlacklist = new UploadWizardFlickrBlacklist(
+ UploadWizardConfig::getConfig(),
+ RequestContext::getMain()
+ );
if ( $flickrBlacklist->isBlacklisted( $url ) ) {
$allowed = false;
}
@@ -661,7 +665,10 @@
* @param ResourceLoader resourceLoader
* @return bool
*/
- public static function onResourceLoaderTestModules( array
&$testModules, ResourceLoader &$resourceLoader ) {
+ public static function onResourceLoaderTestModules(
+ array &$testModules,
+ ResourceLoader &$resourceLoader
+ ) {
$testModules['qunit']['ext.uploadWizard.unit.tests'] = array(
'scripts' => array(
'tests/qunit/mw.UploadWizardLicenseInput.test.js',
diff --git a/i18n/en.json b/i18n/en.json
index 2c320a2..0cb0c8d 100644
--- a/i18n/en.json
+++ b/i18n/en.json
@@ -293,6 +293,7 @@
"mwe-upwiz-category-remove": "Remove this category",
"mwe-upwiz-thumbnail-failed": "The upload succeeded, but the server could
not get a preview thumbnail.",
"mwe-upwiz-unparseable-filename": "Could not understand the file name
\"$1\".",
+ "mwe-upwiz-unparseable-title": "This title is invalid. Make sure to remove
characters like square brackets, colons, comparison operators, pipes and curly
brackets.",
"mwe-upwiz-image-preview": "File preview",
"mwe-upwiz-subhead-bugs": "[$1 Known issues]",
"mwe-upwiz-subhead-alt-upload": "[$1 Back to the old form]",
diff --git a/i18n/qqq.json b/i18n/qqq.json
index 26f6ae7..0a51d3e 100644
--- a/i18n/qqq.json
+++ b/i18n/qqq.json
@@ -253,6 +253,7 @@
"mwe-upwiz-category-will-be-added": "Used to let the user know that the
category they entered will be created upon form submission.",
"mwe-upwiz-category-remove": "Used as action link
text.\n{{Identical|Remove category}}",
"mwe-upwiz-unparseable-filename": "Used as error message.
Parameters:\n* $1 - filename",
+ "mwe-upwiz-unparseable-title": "Used as error message. More information
about page titles [[:mw:Manual:Page title|at MediaWiki.org]].",
"mwe-upwiz-image-preview": "Used as title for the preview of the image
file.",
"mwe-upwiz-subhead-bugs": "Unused at this time. Parameters:\n* $1 -
full URL\n{{Identical|Known issue}}",
"mwe-upwiz-subhead-alt-upload": "Used as a link in the sub-header.
Parameters:\n* $1 - full URL",
diff --git a/resources/mw.DestinationChecker.js
b/resources/mw.DestinationChecker.js
index cb9312f..b078305 100644
--- a/resources/mw.DestinationChecker.js
+++ b/resources/mw.DestinationChecker.js
@@ -122,13 +122,13 @@
checker.spinner( status.unique === null ||
status.blacklist === null );
}
- this.checkUnique( checkerStatus );
- this.checkBlacklist( checkerStatus );
+ this.checkUnique( checkerStatus, title );
+ this.checkBlacklist( checkerStatus, title );
},
/**
* Get the current value of the input, with optional preprocessing
- * @return the current input value, with optional processing
+ * @return {string} the current input value, with optional processing
*/
getTitle: function() {
return this.preprocess( $( this.selector ).val() );
@@ -137,8 +137,11 @@
/**
* Async check if a title is in the titleblacklist.
* @param {Function} takes object, like { 'blacklist': result }
+ * @param {string} title the blacklist should be checked against
*/
- checkBlacklist: function ( callback ) {
+ checkBlacklist: function ( callback, title ) {
+ var checker = this;
+
function blacklistResultProcessor( blacklistResult ) {
var result;
@@ -157,8 +160,6 @@
callback( { 'blacklist': result } );
}
- var checker = this,
- title = this.getTitle();
if ( title === '' ) {
return;
@@ -185,8 +186,12 @@
* Async check if a filename is unique. Can be attached to a field's
change() event
* This is a more abstract version of
AddMedia/UploadHandler.js::doDestCheck
* @param {Function} takes object, like { 'unique': result }
+ * @param {string} title the uniqueness should be checked for
*/
- checkUnique: function( callback ) {
+ checkUnique: function( callback, title ) {
+ var params,
+ checker = this;
+
function ok( data ) {
var result, protection, pageId, ntitle, img;
@@ -267,20 +272,18 @@
mw.log( 'mw.DestinationChecker::checkUnique> Error in
checkUnique result: ' + code );
}
- var checker = this,
- title = this.getTitle(),
- // Setup the request -- will return thumbnail data if
it finds one
- // XXX do not use iiurlwidth as it will create a
thumbnail
- params = {
- 'titles': title,
- 'prop': 'info|imageinfo',
- 'inprop': 'protection',
- 'iiprop': 'url|mime|size',
- 'iiurlwidth': 150
- };
+ // Setup the request -- will return thumbnail data if it finds
one
+ // XXX do not use iiurlwidth as it will create a thumbnail
+ params = {
+ 'titles': title,
+ 'prop': 'info|imageinfo',
+ 'inprop': 'protection',
+ 'iiprop': 'url|mime|size',
+ 'iiurlwidth': 150
+ };
- // if input is empty don't bother.
+ // if input is empty or invalid, don't bother.
if ( title === '' ) {
return;
}
diff --git a/resources/mw.UploadWizardDetails.js
b/resources/mw.UploadWizardDetails.js
index 1089829..b6e0da8 100644
--- a/resources/mw.UploadWizardDetails.js
+++ b/resources/mw.UploadWizardDetails.js
@@ -40,14 +40,14 @@
// descriptions
this.descriptionsDiv = $( '<div
class="mwe-upwiz-details-descriptions"></div>' );
- this.descriptionAdder = $( '<a class="mwe-upwiz-more-options"/>' )
+ this.descriptionAdder = $( '<a class="mwe-upwiz-more-options"></a>' )
.text( mw.message(
'mwe-upwiz-desc-add-0' ).text() )
.click( function( ) {
details.addDescription(); } );
descriptionAdderDiv =
- $( '<div />' ).append(
- $( '<div class="mwe-upwiz-details-fieldname" />' ),
- $( '<div class="mwe-upwiz-details-descriptions-add" />'
)
+ $( '<div>' ).append(
+ $( '<div class="mwe-upwiz-details-fieldname"></div>' ),
+ $( '<div
class="mwe-upwiz-details-descriptions-add"></div>' )
.append( this.descriptionAdder )
);
@@ -64,13 +64,23 @@
api: this.upload.api,
spinner: function(bool) {
details.toggleDestinationBusy(bool); },
preprocess: function( name ) {
+ var cleanTitle;
+
// First, clear out any existing errors, to
prevent strange visual effects.
- // Fixes bug 32469.
- details.$form.find( 'label[for=' +
details.titleId + ']' ).empty();
+ // Fixes bug 32469. But introduced a new bug:
Some validator methods run immediately
+ // and this cleared out any error set by the
validator if no titleblacklist
+ // is installed (where validation is done
entirely remotely) because that second type
+ // of validation had a delay.
+ // Now only clearing errors from the delayed
methods.
+ // TLDR; FIXME: `clearTitleErrors` should not
be in a function called "preprocess"
+ // It's simply counter-intuitive.
+ details.clearTitleErrors();
+
if ( name !== '' ) {
- // turn the contents of the input into
a MediaWiki title ("File:foo_bar.jpg") to look up
+ // turn the contents of the input into
a MediaWiki title ("File:foo bar.jpg") to look up
// side effect -- also sets this as our
current title
- return details.setCleanTitle( name
).toString();
+ cleanTitle = details.setCleanTitle(
name );
+ return cleanTitle &&
cleanTitle.getMainText() || '';
} else {
return name;
}
@@ -398,8 +408,10 @@
this.$form.find( '.mwe-title' )
.rules( 'add', {
required: true,
+ titleParsability: true,
messages: {
- required: mw.message(
'mwe-upwiz-error-blank' ).escaped()
+ required: mw.message(
'mwe-upwiz-error-blank' ).escaped(),
+ titleParsability: mw.message(
'mwe-upwiz-unparseable-title' ).escaped()
}
} );
} else {
@@ -411,15 +423,18 @@
titleSenselessimagename: true,
titleThumbnail: true,
titleExtension: true,
+ titleParsability: true,
messages: {
required: mw.message(
'mwe-upwiz-error-blank' ).escaped(),
titleBadchars: mw.message(
'mwe-upwiz-error-title-badchars' ).escaped(),
titleSenselessimagename: mw.message(
'mwe-upwiz-error-title-senselessimagename' ).escaped(),
titleThumbnail: mw.message(
'mwe-upwiz-error-title-thumbnail' ).escaped(),
- titleExtension: mw.message(
'mwe-upwiz-error-title-extension' ).escaped()
+ titleExtension: mw.message(
'mwe-upwiz-error-title-extension' ).escaped(),
+ titleParsability: mw.message(
'mwe-upwiz-unparseable-title' ).escaped()
}
} );
}
+
// make this a category picker
hiddenCats = mw.UploadWizard.config.autoAdd.categories === undefined ?
[] : mw.UploadWizard.config.autoAdd.categories;
@@ -485,6 +500,31 @@
categories: true,
location: false,
other: true
+ },
+
+ /*
+ * Get a reference to the error labels
+ *
+ * @return {jQuery} reference to error labels
+ */
+ $getTitleErrorLabels: function() {
+ if ( !this.$titleErrorLabels || this.$titleErrorLabels.length
=== 0 ) {
+ this.$titleErrorLabels = this.$form.find( 'label[for='
+ this.titleId + ']' ).not( '.mwe-validator-error' );
+ }
+ return this.$titleErrorLabels;
+ },
+
+ /*
+ * Clears errors shown in UI
+ *
+ * @chainable
+ */
+ clearTitleErrors: function() {
+ var $labels = this.$getTitleErrorLabels();
+
+ $labels.empty();
+
+ return this;
},
/*
@@ -1504,18 +1544,21 @@
/**
* Apply some special cleanups for titles before adding to model. These
cleanups are not reflected in what the user sees in the title input field.
* For example, we remove an extension in the title if it matches the
extension we're going to add anyway. (bug #30676)
- * @param {String} title in human-readable form, e.g. "Foo bar", rather
than "File:Foo_bar.jpg"
- * @return {String} cleaned title with prefix and extension,
stringified.
+ * @param {string} title in human-readable form, e.g. "Foo bar", rather
than "File:Foo_bar.jpg"
+ * @return {mw.Title} cleaned title with prefix and extension,
stringified.
*/
setCleanTitle: function( s ) {
var ext = this.upload.title.getExtension(),
re = new RegExp( '\\.' +
this.upload.title.getExtension() + '$', 'i' ),
cleaned = $.trim( s.replace( re, '' ) );
- this.upload.title = new mw.Title( cleaned + '.' + ext, fileNsId
);
+ this.upload.title = mw.Title.newFromText( cleaned + '.' + ext,
fileNsId ) || this.upload.title;
return this.upload.title;
}
-
};
+$.validator.addMethod( 'titleParsability', function( s, elem ) {
+ return this.optional( elem ) || mw.Title.newFromText( $.trim( s ) );
+} );
+
}) ( mediaWiki, jQuery );
--
To view, visit https://gerrit.wikimedia.org/r/131595
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib54bf84497e37f8fba614e5c4dfa11550029b87d
Gerrit-PatchSet: 10
Gerrit-Project: mediawiki/extensions/UploadWizard
Gerrit-Branch: master
Gerrit-Owner: Rillke <[email protected]>
Gerrit-Reviewer: Gergő Tisza <[email protected]>
Gerrit-Reviewer: Gilles <[email protected]>
Gerrit-Reviewer: Krinkle <[email protected]>
Gerrit-Reviewer: MarkTraceur <[email protected]>
Gerrit-Reviewer: Rillke <[email protected]>
Gerrit-Reviewer: Siebrand <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
