r104474 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r104473‎ | r104474 | r104475 >
Date:19:55, 28 November 2011
Author:brion
Status:ok (Comments)
Tags:
Comment:
Revert r104274, r104284, r104285 -- breaks special pages on non-english
Modified paths:
  • /trunk/phase3/docs/hooks.txt (modified) (history)
  • /trunk/phase3/includes/AutoLoader.php (modified) (history)
  • /trunk/phase3/includes/PathRouter.php (modified) (history)
  • /trunk/phase3/includes/WebRequest.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/PathRouterTest.php (modified) (history)

Diff [purge]

Index: trunk/phase3/docs/hooks.txt
——@@ -2191,8 +2191,14 @@
21922192 $redirect: whether the page is a redirect
21932193 $skin: Skin object
21942194
2195 -'WebRequestPathInfoRouter': While building the PathRouter to parse the REQUEST_URI.
2196 -$router: The PathRouter instance
 2195+'WebRequestGetPathInfoRequestURI': while extracting path info from REQUEST_URI.
 2196+ Allows an extension to extend the extraction of titles from paths.
 2197+ Implementing hooks should follow the pattern used in core:
 2198+ * Use the `$matches = WebRequest::extractTitle` pattern
 2199+ * Ensure that you test `if ( !$matches ) {` before you try extracting a title
 2200+ from the path so that you don't override an already found match.
 2201+$path: The request path to extract a title from.
 2202+&$matches: The array to apply matches to.
