r87208 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r87207‎ | r87208 | r87209 >
Date:19:20, 1 May 2011
Author:krinkle
Status:ok (Comments)
Tags:
Comment:
(bug 27916) Don't prefix wgServer in mw.util.wikiGetlink

The check is not needed, if wgArticlePath is undefined there's probably a much bigger issue.

The wgServer doesn't need to be prefixed to the link (if someone really needs it (ie. external tools using the script and somehow defining wgServer in mw.config) they can surely prefix wgServer themselfs.

In most if not all cases this is used to create anchor tags, which are interprated by browsers relatively to the current window location and in some browsers it even auto-prefixes the current environment when setting the href/src attribute.

I did a grep search for "wikiGetlink" in /trunk (including ./extensions), there was one usage that assumed the wgServer prefix. Turned out that was actually a workaround untill this bug was fixed. I remove that workaround ( in mw.util.isMainPage).
Modified paths:
  • /trunk/phase3/languages/Language.php (modified) (history)
  • /trunk/phase3/resources/mediawiki.util/mediawiki.util.js (modified) (history)

Diff [purge]

Index: trunk/phase3/languages/Language.php
——@@ -2714,7 +2714,7 @@
27152715
27162716 /**
27172717 * Checks that convertPlural was given an array and pads it to requested
2718 - * amound of forms by copying the last one.
 2718+ * amount of forms by copying the last one.
27192719 *
27202720 * @param $count Integer: How many forms should there be at least
27212721 * @param $forms Array of forms given to convertPlural
Index: trunk/phase3/resources/mediawiki.util/mediawiki.util.js
——@@ -201,12 +201,7 @@
202202 */
203203 'wikiGetlink' : function( str ) {
204204
205 - // Exist check is needed since replace() can only be called on a string
206 - if ( mw.config.exists( ['wgServer', 'wgArticlePath'] ) ) {
207 - return mw.config.get( 'wgServer' ) + mw.config.get( 'wgArticlePath' ).replace( '$1', this.wikiUrlencode( str ) );
208 - } else {
209 - return false;
210 - }
 205+ return mw.config.get( 'wgArticlePath' ).replace( '$1', this.wikiUrlencode( str ) );
211206 },
212207
213208 /**
——@@ -296,7 +291,7 @@
297292
298293 // Also check for the title in related namespaces ?
299294 if ( typeof alsoRelated !== 'undefined' && alsoRelated === true ) {
300 - var tabLink = mw.config.get( 'wgServer' ) + $( '#ca-talk' ).prev().find( 'a:first' ).attr( 'href' );
 295+ var tabLink = $( '#ca-talk' ).prev().find( 'a:first' ).attr( 'href' );
301296 isRelatedToMainpage = tabLink === mw.util.wikiGetlink( mw.config.get( 'wgMainPageTitle' ) );
302297
303298 return isRelatedToMainpage || isTheMainPage;

Follow-up revisions

RevisionCommit summaryAuthorDate
r88395* Remove commented-out debug call (FU r88143)...krinkle22:10, 18 May 2011

Comments

#Comment by Krinkle (talk | contribs)   19:21, 1 May 2011

Also fixed a typo in a function comment in Language.php.

#Comment by He7d3r (talk | contribs)   15:27, 18 May 2011

After this change, the function documentation is misleading: 

		/**
		 * Get the full URL to a page name
		 *
		 * @param str Page name to link to
		 * @return Full URL for page with name of 'str' or false on error
		 */

The return value ins't a full URL anymore. Maybe the function should be renamed to make this clear, e.g. wikiGetRelativelink or something like that...

Status & tagging log