r78161 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r78160‎ | r78161 | r78162 >
Date:00:16, 10 December 2010
Author:aaron
Status:ok (Comments)
Tags:
Comment:
*Follow-up r78099,r76275:
**Converted CheckUser to use RL
**Made checkuser.js borrow block.js IP functions to avoid ugly duplication (and fix the IP recognition)
**Added missing backslashes to block.js regex strings and avoided backreferences (which work the opposite of PCRE when referencing an unmatched group)
*Made CU not give a common prefix if both v6 and v4 addresses are on the list
Modified paths:
  • /trunk/extensions/CheckUser/CheckUser.php (modified) (history)
  • /trunk/extensions/CheckUser/CheckUser_body.php (modified) (history)
  • /trunk/extensions/CheckUser/checkuser.js (modified) (history)
  • /trunk/phase3/skins/common/block.js (modified) (history)

Diff [purge]

Index: trunk/phase3/skins/common/block.js
@@ -1,15 +1,16 @@
22 // @TODO: find some better JS file for this
33 // Note: borrows from IP.php
44 window.isIPv4Address = function( address, allowBlock ) {
 5+ var block = allowBlock ? '(?:\\/(?:3[0-2]|[12]?\\d))?' : '';
56 var RE_IP_BYTE = '(?:25[0-5]|2[0-4][0-9]|1[0-9][0-9]|0?[0-9]?[0-9])';
6 - var RE_IP_ADD = '(?:' + RE_IP_BYTE + '\.){3}' + RE_IP_BYTE;
7 - var block = allowBlock ? '(?:\/(?:3[0-2]|[12]?\\d))?' : '';
 7+ var RE_IP_ADD = '(?:' + RE_IP_BYTE + '\\.){3}' + RE_IP_BYTE;
88 return address.search( new RegExp( '^' + RE_IP_ADD + block + '$' ) ) != -1;
99 };
1010
1111 // @TODO: find some better JS file for this
1212 // Note: borrows from IP.php
1313 window.isIPv6Address = function( address, allowBlock ) {
 14+ var block = allowBlock ? '(?:\\/(?:12[0-8]|1[01][0-9]|[1-9]?\\d))?' : '';
1415 var RE_IPV6_ADD =
1516 '(?:' + // starts with "::" (including "::")
1617 ':(?::|(?::' + '[0-9A-Fa-f]{1,4}' + '){1,7})' +
@@ -17,11 +18,14 @@
1819 '[0-9A-Fa-f]{1,4}' + '(?::' + '[0-9A-Fa-f]{1,4}' + '){0,6}::' +
1920 '|' + // contains no "::"
2021 '[0-9A-Fa-f]{1,4}' + '(?::' + '[0-9A-Fa-f]{1,4}' + '){7}' +
21 - '|' + // contains one "::" in the middle
22 - '[0-9A-Fa-f]{1,4}' + '(?::(:())?' + '[0-9A-Fa-f]{1,4}' + '(?!\1)){1,6}\2' +
2322 ')';
24 - var block = allowBlock ? '(?:\/(?:12[0-8]|1[01][0-9]|[1-9]?\\d))?' : '';
25 - return address.search( new RegExp( '^' + RE_IPV6_ADD + block + '$' ) ) != -1;
 23+ if ( address.search( new RegExp( '^' + RE_IPV6_ADD + block + '$' ) ) != -1 ) {
 24+ return true;
 25+ }
 26+ var RE_IPV6_ADD = // contains one "::" in the middle (single '::' check below)
 27+ '[0-9A-Fa-f]{1,4}' + '(?:::?' + '[0-9A-Fa-f]{1,4}' + '){1,6}';
 28+ return address.search( new RegExp( '^' + RE_IPV6_ADD + block + '$' ) ) != -1
 29+ && address.search( /::/ ) != -1 && address.search( /::.*::/ ) == -1;
2630 };
2731
2832 window.considerChangingExpiryFocus = function() {
Index: trunk/extensions/CheckUser/CheckUser_body.php
@@ -115,7 +115,7 @@
116116 }
117117 # Add CIDR calculation convenience form
118118 $this->addJsCIDRForm();
119 - $this->addStyles();
 119+ $wgOut->addModules( 'ext.checkUser' ); // JS