21972203
21982204 'WikiExporter::dumpStableQuery': Get the SELECT query for "stable" revisions
21992205 dumps
Index: trunk/phase3/tests/phpunit/includes/PathRouterTest.php
——@@ -1,155 +0,0 @@
2 -<?php
3 -/**
4 - * Tests for the PathRouter parsing
5 - */
6 -
7 -class PathRouterTest extends MediaWikiTestCase {
8 -
9 - /**
10 - * Test basic path parsing
11 - */
12 - public function testBasic() {
13 - $router = new PathRouter;
14 - $router->add("/$1");
15 - $matches = $router->parse( "/Foo" );
16 - $this->assertEquals( $matches, array( 'title' => "Foo" ) );
17 - }
18 -
19 - /**
20 - * Test loose path auto-$1
21 - */
22 - public function testLoose() {
23 - $router = new PathRouter;
24 - $router->add("/"); # Should be the same as "/$1"
25 - $matches = $router->parse( "/Foo" );
26 - $this->assertEquals( $matches, array( 'title' => "Foo" ) );
27 -
28 - $router = new PathRouter;
29 - $router->add("/wiki"); # Should be the same as /wiki/$1
30 - $matches = $router->parse( "/wiki/Foo" );
31 - $this->assertEquals( $matches, array( 'title' => "Foo" ) );
32 -
33 - $router = new PathRouter;
34 - $router->add("/wiki/"); # Should be the same as /wiki/$1
35 - $matches = $router->parse( "/wiki/Foo" );
36 - $this->assertEquals( $matches, array( 'title' => "Foo" ) );
37 - }
38 -
39 - /**
40 - * Test to ensure that path is based on specifity, not order
41 - */
42 - public function testOrder() {
43 - $router = new PathRouter;
44 - $router->add("/$1");
45 - $router->add("/a/$1");
46 - $router->add("/b/$1");
47 - $matches = $router->parse( "/a/Foo" );
48 - $this->assertEquals( $matches, array( 'title' => "Foo" ) );
49 -
50 - $router = new PathRouter;
51 - $router->add("/b/$1");
52 - $router->add("/a/$1");
53 - $router->add("/$1");
54 - $matches = $router->parse( "/a/Foo" );
55 - $this->assertEquals( $matches, array( 'title' => "Foo" ) );
56 - }
57 -
58 - /**
59 - * Test the handling of key based arrays with a url parameter
60 - */
61 - public function testKeyParameter() {
62 - $router = new PathRouter;
63 - $router->add( array( 'edit' => "/edit/$1" ), array( 'action' => '$key' ) );
64 - $matches = $router->parse( "/edit/Foo" );
65 - $this->assertEquals( $matches, array( 'title' => "Foo", 'action' => 'edit' ) );
66 - }
67 -
68 - /**
69 - * Test the handling of $2 inside paths
70 - */
71 - public function testAdditionalParameter() {
72 - // Basic $2
73 - $router = new PathRouter;
74 - $router->add( '/$2/$1', array( 'test' => '$2' ) );
75 - $matches = $router->parse( "/asdf/Foo" );
76 - $this->assertEquals( $matches, array( 'title' => "Foo", 'test' => 'asdf' ) );
77 - }
78 -
79 - /**
80 - * Test additional restricted value parameter
81 - */
82 - public function testRestrictedValue() {
83 - $router = new PathRouter;
84 - $router->add( '/$2/$1',
85 - array( 'test' => '$2' ),
86 - array( '$2' => array( 'a', 'b' ) )
87 - );
88 - $router->add( '/$2/$1',
89 - array( 'test2' => '$2' ),
90 - array( '$2' => 'c' )
91 - );
92 - $router->add( '/$1' );
93 -
94 - $matches = $router->parse( "/asdf/Foo" );
95 - $this->assertEquals( $matches, array( 'title' => "asdf/Foo" ) );
96 -
97 - $matches = $router->parse( "/a/Foo" );
98 - $this->assertEquals( $matches, array( 'title' => "Foo", 'test' => 'a' ) );
99 -
100 - $matches = $router->parse( "/c/Foo" );
101 - $this->assertEquals( $matches, array( 'title' => "Foo", 'test2' => 'c' ) );
102 - }
103 -
104 - public function callbackForTest( &$matches, $data ) {
105 - $matches['x'] = $data['$1'];
106 - $matches['foo'] = $data['foo'];
107 - }
108 -
109 - public function testCallback() {
110 - $router = new PathRouter;
111 - $router->add( "/$1",
112 - array( 'a' => 'b', 'data:foo' => 'bar' ),
113 - array( 'callback' => array( $this, 'callbackForTest' ) )
114 - );
115 - $matches = $router->parse( '/Foo');
116 - $this->assertEquals( $matches, array(
117 - 'title' => "Foo",
118 - 'x' => 'Foo',
119 - 'a' => 'b',
120 - 'foo' => 'bar'
121 - ) );
122 - }
123 -
124 - /**
125 - * Test to ensure weight of paths is handled correctly
126 - */
127 - public function testWeight() {
128 - $router = new PathRouter;
129 - $router->addStrict( "/Bar", array( 'ping' => 'pong' ) );
130 - $router->add( "/asdf-$1", array( 'title' => 'qwerty-$1' ) );
131 - $router->add( "/$1" );
132 - $router->add( "/qwerty-$1", array( 'title' => 'asdf-$1' ) );
133 - $router->addStrict( "/Baz", array( 'marco' => 'polo' ) );
134 - $router->add( "/a/$1" );
135 - $router->add( "/asdf/$1" );
136 - $router->add( "/$2/$1", array( 'unrestricted' => '$2' ) );
137 - $router->add( array( 'qwerty' => "/qwerty/$1" ), array( 'qwerty' => '$key' ) );
138 - $router->add( "/$2/$1", array( 'restricted-to-y' => '$2' ), array( '$2' => 'y' ) );
139 -
140 - foreach( array(
141 - "/Foo" => array( 'title' => "Foo" ),
142 - "/Bar" => array( 'ping' => 'pong' ),
143 - "/Baz" => array( 'marco' => 'polo' ),
144 - "/asdf-foo" => array( 'title' => "qwerty-foo" ),
145 - "/qwerty-bar" => array( 'title' => "asdf-bar" ),
146 - "/a/Foo" => array( 'title' => "Foo" ),
147 - "/asdf/Foo" => array( 'title' => "Foo" ),
148 - "/qwerty/Foo" => array( 'title' => "Foo", 'qwerty' => 'qwerty' ),
149 - "/baz/Foo" => array( 'title' => "Foo", 'unrestricted' => 'baz' ),
150 - "/y/Foo" => array( 'title' => "Foo", 'restricted-to-y' => 'y' ),
151 - ) as $path => $result ) {
152 - $this->assertEquals( $router->parse( $path ), $result );
153 - }
154 - }
155 -
156 -}
Index: trunk/phase3/includes/WebRequest.php
——@@ -93,49 +93,40 @@
9494 // Abort to keep from breaking...
9595 return $matches;
9696 }
97 -
98 - $router = new PathRouter;
99 -
10097 // Raw PATH_INFO style
101 - $router->add( "$wgScript/$1" );
 98+ $matches = self::extractTitle( $path, "$wgScript/$1" );
