r83280 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r83279‎ \| r83280 \| r83281 >
Date:	12:48, 5 March 2011
Author:	happy-melon
Status:	ok (Comments)
Tags:
Comment:	Follow-up r83183, r83202: * Update SpecialCheckUser.php to new location of IP functions * Spin out the 'hide-other-field-if-select-box-not-on-other' function as one which should apply to all such fields, especially those created via HTMLForm. SpecialBlockip and SpecialGlobalBlock should ultimately be converted to use HTMLForm anyway.
Modified paths:	/trunk/extensions/CheckUser/CheckUser.php (modified) (history) /trunk/extensions/CheckUser/checkuser.js (modified) (history) /trunk/extensions/GlobalBlocking/SpecialGlobalBlock.php (modified) (history) /trunk/phase3/includes/HTMLForm.php (modified) (history) /trunk/phase3/includes/specials/SpecialBlockip.php (modified) (history) /trunk/phase3/resources/Resources.php (modified) (history) /trunk/phase3/resources/mediawiki.special/mediawiki.special.block.js (modified) (history) /trunk/phase3/resources/mediawiki/mediawiki.htmlform.js (added) (history)

Diff [purge]

Index: trunk/phase3/includes/HTMLForm.php
—	—	@@ -184,7 +184,7 @@
185	185	if ( !$class ) {
186	186	throw new MWException( "Descriptor with no class: " . print_r( $descriptor, true ) );
187	187	}
188		-
	188	+
