Fix bug 34684 in my PathRouter code:

- Update the tests to test extra characters and patterns like like \\ and $1
- Also update the tests to make sure that matches that don't have enough data to work fail
- Replace the str_replace and preg_match based code with code based on preg_replace_callback.

diff --git a/includes/AutoLoader.php b/includes/AutoLoader.php
index 4540c79..0df2dcb 100644 (file)
--- a/includes/AutoLoader.php
+++ b/includes/AutoLoader.php
@@ -165,6 +165,7 @@ $wgAutoloadLocalClasses = array(
        'Pager' => 'includes/Pager.php',
        'PasswordError' => 'includes/User.php',
        'PathRouter' => 'includes/PathRouter.php',
+       'PathRouterPatternReplacer' => 'includes/PathRouter.php',
        'PermissionsError' => 'includes/Exception.php',
        'PhpHttpRequest' => 'includes/HttpFunctions.php',
        'PoolCounter' => 'includes/PoolCounter.php',

diff --git a/includes/PathRouter.php b/includes/PathRouter.php
index 53ddd42..29df4b4 100644 (file)
--- a/includes/PathRouter.php
+++ b/includes/PathRouter.php
@@ -278,20 +278,14 @@ class PathRouter {
                                } elseif ( isset( $paramData['pattern'] ) ) {
                                        // For patterns we have to make value replacements on the string
                                        $value = $paramData['pattern'];
-                                       // For each $# match replace any $# within the value
-                                       foreach ( $m as $matchKey => $matchValue ) {
-                                               if ( preg_match( '/^par\d+$/u', $matchKey ) ) {
-                                                       $n = intval( substr( $matchKey, 3 ) );
-                                                       $value = str_replace( '$' . $n, rawurldecode( $matchValue ), $value );
-                                               }
-                                       }
-                                       // If a key was set replace any $key within the value
+                                       $replacer = new PathRouterPatternReplacer;
+                                       $replacer->params = $m;
                                        if ( isset( $pattern->key ) ) {
-                                               $value = str_replace( '$key', $pattern->key, $value );
+                                               $replacer->key = $pattern->key;
                                        }
-                                       if ( preg_match( '/\$(\d+|key)/u', $value ) ) {
-                                               // Still contains $# or $key patterns after replacement
-                                               // Seams like we don't have all the data, abort
+                                       $value = $replacer->replace( $value );
+                                       if ( $value === false ) {
+                                               // Pattern required data that wasn't available, abort
                                                return null;
                                        }
                                }
@@ -317,3 +311,41 @@ class PathRouter {
        }
 
 }
+
+class PathRouterPatternReplacer {
+
+       public $key, $params, $error;
+
+       /**
+        * Replace keys inside path router patterns with text.
+        * We do this inside of a replacement callback because after replacement we can't tell the
+        * difference between a $1 that was not replaced and a $1 that was part of
+        * the content a $1 was replaced with.
+        */
+       public function replace( $value ) {
+               $this->error = false;
+               $value = preg_replace_callback( '/\$(\d+|key)/u', array( $this, 'callback' ), $value );
+               if ( $this->error ) {
+                       return false;
+               }
+               return $value;
+       }
+
+       protected function callback( $m ) {
+               if ( $m[1] == "key" ) {
+                       if ( is_null( $this->key ) ) {
+                               $this->error = true;
+                               return '';
+                       }
+                       return $this->key;
+               } else {
+                       $d = $m[1];
+                       if ( !isset( $this->params["par$d"] ) ) {
+                               $this->error = true;
+                               return '';
+                       }
+                       return rawurldecode( $this->params["par$d"] );
+               }
+       }
+
+}
\ No newline at end of file

diff --git a/tests/phpunit/includes/PathRouterTest.php b/tests/phpunit/includes/PathRouterTest.php
index 3b29ef8..f627458 100644 (file)
--- a/tests/phpunit/includes/PathRouterTest.php
+++ b/tests/phpunit/includes/PathRouterTest.php
@@ -124,6 +124,16 @@ class PathRouterTest extends MediaWikiTestCase {
                ) );
        }
 
+       /**
+        * Test to ensure that matches are not made if a parameter expects nonexistent input
+        */
+       public function testFail() {
+               $router = new PathRouter;
+               $router->add( "/wiki/$1", array( 'title' => "$1$2" ) );
+               $matches = $router->parse( "/wiki/A" );
+               $this->assertEquals( array(), $matches );
+       }
+
        /**
         * Test to ensure weight of paths is handled correctly
         */
@@ -172,12 +182,30 @@ class PathRouterTest extends MediaWikiTestCase {
                $this->assertEquals( $matches, array( 'title' => "Title_With Space" ) );
        }
 
+       public function dataRegexpChars() {
+               return array(
+                       array( "$" ),
+                       array( "$1" ),
+                       array( "\\" ),
+                       array( "\\$1" ),
+               );
+       }
+
+       /**
+        * Make sure the router doesn't break on special characters like $ used in regexp replacements
+        * @dataProvider dataRegexpChars
+        */
+       public function testRegexpChars( $char ) {
+               $matches = $this->basicRouter->parse( "/wiki/$char" );
+               $this->assertEquals( $matches, array( 'title' => "$char" ) );
+       }
+
        /**
         * Make sure the router handles characters like +&() properly
         */
        public function testCharacters() {
-               $matches = $this->basicRouter->parse( "/wiki/Plus+And&Stuff()" );
-               $this->assertEquals( $matches, array( 'title' => "Plus+And&Stuff()" ) );
+               $matches = $this->basicRouter->parse( "/wiki/Plus+And&Dollar\\Stuff();[]{}*" );
+               $this->assertEquals( $matches, array( 'title' => "Plus+And&Dollar\\Stuff();[]{}*" ) );
        }
 
        /**