10299
103 - if( isset( $_SERVER['SCRIPT_NAME'] )
 100+ if( !$matches
 101+ && isset( $_SERVER['SCRIPT_NAME'] )
104102 && preg_match( '/\.php5?/', $_SERVER['SCRIPT_NAME'] ) )
105103 {
106104 # Check for SCRIPT_NAME, we handle index.php explicitly
107105 # But we do have some other .php files such as img_auth.php
108106 # Don't let root article paths clober the parsing for them
109 - $router->add( $_SERVER['SCRIPT_NAME'] . "/$1" );
 107+ $matches = self::extractTitle( $path, $_SERVER['SCRIPT_NAME'] . "/$1" );
110108 }
111109
112110 global $wgArticlePath;
113 - if( $wgArticlePath ) {
114 - $router->add( $wgArticlePath );
 111+ if( !$matches && $wgArticlePath ) {
 112+ $matches = self::extractTitle( $path, $wgArticlePath );
115113 }
116114
117115 global $wgActionPaths;
118 - if( $wgActionPaths ) {
119 - $router->add( $wgActionPaths, array( 'action' => '$key' ) );
 116+ if( !$matches && $wgActionPaths ) {
 117+ $matches = self::extractTitle( $path, $wgActionPaths, 'action' );
120118 }
121119
122120 global $wgVariantArticlePath, $wgContLang;
123 - if( $wgVariantArticlePath ) {
124 - /*$variantPaths = array();
 121+ if( !$matches && $wgVariantArticlePath ) {
 122+ $variantPaths = array();
125123 foreach( $wgContLang->getVariants() as $variant ) {
126124 $variantPaths[$variant] =
127125 str_replace( '$2', $variant, $wgVariantArticlePath );
128126 }
129 - $router->add( $variantPaths, array( 'parameter' => 'variant' ) );*/
130 - // Maybe actually this?
131 - $router->add( $wgVariantArticlePath,
132 - array( 'variant' => '$2'),
133 - array( '$2' => $wgContLang->getVariants() )
134 - );
 127+ $matches = self::extractTitle( $path, $variantPaths, 'variant' );
135128 }
136129
137 - wfRunHooks( 'WebRequestPathInfoRouter', array( $router ) );
138 -
139 - $matches = $router->parse( $path );
 130+ wfRunHooks( 'WebRequestGetPathInfoRequestURI', array( $path, &$matches ) );
140131 }
141132 } elseif ( isset( $_SERVER['ORIG_PATH_INFO'] ) && $_SERVER['ORIG_PATH_INFO'] != '' ) {
142133 // Mangled PATH_INFO
Index: trunk/phase3/includes/PathRouter.php
——@@ -1,272 +0,0 @@
2 -<?php
3 -/**
4 - * PathRouter class.
5 - * This class can take patterns such as /wiki/$1 and use them to
6 - * parse query parameters out of REQUEST_URI paths.
7 - *
8 - * $router->add( "/wiki/$1" );
9 - * - Matches /wiki/Foo style urls and extracts the title
10 - * $router->add( array( 'edit' => "/edit/$1" ), array( 'action' => '$key' ) );
11 - * - Matches /edit/Foo style urls and sets action=edit
12 - * $router->add( '/$2/$1',
13 - * array( 'variant' => '$2' ),
14 - * array( '$2' => array( 'zh-hant', 'zh-hans' )
15 - * );
16 - * - Matches /zh-hant/Foo or /zh-hans/Foo
17 - * $router->addStrict( "/foo/Bar", array( 'title' => 'Baz' ) );
18 - * - Matches /foo/Bar explicitly and uses "Baz" as the title
19 - * $router->add( '/help/$1', array( 'title' => 'Help:$1' ) );
20 - * - Matches /help/Foo with "Help:Foo" as the title
21 - * $router->add( '/help/$1', array( 'title' => 'Help:$1' ) );
22 - * - Matches
23 - * $router->add( '/$1', array( 'foo' => array( 'value' => 'bar$2' ) );
24 - * - Matches /Foo and sets 'foo=bar$2' without $2 being replaced
25 - * $router->add( '/$1', array( 'data:foo' => 'bar' ), array( 'callback' => 'functionname' ) );
26 - * - Matches /Foo, adds the key 'foo' with the value 'bar' to the data array
27 - * and calls functionname( &$matches, $data );
28 - *
29 - * Path patterns:
30 - * - Paths may contain $# patterns such as $1, $2, etc...
31 - * - $1 will match 0 or more while the rest will match 1 or more
32 - * - Unless you use addStrict "/wiki" and "/wiki/" will be expanded to "/wiki/$1"
33 - *
34 - * Params:
35 - * - In a pattern $1, $2, etc... will be replaced with the relevant contents
36 - * - If you used a keyed array as a path pattern $key will be replaced with the relevant contents
37 - * - The default behavior is equivalent to `array( 'title' => '$1' )`, if you don't want the title parameter you can explicitly use `array( 'title' => false )`
38 - * - You can specify a value that won't have replacements in it using `'foo' => array( 'value' => 'bar' );`
39 - *
40 - * Options:
41 - * - The option keys $1, $2, etc... can be specified to restrict the possible values of that variable.
42 - * A string can be used for a single value, or an array for multiple.
43 - * - When the option key 'strict' is set (Using addStrict is simpler than doing this directly)
44 - * the path won't have $1 implicitly added to it.
45 - * - The option key 'callback' can specify a callback that will be run when a path is matched.
46 - * The callback will have the arguments ( &$matches, $data ) and the matches array can be modified.
47 - *
48 - * @since 1.19
49 - * @author Daniel Friesen
50 - */
51 -class PathRouter {
52 -
53 - protected function doAdd( $path, $params, $options, $key = null ) {
54 - if ( $path[0] !== '/' ) {
55 - $path = '/' . $path;
56 - }
57 -
58 - if ( !isset( $options['strict'] ) || !$options['strict'] ) {
59 - // Unless this is a strict path make sure that the path has a $1
60 - if ( strpos( $path, '$1' ) === false ) {
61 - if ( substr( $path, -1 ) !== '/' ) {
62 - $path .= '/';
63 - }
64 - $path .= '$1';
65 - }
66 - }
67 -
68 - if ( !isset( $params['title'] ) && strpos( $path, '$1' ) !== false ) {
69 - $params['title'] = '$1';
70 - }
71 - if ( isset( $params['title'] ) && $params['title'] === false ) {
72 - unset( $params['title'] );
73 - }
74 -
75 - foreach ( $params as $paramName => $paramData ) {
76 - if ( is_string( $paramData ) ) {
77 - if ( preg_match( '/\$(\d+|key)/', $paramData ) ) {
78 - $paramArrKey = 'pattern';
79 - } else {
80 - // If there's no replacement use a value instead
81 - // of a pattern for a little more efficiency
82 - $paramArrKey = 'value';
83 - }
84 - $params[$paramName] = array(
85 - $paramArrKey => $paramData
86 - );
87 - }
88 - }
89 -
90 - foreach ( $options as $optionName => $optionData ) {
91 - if ( preg_match( '/^\$\d+$/', $optionName ) ) {
92 - if ( !is_array( $optionData ) ) {
93 - $options[$optionName] = array( $optionData );
94 - }
95 - }
96 - }
97 -
98 - $pattern = (object)array(
99 - 'path' => $path,
100 - 'params' => $params,
101 - 'options' => $options,
102 - 'key' => $key,
103 - );
104 - $pattern->weight = self::makeWeight( $pattern );
105 - $this->patterns[] = $pattern;
106 - }
107 -
108 - /**
109 - * Add a new path pattern to the path router
110 - *
111 - * @param $path The path pattern to add
112 - * @param $params The params for this path pattern
113 - * @param $options The options for this path pattern
114 - */
115 - public function add( $path, $params = array(), $options = array() ) {
116 - if ( is_array( $path ) ) {
117 - foreach ( $path as $key => $onePath ) {
118 - $this->doAdd( $onePath, $params, $options, $key );
119 - }
120 - } else {
121 - $this->doAdd( $path, $params, $options );
122 - }
123 - }
124 -
125 - /**
126 - * Add a new path pattern to the path router with the strict option on
127 - * @see self::add
128 - */
129 - public function addStrict( $path, $params = array(), $options = array() ) {
130 - $options['strict'] = true;
131 - $this->add( $path, $params, $options );
132 - }
133 -
134 - protected function sortByWeight() {
135 - $weights = array();
136 - foreach( $this->patterns as $key => $pattern ) {
137 - $weights[$key] = $pattern->weight;
138 - }
139 - array_multisort( $weights, SORT_DESC, SORT_NUMERIC, $this->patterns );
140 - }
141 -
142 - public static function makeWeight( $pattern ) {
143 - # Start with a weight of 0
144 - $weight = 0;
145 -
146 - // Explode the path to work with
147 - $path = explode( '/', $pattern->path );
148 -
149 - # For each level of the path
150 - foreach( $path as $piece ) {
151 - if ( preg_match( '/^\$(\d+|key)$/', $piece ) ) {
152 - # For a piece that is only a $1 variable add 1 points of weight
153 - $weight += 1;
154 - } elseif ( preg_match( '/\$(\d+|key)/', $piece ) ) {
155 - # For a piece that simply contains a $1 variable add 2 points of weight
156 - $weight += 2;
157 - } else {
158 - # For a solid piece add a full 3 points of weight
159 - $weight += 3;
160 - }
161 - }
162 -
163 - foreach ( $pattern->options as $key => $option ) {
164 - if ( preg_match( '/^\$\d+$/', $key ) ) {
165 - # Add 0.5 for restrictions to values
166 - # This way given two separate "/$2/$1" patterns the
167 - # one with a limited set of $2 values will dominate
168 - # the one that'll match more loosely
169 - $weight += 0.5;
170 - }
171 - }
172 -
173 - return $weight;
174 - }
175 -
176 - /**
177 - * Parse a path and return the query matches for the path
178 - *
179 - * @param $path The path to parse
180 - * @return Array The array of matches for the path
181 - */
182 - public function parse( $path ) {
183 - $this->sortByWeight();
184 -
185 - $matches = null;
186 -
187 - foreach ( $this->patterns as $pattern ) {
188 - $matches = self::extractTitle( $path, $pattern );
189 - if ( !is_null( $matches ) ) {
190 - break;
191 - }
192 - }
193 -
194 - return is_null( $matches ) ? array() : $matches;
195 - }
196 -
197 - protected static function extractTitle( $path, $pattern ) {
198 - $regexp = preg_quote( $pattern->path, '#' );
199 - $regexp = preg_replace( '#\\\\\$1#', '(?P<par1>.*)', $regexp );
200 - $regexp = preg_replace( '#\\\\\$(\d+)#', '(?P<par$1>.+?)', $regexp );
201 - $regexp = "#^{$regexp}$#";
202 -
203 - $matches = array();
204 - $data = array();
205 -
206 - if ( preg_match( $regexp, $path, $m ) ) {
207 - foreach ( $pattern->options as $key => $option ) {
208 - if ( preg_match( '/^\$\d+$/', $key ) ) {
209 - $n = intval( substr( $key, 1 ) );
210 - $value = $m["par{$n}"];
211 - if ( !in_array( $value, $option ) ) {
212 - return null;
213 - }
214 - }
215 - }
216 -
217 - foreach ( $m as $matchKey => $matchValue ) {
218 - if ( preg_match( '/^par\d+$/', $matchKey ) ) {
219 - $n = intval( substr( $matchKey, 3 ) );
220 - $data['$'.$n] = $matchValue;
221 - }
222 - }
223 - if ( isset( $pattern->key ) ) {
224 - $data['$key'] = $pattern->key;
225 - }
226 -
227 - foreach ( $pattern->params as $paramName => $paramData ) {
228 - $value = null;
229 - if ( preg_match( '/^data:/', $paramName ) ) {
230 - $isData = true;
231 - $key = substr( $paramName, 5 );
232 - } else {
233 - $isData = false;
234 - $key = $paramName;
235 - }
236 -
237 - if ( isset( $paramData['value'] ) ) {
238 - $value = $paramData['value'];
239 - } elseif ( isset( $paramData['pattern'] ) ) {
240 - $value = $paramData['pattern'];
241 - foreach ( $m as $matchKey => $matchValue ) {
242 - if ( preg_match( '/^par\d+$/', $matchKey ) ) {
243 - $n = intval( substr( $matchKey, 3 ) );
244 - $value = str_replace( '$' . $n, $matchValue, $value );
245 - }
246 - }
247 - if ( isset( $pattern->key ) ) {
248 - $value = str_replace( '$key', $pattern->key, $value );
249 - }
250 - if ( preg_match( '/\$(\d+|key)/', $value ) ) {
251 - // Still contains $# or $key patterns after replacement
252 - // Seams like we don't have all the data, abort
253 - return null;
254 - }
255 - }
256 -
257 - if ( $isData ) {
258 - $data[$key] = $value;
259 - } else {
260 - $matches[$key] = $value;
261 - }
262 - }
263 -
264 - if ( isset( $pattern->options['callback'] ) ) {
265 - call_user_func_array( $pattern->options['callback'], array( &$matches, $data ) );
266 - }
267 - } else {
268 - return null;
269 - }
270 - return $matches;
271 - }
272 -
273 -}
Index: trunk/phase3/includes/AutoLoader.php
——@@ -164,7 +164,6 @@
165165 'PageQueryPage' => 'includes/PageQueryPage.php',
166166 'Pager' => 'includes/Pager.php',
167167 'PasswordError' => 'includes/User.php',
168 - 'PathRouter' => 'includes/PathRouter.php',
169168 'PermissionsError' => 'includes/Exception.php',
170169 'PhpHttpRequest' => 'includes/HttpFunctions.php',
171170 'PoolCounter' => 'includes/PoolCounter.php',

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r104274Implement path routing code....dantman16:29, 26 November 2011
r104284Fix svn:eol-style native for r104274reedy17:27, 26 November 2011
r104285Followup r104274; Add '$key' to the $data array as well.dantman17:43, 26 November 2011

Comments

#Comment by Aaron Schulz (talk | contribs)   19:59, 28 November 2011

\o/...this was super annoying.

#Comment by Brion VIBBER (talk | contribs)   20:00, 28 November 2011

MAUVAIS TITRE!!!!!!! MAUUUUUVAAAAIIISSSS TTTIIIIITTTRRREEEEEEEEEE

Status & tagging log