189	189	$descriptor['fieldname'] = $fieldname;
190	190
191	191	$obj = new $class( $descriptor );
—	—	@@ -210,7 +210,7 @@
211	211
212	212	/**
213	213	* Try submitting, with edit token check first
214		~~- * @return Status\|boolean~~
	214	+ * @return Status\|boolean
215	215	*/
216	216	function tryAuthorizedSubmit() {
217	217	global $wgUser, $wgRequest;
—	—	@@ -368,6 +368,7 @@
369	369
370	370	# For good measure (it is the default)
371	371	$wgOut->preventClickjacking();
	372	+ $wgOut->addModules( 'mediawiki.htmlform' );
372	373
373	374	$html = ''
374	375	. $this->getErrors( $submitResult )
—	—	@@ -424,7 +425,6 @@
425	426	global $wgUser;
426	427
427	428	$html = '';
428		-
429	429	if( $this->getMethod() == 'post' ){
430	430	$html .= Html::hidden( 'wpEditToken', $wgUser->editToken(), array( 'id' => 'wpEditToken' ) ) . "\n";
431	431	$html .= Html::hidden( 'title', $this->getTitle()->getPrefixedText() ) . "\n";
—	—	@@ -623,7 +623,7 @@
624	624	function getTitle() {
625	625	return $this->mTitle;
626	626	}
627		-
	627	+
628	628	/**
629	629	* Set the method used to submit the form
630	630	* @param $method String
—	—	@@ -631,7 +631,7 @@
632	632	public function setMethod( $method='post' ){
633	633	$this->mMethod = $method;
634	634	}
635		-
	635	+
636	636	public function getMethod(){
637	637	return $this->mMethod;
638	638	}
—	—	@@ -840,12 +840,12 @@
841	841	if ( isset( $params['name'] ) ) {
842	842	$this->mName = $params['name'];
843	843	}
844		-
	844	+
845	845	$validName = Sanitizer::escapeId( $this->mName );
846	846	if ( $this->mName != $validName && !isset( $params['nodata'] ) ) {
847	847	throw new MWException( "Invalid name '{$this->mName}' passed to " . __METHOD__ );
848	848	}
849		-
	849	+
850	850	$this->mID = "mw-input-{$this->mName}";
851	851
852	852	if ( isset( $params['default'] ) ) {
—	—	@@ -887,10 +887,10 @@
888	888	global $wgRequest;
889	889
890	890	$errors = $this->validate( $value, $this->mParent->mFieldData );
891		-
	891	+
892	892	$cellAttributes = array();
893	893	$verticalLabel = false;
894		-
	894	+
895	895	if ( !empty($this->mParams['vertical-label']) ) {
896	896	$cellAttributes['colspan'] = 2;
897	897	$verticalLabel = true;
—	—	@@ -908,9 +908,9 @@
909	909	array( 'class' => 'mw-input' ) + $cellAttributes,
910	910	$this->getInputHTML( $value ) . "\n$errors"
911	911	);
912		-
	912	+
913	913	$fieldType = get_class( $this );
914		-
	914	+
915	915	if ($verticalLabel) {
916	916	$html = Html::rawElement( 'tr',
917	917	array( 'class' => 'mw-htmlform-vertical-label' ), $label );
—	—	@@ -1139,11 +1139,11 @@
1140	1140	if ( $p !== true ) {
1141	1141	return $p;
1142	1142	}
1143		-
	1143	+
1144	1144	$value = trim( $value );
1145	1145
1146	1146	# http://dev.w3.org/html5/spec/common-microsyntaxes.html#real-numbers
1147		~~- # with the addition that a leading '+' sign is ok.~~
	1147	+ # with the addition that a leading '+' sign is ok.
1148	1148	if ( !preg_match( '/^((\+\|\-)?\d+(\.\d+)?(E(\+\|\-)?\d+)?)?$/i', $value ) ) {
1149	1149	return wfMsgExt( 'htmlform-float-invalid', 'parse' );
1150	1150	}
—	—	@@ -1182,8 +1182,8 @@
1183	1183	}
1184	1184
1185	1185	# http://dev.w3.org/html5/spec/common-microsyntaxes.html#signed-integers
1186		~~- # with the addition that a leading '+' sign is ok. Note that leading zeros~~
1187		~~- # are fine, and will be left in the input, which is useful for things like~~
	1186	+ # with the addition that a leading '+' sign is ok. Note that leading zeros
	1187	+ # are fine, and will be left in the input, which is useful for things like
1188	1188	# phone numbers when you know that they are integers (the HTML5 type=tel
1189	1189	# input does not require its value to be numeric). If you want a tidier
1190	1190	# value to, eg, save in the DB, clean it up with intval().
—	—	@@ -1415,8 +1415,8 @@
1416	1416	} else {
1417	1417	$thisAttribs = array( 'id' => "{$this->mID}-$info", 'value' => $info );
1418	1418
1419		~~- $checkbox = Xml::check(~~
1420		~~- $this->mName . '[]',~~
	1419	+ $checkbox = Xml::check(
	1420	+ $this->mName . '[]',
1421	1421	in_array( $info, $value, true ),
1422	1422	$attribs + $thisAttribs );
1423	1423	$checkbox .= ' ' . Html::rawElement( 'label', array( 'for' => "{$this->mID}-$info" ), $label );
—	—	@@ -1556,7 +1556,7 @@
1557	1557	class HTMLHiddenField extends HTMLFormField {
1558	1558	public function __construct( $params ) {
1559	1559	parent::__construct( $params );
1560		-
	1560	+
1561	1561	# Per HTML5 spec, hidden fields cannot be 'required'
1562	1562	# http://dev.w3.org/html5/spec/states-of-the-type-attribute.html#hidden-state
1563	1563	unset( $this->mParams['required'] );
—	—	@@ -1605,7 +1605,7 @@
1606	1606	protected function needsLabel() {
1607	1607	return false;
1608	1608	}
1609		-
	1609	+
1610	1610	/**
1611	1611	* Button cannot be invalid
1612	1612	*/
Index: trunk/phase3/includes/specials/SpecialBlockip.php
—	—	@@ -199,7 +199,9 @@
200	200	wfMsgForContent( 'ipbreason-dropdown' ),
201	201	wfMsgForContent( 'ipbreasonotherlist' ), $this->BlockReasonList, 'wpBlockDropDown', 4 );
202	202
203		~~- $wgOut->addModules( 'mediawiki.special.block' );~~
	203	+ # FIXME: this should actually use HTMLForm, not just some of its JavaScript
	204	+ $wgOut->addModules( array( 'mediawiki.special.block', 'mediawiki.htmlform' ) );
	205	+
204	206	$wgOut->addHTML(
205	207	Xml::openElement( 'form', array( 'method' => 'post', 'action' => $titleObj->getLocalURL( 'action=submit' ), 'id' => 'blockip' ) ) .
206	208	Xml::openElement( 'fieldset' ) .
—	—	@@ -218,33 +220,35 @@
219	221	) + ( $this->BlockAddress ? array() : array( 'autofocus' ) ) ). "
220	222	</td>
221	223	</tr>
222		~~- <tr>"~~
223		~~- );~~
224		~~- if( $showblockoptions ) {~~
225		~~- $wgOut->addHTML("~~
	224	+ <tr>
226	225	<td class='mw-label'>
227	226	{$mIpbexpiry}
228	227	</td>
229		~~- <td class='mw-input'>" .~~
	228	+ <td class='mw-input'>"
	229	+ );
	230	+ if( $showblockoptions ) {
	231	+ $wgOut->addHTML(
230	232	Xml::tags( 'select',
231	233	array(
232	234	'id' => 'wpBlockExpiry',
233	235	'name' => 'wpBlockExpiry',
	236	+ 'class' => 'mw-htmlform-select-or-other', # FIXME: actually make this use HTMLForm
234	237	'tabindex' => '2' ),
235		~~- $blockExpiryFormOptions ) .~~
236		~~- "</td>"~~
	238	+ $blockExpiryFormOptions
	239	+ ) . "<br/>\n"
237	240	);
238	241	}
239		~~- $wgOut->addHTML("~~
240		~~- </tr>~~
241		~~- <tr id='wpBlockOther'>~~
242		~~- <td class='mw-label'>~~
243		~~- {$mIpbother}~~
	242	+ $wgOut->addHTML(
	243	+ Xml::input(
	244	+ 'wpBlockOther',
	245	+ 45,
	246	+ $this->BlockOther,
	247	+ array(
	248	+ 'tabindex' => '3',
	249	+ 'id' => 'wpBlockExpiry-other'
	250	+ )
	251	+ ) . "
244	252	</td>
245		~~- <td class='mw-input'>" .~~
246		~~- Xml::input( 'wpBlockOther', 45, $this->BlockOther,~~
247		~~- array( 'tabindex' => '3', 'id' => 'mw-bi-other' ) ) . "~~
248		~~- </td>~~
249	253	</tr>
250	254	<tr>
251	255	<td class='mw-label'>
Index: trunk/phase3/resources/Resources.php
—	—	@@ -382,6 +382,9 @@
383	383	'debugScripts' => 'resources/mediawiki/mediawiki.log.js',
384	384	'debugRaw' => false,
385	385	),
	386	+ 'mediawiki.htmlform' => array(
	387	+ 'scripts' => 'resources/mediawiki/mediawiki.htmlform.js',
	388	+ ),
386	389	'mediawiki.util' => array(
387	390	'scripts' => 'resources/mediawiki.util/mediawiki.util.js',
388	391	'dependencies' => array(
Index: trunk/phase3/resources/mediawiki.special/mediawiki.special.block.js
—	—	@@ -1,34 +1,12 @@
2	2	/* JavaScript for Special:Block */
3	3
4		~~-// Fade or snap depending on argument~~
5		~~-jQuery.fn.goIn = function( instantToggle ) {~~
6		~~- if ( typeof instantToggle != 'undefined' && instantToggle === true ) {~~
7		~~- return jQuery(this).show();~~
8		~~- }~~
9		~~- return jQuery(this).stop( true, true ).fadeIn();~~
10		~~-};~~
11		~~-jQuery.fn.goOut = function( instantToggle ) {~~
12		~~- if ( typeof instantToggle != 'undefined' && instantToggle === true ) {~~
13		~~- return jQuery(this).hide();~~
14		~~- }~~
15		~~- return jQuery(this).stop( true, true ).fadeOut();~~
16		~~-};~~
17		-
18	4	jQuery( function( $ ) {
19	5
20	6	var DO_INSTANT = true,
21		~~- $blockExpiry = $( '#wpBlockExpiry' ), $blockOther = $( '#wpBlockOther' ),~~
22	7	$blockTarget = $( '#mw-bi-target' ), $anonOnlyRow = $( '#wpAnonOnlyRow' ),
23	8	$enableAutoblockRow = $( '#wpEnableAutoblockRow' ),
24	9	$hideUser = $( '#wpEnableHideUser' ), $watchUser = $( '#wpEnableWatchUser' );
25	10
26		~~- var considerChangingExpiryFocus = function( instant ) {~~
27		~~- if ( $blockExpiry.val() == 'other' ) {~~
28		~~- $blockOther.goIn( instant );~~
29		~~- } else {~~
30		~~- $blockOther.goOut( instant );~~
31		~~- }~~
32		~~- };~~
33	11	var updateBlockOptions = function( instant ) {
34	12	if ( !$blockTarget.length ) {
35	13	return;
—	—	@@ -59,10 +37,8 @@
60	38	};
61	39
62	40	// Bind functions so they're checked whenever stuff changes
63		~~- $blockExpiry.change( considerChangingExpiryFocus );~~
64	41	$blockTarget.keyup( updateBlockOptions );
65	42
66	43	// Call them now to set initial state (ie. Special:Block/Foobar?wpBlockExpiry=2+hours)
67		~~- considerChangingExpiryFocus( DO_INSTANT );~~
68	44	updateBlockOptions( DO_INSTANT );
69	45	});
\ No newline at end of file
Index: trunk/phase3/resources/mediawiki/mediawiki.htmlform.js
—	—	@@ -0,0 +1,46 @@
	2	+/**
	3	+ * Utility functions for jazzing up HTMLForm elements
	4	+ */
	5	+
	6	+// Fade or snap depending on argument
	7	+jQuery.fn.goIn = function( instantToggle ) {
	8	+ if ( typeof instantToggle != 'undefined' && instantToggle === true ) {
	9	+ return $(this).show();
	10	+ }
	11	+ return jQuery(this).stop( true, true ).fadeIn();
	12	+};
	13	+jQuery.fn.goOut = function( instantToggle ) {
	14	+ if ( typeof instantToggle != 'undefined' && instantToggle === true ) {
	15	+ return $(this).hide();
	16	+ }
	17	+ return jQuery(this).stop( true, true ).fadeOut();
	18	+};
	19	+
	20	+/**
	21	+ * Bind a function to the jQuery object via live(), and also immediately trigger
	22	+ * the function on the objects with an 'instant' paramter set to true
	23	+ * @param callback function taking one paramter, which is Bool true when the event
	24	+ * is called immediately, and the EventArgs object when triggered from an event
	25	+ */
	26	+jQuery.fn.liveAndTestAtStart = function( callback ){
	27	+ $(this)
	28	+ .live( 'change', callback )
	29	+ .each( function( index, element ){
	30	+ callback.call( this, true );
	31	+ } );
	32	+}
	33	+
	34	+jQuery( function( $ ) {
	35	+
	36	+ // animate the SelectOrOther fields, to only show the text field when
	37	+ // 'other' is selected
	38	+ $( '.mw-htmlform-select-or-other' ).liveAndTestAtStart( function( instant ){
	39	+ var $other = $( '#' + $(this).attr( 'id' ) + '-other' );
	40	+ if ( $(this).val() == 'other' ) {
	41	+ $other.goIn( instant );
	42	+ } else {
	43	+ $other.goOut( instant );
	44	+ }
	45	+ });
	46	+
	47	+});
\ No newline at end of file
Property changes on: trunk/phase3/resources/mediawiki/mediawiki.htmlform.js
___________________________________________________________________
Added: svn:eol-style
1	48	+ native
Index: trunk/extensions/GlobalBlocking/SpecialGlobalBlock.php
—	—	@@ -221,7 +221,7 @@
222	222	'wpExpiryOther',
223	223	45,
224	224	$this->mExpiry == $this->mExpirySelection ? '' : $this->mExpiry,
225		~~- array( 'id' => 'mw-globalblock-expiry-other' )~~
	225	+ array( 'id' => 'mw-globalblock-expiry-selector-other' )
226	226	);
227	227	}
228	228
—	—	@@ -264,6 +264,9 @@
265	265	$form .= Xml::closeElement( 'form' );
266	266	$form .= Xml::closeElement( 'fieldset' );
267	267
	268	+ #FIXME: make this actually use HTMLForm, instead of just its JavaScript
	269	+ $wgOut->addModules( 'mediawiki.htmlform' );
	270	+
268	271	$wgOut->addHTML( $form );
269	272
270	273	// Show loglist of previous blocks
—	—	@@ -289,7 +292,10 @@
290	293	if ($id == null) { $id = $name; }
291	294	if ($selected == null) { $selected = 'other'; }
292	295
293		~~- $attribs = array( 'id' => $id, 'name' => $name, 'onchange' => 'considerChangingExpiryFocus()' );~~
	296	+ $attribs = array(
	297	+ 'id' => $id,
	298	+ 'name' => $name,
	299	+ 'class' => 'mw-htmlform-select-or-other' ); # FIXME: make this actually use HTMLForm
294	300
295	301	$selector .= Xml::openElement( 'select', $attribs );
296	302
Index: trunk/extensions/CheckUser/CheckUser.php
—	—	@@ -71,8 +71,8 @@
72	72	$wgHooks['ContributionsToolLinks'][] = 'efLoadCheckUserLink';
73	73
74	74	$wgResourceModules['ext.checkUser'] = array(
75		~~- 'scripts' => 'checkuser.js',~~
76		~~- 'dependencies' => array( 'mediawiki.legacy.block' ), // IP stuff~~
	75	+ 'scripts' => 'checkuser.js',
	76	+ 'dependencies' => array( 'mediawiki.util' ), // IP stuff
77	77	'localBasePath' => dirname( __FILE__ ),
78	78	'remoteExtPath' => 'CheckUser',
79	79	);
Index: trunk/extensions/CheckUser/checkuser.js
—	—	@@ -40,11 +40,11 @@
41	41	// Go through each IP in the list, get its binary form, and
42	42	// track the largest binary prefix among them...
43	43	for( var i = 0; i < ips.length; i++ ) {
44		~~- // ...in the spirit of block.js, call this "addy"~~
	44	+ // ...in the spirit of mediawiki.special.block.js, call this "addy"
45	45	var addy = ips[i].replace(/^\s\|\s$/, '' ); // trim
46	46	// Match the first IP in each list (ignore other garbage)
47		~~- var ipV4 = isIPv4Address( addy, true ); // from block.js~~
48		~~- var ipV6 = isIPv6Address( addy, true ); // from block.js~~
	47	+ var ipV4 = mw.util.isIPv4Address( addy, true );
	48	+ var ipV6 = mw.util.isIPv6Address( addy, true );
49	49	var ip_cidr = addy.match(/^(.*)(?:\/(\d+))?$/);
50	50	// Binary form
51	51	var bin = new String( '' );

Sign-offs

User	Flag	Date
Krinkle	inspected	23:35, 10 June 2011

Follow-up revisions

Revision	Commit summary	Author	Date
r83300	Follow-up r83280; looks like I accidentally deprecated a piece of legacy JS :D	happy-melon	17:06, 5 March 2011

Past revisions this follows-up on

Revision	Commit summary	Author	Date
r83183	Convert Special:BlockIP JS to jQuery/ResourceLoader. Change the order of the...	happy-melon	23:54, 3 March 2011
r83202	Moving IP-check to mw.util & some enhancements to and applying code conventio...	krinkle	01:48, 4 March 2011

Comments

#Comment by Happy-melon (talk | contribs) 12:49, 5 March 2011

Hmm, seem to be quite a few random whitespace changes in HTMLForm.php...

#Comment by Aaron Schulz (talk | contribs) 16:07, 16 June 2011

- if( $showblockoptions ) {

Why was that check shifted down?

#Comment by Aaron Schulz (talk | contribs) 23:00, 11 July 2011

Nevermind.

Status & tagging log

22:59, 11 July 2011 Aaron Schulz (talk | contribs) changed the status of r83280 [removed: new added: ok]