r94373 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r94372‎ | r94373 | r94374 >
Date:19:23, 12 August 2011
Author:dantman
Status:ok (Comments)
Tags:
Comment:
Improve the ability for extensions to participate in how MediaWiki handles url paths:
- Allow extensions to hook into WebRequest::getPathInfo and add to or alter the way titles are extracted from paths
- Add a $variant argument to the GetLocalURL hook; It's always had $query, but never had $variant. As a result extensions using GetLocalURL never new if getLocalURL and have the possibility of trying to change the url in cases where they shouldn't and as a result breaking links on wiki with language variants.
- Add GetLocalURL::Internal hook for non-interwiki links. These kinds of links internally use a ugly hack for action=render and an extension using GetLocalURL can be buggy in render mode if they don't re-implement the same ugly hack that MW does. This ::Internal hook runs before the hack does so extension authors don't need to be exposed to our ugly hacky code.
- Add GetLocalURL::Article hook specifically for url tweaks to pretty urls (ie: Only when we would apply $wgArticlePath); This hook avoids the need for extensions that only want to tweak pretty url output. This hook avoids the need to make a bunch of tests for things like !$title->isExternal(), $query == '', and $variant === false which getLocalURL does and could potentially change in the future making wider GetLocalURL hooks change in function requiring extension updates.
Modified paths:
  • /trunk/phase3/RELEASE-NOTES-1.19 (modified) (history)
  • /trunk/phase3/docs/hooks.txt (modified) (history)
  • /trunk/phase3/includes/Title.php (modified) (history)
  • /trunk/phase3/includes/WebRequest.php (modified) (history)

Diff [purge]

Index: trunk/phase3/docs/hooks.txt
@@ -903,11 +903,28 @@
904904 indexed by page_id.
905905 &$colours: (output) array of CSS classes, indexed by prefixed DB keys
906906
907 -'GetLocalURL': modify local URLs as output into page links
 907+'GetLocalURL': modify local URLs as output into page links. Note that if you
 908+ are working with internal urls (non-interwiki) then it may be preferable
 909+ to work with the GetLocalURL::Internal or GetLocalURL::Article hooks as
 910+ GetLocalURL can be buggy for internal urls on render if you do not
 911+ re-implement the horrible hack that Title::getLocalURL uses
 912+ in your own extension.
908913 $title: Title object of page
909 -$url: string value as output (out parameter, can modify)
 914+&$url: string value as output (out parameter, can modify)
910915 $query: query options passed to Title::getLocalURL()
 916+$variant: variant options passed to Title::getLocalURL()
911917
 918+'GetLocalURL::Internal': modify local URLs to internal pages.
 919+$title: Title object of page
 920+&$url: string value as output (out parameter, can modify)
 921+$query: query options passed to Title::getLocalURL()
 922+$variant: variant options passed to Title::getLocalURL()
 923+
 924+'GetLocalURL::Article': modify local URLs specifically pointing to article paths
 925+ without any fancy queries or variants.
 926+$title: Title object of page
 927+&$url: string value as output (out parameter, can modify)
 928+
912929 'GetMetadataVersion': modify the image metadata version currently in use. This is
913930 used when requesting image metadata from a ForiegnApiRepo. Media handlers
914931 that need to have versioned metadata should add an element to the end of
@@ -2056,6 +2073,15 @@
20572074 $redirect: whether the page is a redirect
20582075 $skin: Skin object
20592076
 2077+'WebRequestGetPathInfoRequestURI': while extracting path info from REQUEST_URI.
 2078+ Allows an extension to extend the extraction of titles from paths.
 2079+ Implementing hooks should follow the pattern used in core:
 2080+ * Use the `$matches = WebRequest::extractTitle` pattern
 2081+ * Ensure that you test `if ( !$matches ) {` before you try extracting a title
 2082+ from the path so that you don't override an already found match.
 2083+$path: The request path to extract a title from.
 2084+&$matches: The array to apply matches to.
 2085+
20602086 'WikiExporter::dumpStableQuery': Get the SELECT query for "stable" revisions
20612087 dumps
20622088 One, and only one hook should set this, and return false.
Index: trunk/phase3/RELEASE-NOTES-1.19
@@ -28,6 +28,9 @@
2929 as with templates.
3030 * Installer now issues a warning if mod_security is present.
3131 * (bug 29455) Add support for a filter callback function in jQuery byteLimit plugin.
 32+* Extensions can now participate in the extraction of titles from url paths
 33+* Added two new GetLocalURL hooks to better serve extensions working on a limited
 34+ type of titles.
3235
3336 === Bug fixes in 1.19 ===
3437 * $wgUploadNavigationUrl should be used for file redlinks if
Index: trunk/phase3/includes/WebRequest.php
@@ -109,6 +109,8 @@
110110 }
111111 $matches = self::extractTitle( $path, $variantPaths, 'variant' );
112112 }
 113+
 114+ wfRunHooks( 'WebRequestGetPathInfoRequestURI', array( $path, &$matches ) );
113115 }
114116 } elseif ( isset( $_SERVER['ORIG_PATH_INFO'] ) && $_SERVER['ORIG_PATH_INFO'] != '' ) {
115117 // Mangled PATH_INFO
@@ -192,7 +194,7 @@
193195 }
194196
195197 /**
196 - * Internal URL rewriting function; tries to extract page title and,
 198+ * URL rewriting function; tries to extract page title and,
197199 * optionally, one other fixed parameter value from a URL path.
198200 *
199201 * @param $path string: the URL path given from the client
@@ -201,7 +203,7 @@
202204 * passed on as the value of this URL parameter
203205 * @return array of URL variables to interpolate; empty if no match
204206 */
205 - private static function extractTitle( $path, $bases, $key = false ) {
 207+ static function extractTitle( $path, $bases, $key = false ) {
206208 foreach( (array)$bases as $keyValue => $base ) {
207209 // Find the part after $wgArticlePath
208210 $base = str_replace( '$1', '', $base );
Index: trunk/phase3/includes/Title.php
@@ -909,6 +909,7 @@
910910 $url = str_replace( '$1', $dbkey, $url );
911911 } else {
912912 $url = str_replace( '$1', $dbkey, $wgArticlePath );
 913+ wfRunHooks( 'GetLocalURL::Article', array( &$this, &$url ) );
913914 }
914915 } else {
915916 global $wgActionPaths;
@@ -937,6 +938,8 @@
938939 $url = "{$wgScript}?title={$dbkey}&{$query}";
939940 }
940941 }
 942+
 943+ wfRunHooks( 'GetLocalURL::Internal', array( &$this, &$url, $query, $variant ) );
941944
942945 // @todo FIXME: This causes breakage in various places when we
943946 // actually expected a local URL and end up with dupe prefixes.
@@ -944,7 +947,7 @@
945948 $url = $wgServer . $url;
946949 }
947950 }
948 - wfRunHooks( 'GetLocalURL', array( &$this, &$url, $query ) );
 951+ wfRunHooks( 'GetLocalURL', array( &$this, &$url, $query, $variant ) );
949952 return $url;
950953 }
951954

Comments

#Comment by Nikerabbit (talk | contribs)   15:46, 13 August 2011

Yay, I think I can now simplify my hook which disables pretty paths if there is ? & or // in the url.

Documentation for GetLocalURL::Internal doesn't mention & for $this... is it needed at all?

#Comment by Dantman (talk | contribs)   15:55, 13 August 2011

The & is really only there for consistency. It practically has no meaning, but since GetLocalURL already has it there, I just kept the pattern instead of messing with things.

Status & tagging log