r105919 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r105918‎ | r105919 | r105920 >
Date:19:19, 12 December 2011
Author:dantman
Status:resolved (Comments)
Tags:backcompat 
Comment:
Replace get{Local,Full,Link,Canonical}URL's $variant argument with a secondary $query argument and treat variant paths like we do action paths.
Modified paths:
  • /trunk/phase3/docs/hooks.txt (modified) (history)
  • /trunk/phase3/includes/OutputPage.php (modified) (history)
  • /trunk/phase3/includes/SkinTemplate.php (modified) (history)
  • /trunk/phase3/includes/Title.php (modified) (history)
  • /trunk/phase3/languages/Language.php (modified) (history)
  • /trunk/phase3/languages/LanguageConverter.php (modified) (history)

Diff [purge]

Index: trunk/phase3/docs/hooks.txt
——@@ -914,7 +914,6 @@
915915 $title: Title object of page
916916 $url: string value as output (out parameter, can modify)
917917 $query: query options passed to Title::getCanonicalURL()
918 -$variant: variant passed to Title::getCanonicalURL()
919918
920919 'GetDefaultSortkey': Override the default sortkey for a page.
921920 $title: Title object that we need to get a sortkey for
——@@ -924,13 +923,11 @@
925924 $title: Title object of page
926925 $url: string value as output (out parameter, can modify)
927926 $query: query options passed to Title::getFullURL()
928 -$variant: variant passed to Title::getFullURL()
929927
930928 'GetInternalURL': modify fully-qualified URLs used for squid cache purging
931929 $title: Title object of page
932930 $url: string value as output (out parameter, can modify)
933931 $query: query options passed to Title::getInternalURL()
934 -$variant: variant passed to Title::getFullURL()
935932
936933 'GetIP': modify the ip of the current user (called only once)
937934 &$ip: string holding the ip as determined so far
——@@ -949,13 +946,11 @@
950947 $title: Title object of page
951948 &$url: string value as output (out parameter, can modify)
952949 $query: query options passed to Title::getLocalURL()
953 -$variant: variant passed to Title::getLocalURL()
954950
955951 'GetLocalURL::Internal': modify local URLs to internal pages.
956952 $title: Title object of page
957953 &$url: string value as output (out parameter, can modify)
958954 $query: query options passed to Title::getLocalURL()
959 -$variant: variant passed to Title::getLocalURL()
960955
961956 'GetLocalURL::Article': modify local URLs specifically pointing to article paths
962957 without any fancy queries or variants.
Index: trunk/phase3/includes/OutputPage.php
——@@ -3009,7 +3009,7 @@
30103010 $tags[] = Html::element( 'link', array(
30113011 'rel' => 'alternate',
30123012 'hreflang' => $_v,
3013 - 'href' => $this->getTitle()->getLocalURL( '', $_v ) )
 3013+ 'href' => $this->getTitle()->getLocalURL( array( 'variant' => $_v ) ) )
