r103865 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r103864‎ | r103865 | r103866 >
Date:23:16, 21 November 2011
Author:brion
Status:ok (Comments)
Tags:
Comment:
* (bug 32548) fix minification bug when numeric literal with exponent was split over lines

This broke the OpenLayers support in the Maps extension, as used for example on TranslateWiki.net.
The original JavaScriptMinifier's tokenizer (r83885) explicitly didn't bother looking for the exponent part because it "didn't matter" to its internal state machine; however since r83891 added a max line length that definitely is not true.

I've split out handling of hex and decimal numerals, and let the decimal numeral handling check for exponents.

PHPUnit test cases were added in r103846.
Modified paths:
  • /trunk/phase3/includes/libs/JavaScriptMinifier.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/libs/JavaScriptMinifier.php
@@ -484,23 +484,34 @@
485485 $end++;
486486 }
487487 } elseif(
 488+ $ch === '0'
 489+ && ($pos + 1 < $length) && ($s[$pos + 1] === 'x' || $s[$pos + 1] === 'X' )
 490+ ) {
 491+ // Hex numeric literal
 492+ $end++; // x or X
 493+ $end += strspn( $s, '0123456789ABCDEF', $end );
 494+ // @fixme if no hex digits, parse error
 495+ } elseif(
488496 ctype_digit( $ch )
489497 || ( $ch === '.' && $pos + 1 < $length && ctype_digit( $s[$pos + 1] ) )
490498 ) {
491 - // Numeric literal. Search for the end of it, but don't care about [+-]exponent
492 - // at the end, as the results of "numeric [+-] numeric" and "numeric" are
493 - // identical to our state machine.
494 - $end += strspn( $s, '0123456789ABCDEFabcdefXx.', $end );
495 - while( $s[$end - 1] === '.' ) {
496 - // Special case: When a numeric ends with a dot, we have to check the
497 - // literal for proper syntax
498 - $decimal = strspn( $s, '0123456789', $pos, $end - $pos - 1 );
499 - if( $decimal === $end - $pos - 1 ) {
500 - break;
501 - } else {
502 - $end--;
503 - }
 499+ $end += strspn( $s, '0123456789', $end );
 500+ $decimal = strspn( $s, '.', $end );
 501+ if ($decimal) {
 502+ $end += $decimal;
 503+ $end += strspn( $s, '0123456789', $end );
 504+ // @fixme If no decimal digits after the . we cannot be followed
 505+ // by an identifier, and should throw a parse error
504506 }
 507+ $exponent = strspn( $s, 'eE', $end );
 508+ if( $exponent ) {
 509+ $end += $exponent;;
 510+ // + sign is optional; - sign is required.
 511+ $end += strspn( $s, '-+', $end );
 512+ $end += strspn( $s, '0123456789', $end );
 513+ // @fixme if no decimal digits after the e/+/- we should
 514+ // throw a parse error
 515+ }
505516 } elseif( isset( $opChars[$ch] ) ) {
506517 // Punctuation character. Search for the longest matching operator.
507518 while(

Follow-up revisions

RevisionCommit summaryAuthorDate
r103910Follow-up r103865. Accept lowercase hex and remove empty statement.platonides16:04, 22 November 2011
r103915Fix r103865 fixmes about case where there a parse error should be raised....platonides16:21, 22 November 2011
r103928Follow-up r103915. The @fixme of r103865 was wrong....platonides17:48, 22 November 2011
r103978Followup r103865 and r103915...brion23:10, 22 November 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r83885(bug 27528) Incorporate Paul Copperman's minifiercatrope11:44, 14 March 2011
r83891Followup r83885: implement maximum line length and statement termination (eac...catrope13:24, 14 March 2011
r103846Add PHPUnit tests for the minification failure case in bug 32548....brion22:20, 21 November 2011

Comments

#Comment by Raymond (talk | contribs)   08:21, 22 November 2011
#Comment by Brion VIBBER (talk | contribs)   23:18, 22 November 2011

Old bad copy is probably stuck in cache... lemme see if can increment a version or something.

#Comment by Brion VIBBER (talk | contribs)   23:21, 22 November 2011

Ok, done in r103979.

#Comment by Lupo (talk | contribs)   14:15, 22 November 2011

Hex case lacks the lower case "abcdef" in strspn.

Stray semicolon after "$end += $exponent;".

#Comment by Brion VIBBER (talk | contribs)   23:18, 22 November 2011

Ok I think we've cleaned up the above bits; reverting to 'new' state.

Status & tagging log