120120 }
121121
122122 /**
@@ -204,16 +204,6 @@
205205 }
206206
207207 /**
208 - * Add CSS/JS
209 - */
210 - protected function addStyles() {
211 - global $wgScriptPath, $wgCheckUserStyleVersion, $wgOut;
212 - // FIXME, use Html::
213 - $encJSFile = htmlspecialchars( "$wgScriptPath/extensions/CheckUser/checkuser.js?$wgCheckUserStyleVersion" );
214 - $wgOut->addScript( "<script type=\"text/javascript\" src=\"$encJSFile\"></script>" );
215 - }
216 -
217 - /**
218208 * Get a selector of time period options
219209 * @param int $selected, selected level
220210 */
Index: trunk/extensions/CheckUser/CheckUser.php
@@ -61,7 +61,6 @@
6262 $wgCheckUserStyleVersion = 5;
6363
6464 # Recent changes data hook
65 -global $wgHooks;
6665 $wgHooks['RecentChange_save'][] = 'efUpdateCheckUserData';
6766 $wgHooks['EmailUser'][] = 'efUpdateCUEmailData';
6867 $wgHooks['User::mailPasswordInternal'][] = 'efUpdateCUPasswordResetData';
@@ -71,6 +70,15 @@
7271 $wgHooks['LoadExtensionSchemaUpdates'][] = 'efCheckUserSchemaUpdates';
7372 $wgHooks['ContributionsToolLinks'][] = 'efLoadCheckUserLink';
7473
 74+$wgResourceModules['ext.checkUser'] = array(
 75+ 'scripts' => 'checkuser.js',
 76+ 'dependencies' => array( 'mediawiki.legacy.block' ), // IP stuff
 77+ 'localBasePath' => dirname( __FILE__ ),
 78+ 'remoteExtPath' => 'CheckUser',
 79+);
 80+
 81+// TODO: move hooks to CheckUser.hooks.php
 82+
