r86846 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r86845‎ | r86846 | r86847 >
Date:11:39, 25 April 2011
Author:catrope
Status:ok (Comments)
Tags:
Comment:
(bug 16921) Add JavaScript-based enforcing of byte limits on move and protection forms. Patch by Jan Paul Posma
Modified paths:
  • /trunk/phase3/includes/ProtectionForm.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialMovepage.php (modified) (history)
  • /trunk/phase3/resources/Resources.php (modified) (history)
  • /trunk/phase3/resources/mediawiki.special/mediawiki.special.movePage.js (added) (history)
  • /trunk/phase3/skins/common/protect.js (modified) (history)

Diff [purge]

Index: trunk/phase3/skins/common/protect.js
@@ -45,6 +45,8 @@
4646 check.checked = !this.areAllTypesMatching();
4747 this.enableUnchainedInputs( check.checked );
4848 }
 49+
 50+ $( '#mwProtect-reason' ).byteLimit( 180 );
4951
5052 this.updateCascadeCheckbox();
5153
Index: trunk/phase3/includes/ProtectionForm.php
@@ -467,7 +467,10 @@
468468 </td>
469469 <td class='mw-input'>" .
470470 Xml::input( 'mwProtect-reason', 60, $this->mReason, array( 'type' => 'text',
471 - 'id' => 'mwProtect-reason', 'maxlength' => 255 ) ) .
 471+ 'id' => 'mwProtect-reason', 'maxlength' => 180 ) ) .
 472+ // Limited maxlength as the database trims at 255 bytes and other texts
 473+ // chosen by dropdown menus on this page are also included in this database field.
 474+ // The byte limit of 180 bytes is enforced in javascript
472475 "</td>
473476 </tr>";
474477 # Disallow watching is user is not logged in
Index: trunk/phase3/includes/specials/SpecialMovepage.php
@@ -113,6 +113,8 @@
114114
115115 $wgOut->setPagetitle( wfMsg( 'move-page', $this->oldTitle->getPrefixedText() ) );
116116 $skin->setRelevantTitle( $this->oldTitle );
 117+
 118+ $wgOut->addModules( 'mediawiki.special.movePage' );
117119
118120 $newTitle = $this->newTitle;
119121
@@ -237,7 +239,7 @@
238240 "</td>
239241 <td class='mw-input'>" .
240242 Html::element( 'textarea', array( 'name' => 'wpReason', 'id' => 'wpReason', 'cols' => 60, 'rows' => 2,
241 - 'maxlength' => 200 ), $this->reason ) .
 243+ 'maxlength' => 200 ), $this->reason ) . // maxlength byte limit is enforce in mediawiki.special.movePage.js
242244 "</td>
243245 </tr>"
244246 );
Index: trunk/phase3/resources/Resources.php
@@ -455,6 +455,10 @@
456456 'mediawiki.special.block' => array(
457457 'scripts' => 'resources/mediawiki.special/mediawiki.special.block.js',
458458 ),
 459+ 'mediawiki.special.movePage' => array(
 460+ 'scripts' => 'resources/mediawiki.special/mediawiki.special.movePage.js',
 461+ 'dependencies' => 'jquery.byteLimit',
 462+ ),
459463 'mediawiki.special.upload' => array(
460464 // @TODO: merge in remainder of mediawiki.legacy.upload
461465 'scripts' => 'resources/mediawiki.special/mediawiki.special.upload.js',
@@ -586,7 +590,10 @@
587591 'scripts' => 'common/protect.js',
588592 'remoteBasePath' => $GLOBALS['wgStylePath'],
589593 'localBasePath' => "{$GLOBALS['IP']}/skins",
590 - 'dependencies' => 'mediawiki.legacy.wikibits',
 594+ 'dependencies' => array(
 595+ 'mediawiki.legacy.wikibits',
 596+ 'jquery.byteLimit',
 597+ ),
591598 ),
592599 'mediawiki.legacy.search' => array(
593600 'scripts' => 'common/search.js',
Index: trunk/phase3/resources/mediawiki.special/mediawiki.special.movePage.js
@@ -0,0 +1,5 @@
 2+/* JavaScript for Special:MovePage */
 3+
 4+jQuery( function( $ ) {
 5+ $( '#wpReason' ).byteLimit( 200 );
 6+});
Property changes on: trunk/phase3/resources/mediawiki.special/mediawiki.special.movePage.js
___________________________________________________________________
Added: svn:eol-style
17 + native

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r45517* (bug 16921) Add a maxlength for the reason field of the page move form...raymond20:34, 7 January 2009
r45571Reverted r45517 " * (bug 16921) Add a maxlength for the reason field of the p...brion18:51, 8 January 2009

Comments

#Comment by Brion VIBBER (talk | contribs)   17:13, 25 April 2011

The 180-byte limit seems to be arbitrary; is there a fixed length on the 'other' stuff that goes into the field, or was that just chosen to sound kinda nice?

I also recommend against duplicating magic numbers in multiple places like this (once on the maxlength attribute, and again in JS code), as they're likely to get out of sync during future maintenance. It might be most consistent to do something like this:

  • define a class we can apply to length-limited fields that we know we really have a byte limit for
  • have common JS apply the byte length limitation on all fields that are so marked, using the maxlength value that's already been specified (and is equal to byte length for the pure-ASCII case, thus always being the upper limit)

Status & tagging log