r109161 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r109160‎ | r109161 | r109162 >
Date:16:42, 17 January 2012
Author:hashar
Status:ok (Comments)
Tags:core 
Comment:
deprecated $query2 in Title.php

$query2 was used to pass a variant. Make that deprecated, the
recommanded way is to use an array as a first parameter. Ex:

$this->getLocalUrl( array( 'variant' => 'foo' ) );

Ping r105919
Modified paths:
  • /trunk/phase3/includes/Title.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/Title.php
@@ -1219,8 +1219,18 @@
12201220
12211221 /**
12221222 * Helper to fix up the get{Local,Full,Link,Canonical}URL args
 1223+ * get{Canonical,Full,Link,Local}URL methods accepted an optional
 1224+ * second argument named variant. This was deprecated in favor
 1225+ * of passing an array of option with a "variant" key
 1226+ * Once $query2 is removed for good, this helper can be dropped
 1227+ * andthe wfArrayToCGI moved to getLocalURL();
 1228+ *
 1229+ * @since 1.19 (r105919)
12231230 */
1224 - private static function fixUrlQueryArgs( $query, $query2 ) {
 1231+ private static function fixUrlQueryArgs( $query, $query2 = false ) {
 1232+ if( $query2 !== false ) {
 1233+ wfDeprecated( "Title::get{Canonical,Full,Link,Local} method called with a second parameter is deprecated. Add your parameter to an array passed as the first parameter. This ", "1.19" );
 1234+ }
12251235 if ( is_array( $query ) ) {
12261236 $query = wfArrayToCGI( $query );
12271237 }
@@ -1282,6 +1292,9 @@
12831293 * be an array. If a string is passed it will be interpreted as a deprecated
12841294 * variant argument and urlencoded into a variant= argument.
12851295 * This second query argument will be added to the $query
 1296+ * The second parameter is deprecated since 1.19. Pass it as a key,value
 1297+ * pair in the first parameter array instead.
 1298+ *
12861299 * @return String the URL
12871300 */
12881301 public function getLocalURL( $query = '', $query2 = false ) {

Follow-up revisions

RevisionCommit summaryAuthorDate
r110120Minor tweak to r109161, remove trailing "this"demon13:27, 27 January 2012
r110271Drop a comment. We'll never turn to variant-as-2nd-arg because of r109161.liangent08:45, 30 January 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r105919Replace get{Local,Full,Link,Canonical}URL's $variant argument with a secondar...dantman19:19, 12 December 2011

Comments

#Comment by 😂 (talk | contribs)   15:42, 20 January 2012

This doesn't accomplish anything really, fixUrlQueryArgs() is private, so nothing outside of this class can possibly be using it. Why not just fix those 4 callers rather than doing this?

#Comment by Hashar (talk | contribs)   17:48, 20 January 2012

I the idea of r105919 was to not break back compatibility. Since any call of a public getter eventually end up calling fixUrlQueryArgs() I though it would be a good it to add the deprecation notice there.

If you drop the second argument from the 4 callers, you will get PHP errors which is not really nice and my cause trouble with legacy extensions.

#Comment by Hashar (talk | contribs)   09:17, 24 January 2012

Rewriting my above comment /me needs a neurologist appointment.

The idea of r105919 was to not break back compatibility. Since a call to any of the public getter eventually end up calling fixUrlQueryArgs(), I though it would be cleaner to add the deprecation notice there.

If you drop the second argument from the 4 callers, you will get PHP errors which is not really nice and my cause trouble with legacy extensions.

#Comment by 😂 (talk | contribs)   13:27, 27 January 2012

I suppose my main point was that passing extra arguments is harmless, so why bother throwing notices at all?

#Comment by Reedy (talk | contribs)   17:35, 23 January 2012

Surely this is too early to be giving deprecated notices anyway? ie a version or 2 more needed first

#Comment by Hashar (talk | contribs)   09:19, 24 January 2012

The change is transparent to extension authors. So they will only discover the issue once the deprecation notices are emitted.

Status & tagging log