7583 /**
7684 * Hook function for RecentChange_save
7785 * Saves user data into the cu_changes table
Index: trunk/extensions/CheckUser/checkuser.js
@@ -6,7 +6,7 @@
77 * This function calculates the common range of a list of
88 * IPs. It should be set to update on keyUp.
99 */
10 -function updateCIDRresult() {
 10+window.updateCIDRresult = function() {
1111 var form = document.getElementById( 'mw-checkuser-cidrform' );
1212 if( !form ) {
1313 return; // no JS form
@@ -20,7 +20,7 @@
2121 // Each line should have one IP or range
2222 if( text.indexOf("\n") != -1 ) {
2323 var ips = text.split("\n");
24 - // Try some other delimiters too
 24+ // Try some other delimiters too...
2525 } else if( text.indexOf("\t") != -1 ) {
2626 var ips = text.split("\t");
2727 } else if( text.indexOf(",") != -1 ) {
@@ -35,24 +35,29 @@
3636 var bin_prefix = 0;
3737 var prefix_cidr = 0;
3838 var prefix = new String( '' );
 39+ var foundV4 = false;
 40+ var foundV6 = false;
3941 // Go through each IP in the list, get its binary form, and
4042 // track the largest binary prefix among them...
4143 for( var i = 0; i < ips.length; i++ ) {
4244 var invalid = false;
4345 // ...in the spirit of block.js, call this "addy"
44 - var addy = ips[i];
45 - // @TODO: get some core JS IP functions
 46+ var addy = ips[i].replace(/^\s*|\s*$/, '' ); // trim
4647 // Match the first IP in each list (ignore other garbage)
47 - var ipV4 = addy.match(/(^|\b)(\d+\.\d+\.\d+\.\d+)(\/\d+)?\b/);
48 - // Regexp has 3 cases: (starts with '::',ends with '::',neither)
49 - var ipV6 = !addy.match(/::.*::/) // not ambiguous
50 - && addy.match(/(^|\b)(:(:[0-9A-Fa-f]{1,4}){1,7}|[0-9A-Fa-f]{1,4}(::?[0-9A-Fa-f]{1,4}){0,6}::|[0-9A-Fa-f]{1,4}(::?[0-9A-Fa-f]{1,4}){1,7})(\/\d+)?\b/);
 48+ var ipV4 = isIPv4Address( addy, true ); // from block.js
 49+ var ipV6 = isIPv6Address( addy, true ); // from block.js
 50+ var ip_cidr = addy.match(/^(.*)(?:\/(\d+))?$/);
5151 // Binary form
5252 var bin = new String( '' );
5353 // Convert the IP to binary form: IPv4
5454 if( ipV4 ) {
55 - var ip = ipV4[2];
56 - var cidr = ipV4[3]; // CIDR, if it exists
 55+ foundV4 = true;
 56+ if ( foundV6 ) { // disjoint address space
 57+ prefix = '';
 58+ break;
 59+ }
 60+ var ip = ip_cidr[1];
 61+ var cidr = ip_cidr[2] ? ip_cidr[2] : null; // CIDR, if it exists
5762 // Get each quad integer
5863 var blocs = ip.split('.');
5964 // IANA 1.0.0.0/8, 2.0.0.0/8
@@ -77,7 +82,6 @@
7883 prefix = ''; // Rebuild formatted bin_prefix for each IP
7984 // Apply any valid CIDRs
8085 if( cidr ) {
81 - cidr = cidr.match( /\d+$/ )[0]; // get rid of slash
8286 bin = bin.substring( 0, cidr ); // truncate bin
8387 }
8488 // Init bin_prefix
@@ -122,14 +126,15 @@
123127 }
124128 // Convert the IP to binary form: IPv6
125129 } else if( ipV6 ) {
126 - var ip = ipV6[2];
127 - var cidr = ipV6[0].match( /\/\d+$/ );
128 - cidr = cidr ? cidr[0] : false;
129 - var abbrevs = ip.match( /::/g );
130 - if( abbrevs && abbrevs.length > 1 ) {
131 - continue; // bad IP!
 130+ foundV6 = true;
 131+ if ( foundV4 ) { // disjoint address space
 132+ prefix = '';
 133+ break;
132134 }
 135+ var ip = ip_cidr[1];
 136+ var cidr = ip_cidr[2] ? ip_cidr[2] : null; // CIDR, if it exists
133137 // Expand out "::"s
 138+ var abbrevs = ip.match( /::/g );
134139 if( abbrevs && abbrevs.length > 0 ) {
135140 var colons = ip.match( /:/g );
136141 var needed = 7 - ( colons.length - 2 ); // 2 from "::"
@@ -139,7 +144,8 @@
140145 needed--;
141146 }
142147 ip = ip.replace( '::', insert + ':' );
143 - // For IPs that start with "::", correct the final IP so that it starts with '0' and not ':'
 148+ // For IPs that start with "::", correct the final IP
 149+ // so that it starts with '0' and not ':'
144150 if( ip[0] == ':' ) {
145151 ip = '0' + ip;
146152 }
@@ -165,7 +171,6 @@
166172 prefix = ''; // Rebuild formatted bin_prefix for each IP
167173 // Apply any valid CIDRs
168174 if( cidr ) {
169 - cidr = cidr.match( /\d+$/ )[0]; // get rid of slash
170175 bin = bin.substring( 0, cidr ); // truncate bin
171176 }
172177 // Init bin_prefix
@@ -228,7 +233,7 @@
229234 addOnloadHook( updateCIDRresult );
230235
231236 // Utility function to convert hex to integers
232 -function hex2int( hex ) {
 237+window.hex2int = function( hex ) {
233238 hex = new String( hex );
234239 hex = hex.toLowerCase();
235240 var intform = 0;

Follow-up revisions

RevisionCommit summaryAuthorDate
r78162* Remove some unecessary JS code (given r78161)...aaron00:26, 10 December 2010
r79129MFT r78011 r78014 r78015 r78016 r78099 r78117 r78161 r78170 r78172 r78199 r78......platonides19:58, 28 December 2010
r79343MFT r78161 and r78511, incorrectly stated as merged in r79129....platonides15:56, 31 December 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r76275Similar to r76267 but for JS. Should finish bug 24293.aaron22:46, 7 November 2010
r78099*Follow-up r76275: regexp improvement akin to IP.php...aaron22:33, 8 December 2010

Comments

#Comment by Catrope (talk | contribs)   10:26, 10 December 2010
+$wgResourceModules['ext.checkUser'] = array(
+	'scripts' 		=> 'checkuser.js',
+	'dependencies' 	=> array( 'mediawiki.legacy.block' ), // IP stuff
+	'localBasePath' => dirname( __FILE__ ),
+    'remoteExtPath' => 'CheckUser',
+);

Indentation messed up on the second and fifth line.

-function hex2int( hex ) {
+window.hex2int = function( hex ) {

You need to change the closing } of the function to }; to terminate the assignment statement. JS tolerates unterminated statements by automagically inserting semicolons, but this behavior should not be relied on and breaks in certain cases.

RL-ification looks OK otherwise; have not reviewed the behavioral JS changes, so not marking as OK.

#Comment by Aaron Schulz (talk | contribs)   14:51, 10 December 2010

I know that ";" should be used for these kind of declarations. I didn't notice this file was missing them though. This problem seems to be all over the place.

#Comment by Catrope (talk | contribs)   14:52, 10 December 2010

It's only needed for window.foo = function() { ... }; (which is an assignment), not for function foo() { ... } (which is a declaration).

#Comment by Aaron Schulz (talk | contribs)   15:03, 10 December 2010

Yes, I know, I was calling the former a "declaration".

Status & tagging log