r61713 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r61712‎ | r61713 | r61714 >
Date:12:46, 30 January 2010
Author:conrad
Status:resolved (Comments)
Tags:
Comment:
Fix for r61710. Changing subst: to subst:$1 would cause huge problems with localisation
instead add some proper functions to MagicWord.php to deal with the situation.
Modified paths:
  • /trunk/phase3/includes/MagicWord.php (modified) (history)
  • /trunk/phase3/includes/parser/Parser.php (modified) (history)
  • /trunk/phase3/languages/messages/MessagesEn.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/parser/Parser.php
——@@ -2799,18 +2799,14 @@
28002800 wfProfileIn( __METHOD__.'-modifiers' );
28012801 if ( !$found ) {
28022802
2803 - $substMatch = $this->mSubsts->matchVariableStartToEnd( $part1 );
 2803+ $substMatch = $this->mSubsts->matchStartAndRemove( $part1 );
28042804
2805 - # Possibilities for substMatch[0]: "subst", "safesubst" or FALSE
 2805+ # Possibilities for substMatch: "subst", "safesubst" or FALSE
28062806 # Whether to include depends also on whether we are in the pre-save-transform
28072807 #
2808 - # safesubst || (subst && PST) => transclude (handled by if)
2809 - # (false && PST) || (subst && !PST) => return input (handled by else if)
2810 - # false && !PST => transclude (no handling needed here)
2811 - if ( $substMatch[0] && ( $this->ot['wiki'] || $substMatch[0] == 'safesubst' ) ) {
2812 - $part1 = $substMatch[1];
2813 -
2814 - } else if ( $substMatch[0] xor $this->ot['wiki'] ) {
 2808+ # safesubst || (subst && PST) || (false && !PST) => transclude (skip the if)
 2809+ # (false && PST) || (subst && !PST) => return input (handled by if)
 2810+ if ( $substMatch != 'safesubst' && ($substMatch == 'subst' xor $this->ot['wiki']) ) {
28152811 $text = $frame->virtualBracketedImplode( '{{', '|', '}}', $titleWithSpaces, $args );
28162812 $isLocalObj = true;
28172813 $found = true;
Index: trunk/phase3/includes/MagicWord.php
——@@ -594,6 +594,21 @@
595595 }
596596
597597 /**
 598+ * Get a regex for matching a prefix. Does not match parameters.
 599+ */
 600+ function getRegexStart() {
 601+ $base = $this->getBaseRegex();
 602+ $newRegex = array( '', '' );
 603+ if ( $base[0] !== '' ) {
 604+ $newRegex[0] = str_replace( "\\$1", "", "/^(?:{$base[0]})/iuS" );
 605+ }
 606+ if ( $base[1] !== '' ) {
 607+ $newRegex[1] = str_replace( "\\$1", "", "/^(?:{$base[1]})/S" );
 608+ }
 609+ return $newRegex;
 610+ }
 611+
 612+ /**
598613 * Get an anchored regex for matching variables
599614 */
600615 function getVariableStartToEndRegex() {
——@@ -691,4 +706,24 @@
692707 }
693708 return $found;
694709 }
 710+
 711+ /**
 712+ * Returns the magic word id removed from the start, or false
 713+ * does not match parameters.
 714+ */
 715+ public function matchStartAndRemove( &$text ) {
 716+ $found = FALSE;
 717+ $regexes = $this->getRegexStart();
 718+ foreach ( $regexes as $regex ) {
 719+ if ( $regex === '' ) {
 720+ continue;
 721+ }
 722+ preg_match_all( $regex, $text, $matches, PREG_SET_ORDER );
 723+ foreach ( $matches as $m ) {
 724+ list( $found, $param ) = $this->parseMatch( $m );
 725+ }
 726+ $text = preg_replace( $regex, '', $text );
 727+ }
 728+ return $found;
 729+ }
695730 }
Index: trunk/phase3/languages/messages/MessagesEn.php
——@@ -264,8 +264,8 @@
265265 'subjectpagename' => array( 1, 'SUBJECTPAGENAME', 'ARTICLEPAGENAME' ),
266266 'subjectpagenamee' => array( 1, 'SUBJECTPAGENAMEE', 'ARTICLEPAGENAMEE' ),
267267 'msg' => array( 0, 'MSG:' ),
268 - 'subst' => array( 0, 'SUBST:$1' ),
269 - 'safesubst' => array( 0, 'SAFESUBST:$1' ),
 268+ 'subst' => array( 0, 'SUBST:' ),
 269+ 'safesubst' => array( 0, 'SAFESUBST:' ),
270270 'msgnw' => array( 0, 'MSGNW:' ),
271271 'img_thumbnail' => array( 1, 'thumbnail', 'thumb' ),
272272 'img_manualthumb' => array( 1, 'thumbnail=$1', 'thumb=$1'),

Follow-up revisions

RevisionCommit summaryAuthorDate
r62505clean r61713 (and r61710) per code reviewconrad09:34, 15 February 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r61710bug 22297 - "syntax for substitution that doesn't break transclusion"...conrad11:58, 30 January 2010

Comments

#Comment by Tim Starling (talk | contribs)   03:12, 15 February 2010

Review of r61710 and r61713 together:

Rename $this->mSubsts to something more descriptive, like $this->mSubstWords or $this->mSubstMagicArray. 

# safesubst || (subst && PST) || (false && !PST) => transclude (skip the if)
# (false && PST) || (subst && !PST)  => return input (handled by if)
if ( $substMatch != 'safesubst' && ($substMatch == 'subst' xor $this->ot['wiki']) ) {

Neither your cryptic comment nor the line below it is comprehensible without an awful lot of staring. It would not be hard to rewrite it in a way that humans can understand, e.g. 

if ( $this->ot['wiki'] ) {
    if ( $substMatch ) {
        // Subst during PST
        $literal = false;
    } else {
        // PST with no subst
        $literal = true;
    }
} else {
    if ( $substMatch === false || $substMatch === 'safesubst' ) {
        // Ordinary template expansion
        $literal = false;
    } else {
        // Plain subst not allowed during non-PST expansion
        $literal = true;
    }
}
if ( $literal ) {
    $found = true;
    ...
}

The problem with the original code, which you are exacerbating, is that it's excessively short and cute, apparently due to a misguided attempt at cleverness.

You should use preg_match() not preg_match_all() in matchStartAndRemove(), since only a single match is possible. You could have used MagicWordArray::matchVariableStartToEnd() as a model, it uses preg_match() in a very similar way. Also note that you don't have to run preg_replace() just to remove this single match. You can use substr(), which is faster.

I'm not sure why you're removing $1 in getRegexStart(). Is it just for backwards compatibility with old versions of MessagesEn.php?

#Comment by Conrad.Irwin (talk | contribs)   09:40, 15 February 2010

I was removing $1 to make it clear that $1 would not be matched, as MagicWordArray::getRegex() does not do anything to the prefix, now neither does getRegexStart()

Status & tagging log