30143014 );
30153015 }
30163016 } else {
Index: trunk/phase3/includes/Title.php
——@@ -1230,6 +1230,31 @@
12311231 }
12321232
12331233 /**
 1234+ * Helper to fix up the get{Local,Full,Link,Canonical}URL args
 1235+ */
 1236+ private static function fixUrlQueryArgs( $query, $query2 ) {
 1237+ if ( is_array( $query ) ) {
 1238+ $query = wfArrayToCGI( $query );
 1239+ }
 1240+ if ( $query2 ) {
 1241+ if ( is_string( $query2 ) ) {
 1242+ // $query2 is a string, we will consider this to be
 1243+ // a deprecated $variant argument and add it to the query
 1244+ $query2 = wfArrayToCGI( array( 'variant' => $query2 ) );
 1245+ } else {
 1246+ $query2 = wfArrayToCGI( $query2 );
 1247+ }
 1248+ // If we have $query content add a & to it first
 1249+ if ( $query ) {
 1250+ $query .= '&';
 1251+ }
 1252+ // Now append the queries together
 1253+ $query .= $query2;
 1254+ }
 1255+ return $query;
 1256+ }
 1257+
 1258+ /**
12341259 * Get a real URL referring to this title, with interwiki link and
12351260 * fragment
12361261 *
——@@ -1239,9 +1264,11 @@
12401265 * @param $variant String language variant of url (for sr, zh..)
12411266 * @return String the URL
12421267 */
1243 - public function getFullURL( $query = '', $variant = false ) {
 1268+ public function getFullURL( $query = '', $query2 = false ) {
 1269+ $query = self::fixUrlQueryArgs( $query, $query2 );
 1270+
12441271 # Hand off all the decisions on urls to getLocalURL
1245 - $url = $this->getLocalURL( $query, $variant );
 1272+ $url = $this->getLocalURL( $query );
12461273
12471274 # Expand the url to make it a full url. Note that getLocalURL has the
12481275 # potential to output full urls for a variety of reasons, so we use
——@@ -1251,7 +1278,7 @@
12521279 # Finally, add the fragment.
12531280 $url .= $this->getFragmentForURL();
12541281
1255 - wfRunHooks( 'GetFullURL', array( &$this, &$url, $query, $variant ) );
 1282+ wfRunHooks( 'GetFullURL', array( &$this, &$url, $query ) );
12561283 return $url;
12571284 }
12581285
——@@ -1266,13 +1293,10 @@
12671294 * @param $variant String language variant of url (for sr, zh..)
12681295 * @return String the URL
12691296 */
1270 - public function getLocalURL( $query = '', $variant = false ) {
 1297+ public function getLocalURL( $query = '', $query2 = false ) {
12711298 global $wgArticlePath, $wgScript, $wgServer, $wgRequest;
1272 - global $wgVariantArticlePath;
12731299
1274 - if ( is_array( $query ) ) {
1275 - $query = wfArrayToCGI( $query );
1276 - }
 1300+ $query = self::fixUrlQueryArgs( $query, $query2 );
12771301
12781302 $interwiki = Interwiki::fetch( $this->mInterwiki );
12791303 if ( $interwiki ) {
——@@ -1287,22 +1311,13 @@
12881312 } else {
12891313 $dbkey = wfUrlencode( $this->getPrefixedDBkey() );
12901314 if ( $query == '' ) {
1291 - if ( $variant != false && $this->getPageLanguage()->hasVariants() ) {
1292 - if ( !$wgVariantArticlePath ) {
1293 - $variantArticlePath = "$wgScript?title=$1&variant=$2"; // default
1294 - } else {
1295 - $variantArticlePath = $wgVariantArticlePath;
1296 - }
1297 - $url = str_replace( '$2', urlencode( $variant ), $variantArticlePath );
1298 - $url = str_replace( '$1', $dbkey, $url );
1299 - } else {
1300 - $url = str_replace( '$1', $dbkey, $wgArticlePath );
1301 - wfRunHooks( 'GetLocalURL::Article', array( &$this, &$url ) );
1302 - }
 1315+ $url = str_replace( '$1', $dbkey, $wgArticlePath );
 1316+ wfRunHooks( 'GetLocalURL::Article', array( &$this, &$url ) );
13031317 } else {
1304 - global $wgActionPaths;
 1318+ global $wgVariantArticlePath, $wgActionPaths;
13051319 $url = false;
13061320 $matches = array();
 1321+
13071322 if ( !empty( $wgActionPaths ) &&
13081323 preg_match( '/^(.*&|)action=([^&]*)(&(.*)|)$/', $query, $matches ) )
13091324 {
——@@ -1319,6 +1334,20 @@
13201335 }
13211336 }
13221337
 1338+ if ( $url === false &&
 1339+ $wgVariantArticlePath &&
 1340+ $this->getPageLanguage()->hasVariants() &&
 1341+ preg_match( '/^variant=([^&]*)$/', $query, $matches ) )
 1342+ {
 1343+ $variant = urldecode( $matches[1] );
 1344+ if ( $this->getPageLanguage()->hasVariant( $variant ) ) {
 1345+ // Only do the variant replacement if the given variant is a valid
 1346+ // variant for the page's language.
 1347+ $url = str_replace( '$2', urlencode( $variant ), $wgVariantArticlePath );
 1348+ $url = str_replace( '$1', $dbkey, $url );
 1349+ }
 1350+ }
 1351+
13231352 if ( $url === false ) {
13241353 if ( $query == '-' ) {
13251354 $query = '';
——@@ -1327,7 +1356,7 @@
13281357 }
13291358 }
13301359
1331 - wfRunHooks( 'GetLocalURL::Internal', array( &$this, &$url, $query, $variant ) );
 1360+ wfRunHooks( 'GetLocalURL::Internal', array( &$this, &$url, $query ) );
13321361
13331362 // @todo FIXME: This causes breakage in various places when we
13341363 // actually expected a local URL and end up with dupe prefixes.
——@@ -1335,7 +1364,7 @@
13361365 $url = $wgServer . $url;
13371366 }
13381367 }
1339 - wfRunHooks( 'GetLocalURL', array( &$this, &$url, $query, $variant ) );
 1368+ wfRunHooks( 'GetLocalURL', array( &$this, &$url, $query ) );
13401369 return $url;
13411370 }
13421371
——@@ -1356,14 +1385,14 @@
13571386 * for anonymous users).
13581387 * @return String the URL
13591388 */
1360 - public function getLinkURL( $query = array(), $variant = false ) {
 1389+ public function getLinkURL( $query = '', $query2 = false ) {
13611390 wfProfileIn( __METHOD__ );
13621391 if ( $this->isExternal() ) {
1363 - $ret = $this->getFullURL( $query );
 1392+ $ret = $this->getFullURL( $query, $query2 );
13641393 } elseif ( $this->getPrefixedText() === '' && $this->getFragment() !== '' ) {
13651394 $ret = $this->getFragmentForURL();
13661395 } else {
1367 - $ret = $this->getLocalURL( $query, $variant ) . $this->getFragmentForURL();
 1396+ $ret = $this->getLocalURL( $query, $query2 ) . $this->getFragmentForURL();
13681397 }
13691398 wfProfileOut( __METHOD__ );
13701399 return $ret;
——@@ -1376,8 +1405,8 @@
13771406 * @param $query String an optional query string
13781407 * @return String the URL
13791408 */
1380 - public function escapeLocalURL( $query = '' ) {
1381 - return htmlspecialchars( $this->getLocalURL( $query ) );
 1409+ public function escapeLocalURL( $query = '', $query2 = false ) {
 1410+ return htmlspecialchars( $this->getLocalURL( $query, $query2 ) );
13821411 }
13831412
13841413 /**
——@@ -1387,8 +1416,8 @@
13881417 * @param $query String an optional query string
13891418 * @return String the URL
13901419 */
1391 - public function escapeFullURL( $query = '' ) {
1392 - return htmlspecialchars( $this->getFullURL( $query ) );
 1420+ public function escapeFullURL( $query = '', $query2 = false ) {
 1421+ return htmlspecialchars( $this->getFullURL( $query, $query2 ) );
13931422 }
13941423
13951424 /**
——@@ -1404,11 +1433,12 @@
14051434 * @param $variant String language variant of url (for sr, zh..)
14061435 * @return String the URL
14071436 */
1408 - public function getInternalURL( $query = '', $variant = false ) {
 1437+ public function getInternalURL( $query = '', $query2 = false ) {
14091438 global $wgInternalServer, $wgServer;
 1439+ $query = self::fixUrlQueryArgs( $query, $query2 );
14101440 $server = $wgInternalServer !== false ? $wgInternalServer : $wgServer;
1411 - $url = wfExpandUrl( $server . $this->getLocalURL( $query, $variant ), PROTO_HTTP );
1412 - wfRunHooks( 'GetInternalURL', array( &$this, &$url, $query, $variant ) );
 1441+ $url = wfExpandUrl( $server . $this->getLocalURL( $query ), PROTO_HTTP );
 1442+ wfRunHooks( 'GetInternalURL', array( &$this, &$url, $query ) );
14131443 return $url;
14141444 }
14151445
——@@ -1424,9 +1454,10 @@
14251455 * @return string The URL
14261456 * @since 1.18
14271457 */
1428 - public function getCanonicalURL( $query = '', $variant = false ) {
 1458+ public function getCanonicalURL( $query = '', $query2 = false ) {
 1459+ $query = self::fixUrlQueryArgs( $query, $query2 );
14291460 $url = wfExpandUrl( $this->getLocalURL( $query, $variant ) . $this->getFragmentForURL(), PROTO_CANONICAL );
1430 - wfRunHooks( 'GetCanonicalURL', array( &$this, &$url, $query, $variant ) );
 1461+ wfRunHooks( 'GetCanonicalURL', array( &$this, &$url, $query ) );
14311462 return $url;
14321463 }
14331464
——@@ -1434,8 +1465,9 @@
14351466 * HTML-escaped version of getCanonicalURL()
14361467 * @since 1.18
14371468 */
1438 - public function escapeCanonicalURL( $query = '', $variant = false ) {
1439 - return htmlspecialchars( $this->getCanonicalURL( $query, $variant ) );
 1469+ public function escapeCanonicalURL( $query = '', $query2 = false ) {
 1470+ wfDeprecated( __METHOD__, '1.19' );
 1471+ return htmlspecialchars( $this->getCanonicalURL( $query, $query2 ) );
14401472 }
14411473
14421474 /**
Index: trunk/phase3/includes/SkinTemplate.php
——@@ -1009,7 +1009,7 @@
10101010 $content_navigation['variants'][] = array(
10111011 'class' => ( $code == $preferred ) ? 'selected' : false,
10121012 'text' => $varname,
1013 - 'href' => $title->getLocalURL( '', $code )
 1013+ 'href' => $title->getLocalURL( array( 'variant' => $code ) )
10141014 );
10151015 }
10161016 }
Index: trunk/phase3/languages/LanguageConverter.php
——@@ -189,7 +189,7 @@
190190 * @param $variant String: the variant to validate
191191 * @return Mixed: returns the variant if it is valid, null otherwise
192192 */
193 - protected function validateVariant( $variant = null ) {
 193+ public function validateVariant( $variant = null ) {
194194 if ( $variant !== null && in_array( $variant, $this->mVariants ) ) {
195195 return $variant;
196196 }
Index: trunk/phase3/languages/Language.php
——@@ -3260,6 +3260,14 @@
32613261 }
32623262
32633263 /**
 3264+ * Return the LanguageConverter used in the Language
 3265+ * @return LanguageConverter
 3266+ */
 3267+ function getConverter() {
 3268+ return $this->mConverter;
 3269+ }
 3270+
 3271+ /**
32643272 * convert text to all supported variants
32653273 *
32663274 * @param $text string
——@@ -3300,6 +3308,14 @@
33013309 }
33023310
33033311 /**
 3312+ * Check if the language has the specific variant
 3313+ * @return bool
 3314+ */
 3315+ function hasVariant( $variant ) {
 3316+ return (bool)$this->mConverter->validateVariant( $variant );
 3317+ }
 3318+
 3319+ /**
33043320 * Put custom tags (e.g. -{ }-) around math to prevent conversion
33053321 *
33063322 * @param $text string

Follow-up revisions

RevisionCommit summaryAuthorDate
r105924Followup r105919:...dantman20:03, 12 December 2011
r109161deprecated $query2 in Title.php...hashar16:42, 17 January 2012
r110270@since for r105919 (hasVariant / getConverter)hashar08:10, 30 January 2012

Comments

#Comment by Dantman (talk | contribs)   19:34, 12 December 2011

Looks like one of the deprecations that were intended for r105921 leaked through. Oh well, we're using svn, what else is new.

#Comment by Hashar (talk | contribs)   15:02, 9 January 2012

The function documentations need an update.

What is the points of query2 ? fixUrlQueryArgs() adds yet another layer of stuff that is prone to break and make the framework harder for little benefits.

Just use one $query argument that should either be a string or an array. That would make things wayyy easier to maintain in the long term.


FakeTitle methods should be updated to reflect Title changes.

#Comment by Dantman (talk | contribs)   15:16, 9 January 2012

Docs were already added in a followup.

The point of query2 was for patterns such as $title->getLocalURL( $query, array( 'variant' => 'zh-tw' ) );.

#Comment by Hashar (talk | contribs)   06:44, 10 January 2012

Well a second parameter should at least raise a development warning.

#Comment by Dantman (talk | contribs)   07:10, 10 January 2012

Huh, development warning. What are you talking about?

#Comment by Hashar (talk | contribs)   09:18, 10 January 2012

What I mean is that calling those methods with a second parameter should trigger an error. Something like: 

if( isset($query2) ) {
 wfWarn( "Second argument passed to " . __METHOD__ . " is deprecated\n");
}
#Comment by Dantman (talk | contribs)   09:39, 10 January 2012

It's not an error it's a valid pattern. The only thing we should consider an error for is the simple variant strings. 

// [...]
  function mySpecialURL( $query ) {
    // [...]    
    return $someTitle->getLocalURL( $query, array( 'variant' => 'zh-tw' ) );
  }
// [...]
#Comment by Hashar (talk | contribs)   15:56, 17 January 2012

What I meant is that I would like the second parameter (query2) to be deprecated with the release of 1.19.

So if $query2 is set to something, we should emit a deprecation warning.

#Comment by Hashar (talk | contribs)   16:43, 17 January 2012

I have added a deprecation warning with r109161.

All the code is fine. Marking resolved.

#Comment by Liangent (talk | contribs)   12:17, 29 January 2012

Language::getConverter and Language::hasVariant don't have @since tag.

btw. Why [removed: new added: resolved]?

#Comment by 😂 (talk | contribs)   20:45, 29 January 2012
  • @since tags are not worth a fixme
  • It means the status was changed from new -> resolved.
#Comment by Hashar (talk | contribs)   22:16, 29 January 2012

> Language::getConverter and Language::hasVariant don't have @since tag.

Please be bold and add them! That would save time to everyone :-)

#Comment by Liangent (talk | contribs)   03:13, 30 January 2012
  • I don't have core access so I have to FIXME a commit if I don't file a bug to propose a change.
  • Why new -> resolved instead of new -> ok? What's the real status of this commit then
#Comment by Hashar (talk | contribs)   08:10, 30 January 2012

> I don't have core access so I have to FIXME a commit if I don't file a bug to propose a change.

Ohhh embarrassing. Sorry about that Liangent :-/ Done with r110270.

#Comment by Hashar (talk | contribs)   08:14, 30 January 2012

> Why new -> resolved instead of new -> ok? What's the real status of this commit then

I did not like how this revision kept the second parameter to get.*URL() methods but did not think it was worth a fixme cause the code is fine. More like a todo.

I did fix the "issue" with r109161, so I marked this revision as resolved. That is also a good indicator that if we merge this revision, it needs to be merged together with another revision.

Anyway, resolved is more or less the same as new :D Hope it is clearer for you Liangent. If not, please ask!

Status & tagging log