r70741 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r70740‎ | r70741 | r70742 >
Date:06:16, 9 August 2010
Author:jeroendedauw
Status:ok (Comments)
Tags:
Comment:
Added ExtensionTypes hook that can be called from static context and deprecated the old one.
Modified paths:
  • /trunk/phase3/includes/specials/SpecialVersion.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/specials/SpecialVersion.php
——@@ -210,13 +210,17 @@
211211 * @return array
212212 */
213213 public static function getExtensionTypes() {
214 - return array(
 214+ $extensionTypes = array(
215215 'specialpage' => wfMsg( 'version-specialpages' ),
216216 'parserhook' => wfMsg( 'version-parserhooks' ),
217217 'variable' => wfMsg( 'version-variables' ),
218218 'media' => wfMsg( 'version-mediahandlers' ),
219219 'other' => wfMsg( 'version-other' ),
220220 );
 221+
 222+ wfRunHooks( 'ExtensionTypes', array( &$extensionTypes ) );
 223+
 224+ return $extensionTypes;
221225 }
222226
223227 /**
——@@ -233,6 +237,9 @@
234238
235239 $extensionTypes = self::getExtensionTypes();
236240
 241+ /**
 242+ * @deprecated as of 1.17, use hook ExtensionTypes instead.
 243+ */
237244 wfRunHooks( 'SpecialVersionExtensionTypes', array( &$this, &$extensionTypes ) );
238245
239246 $out = Xml::element( 'h2', array( 'id' => 'mw-version-ext' ), wfMsg( 'version-extensions' ) ) .

Follow-up revisions

RevisionCommit summaryAuthorDate
r70742Follow up to r70741jeroendedauw06:16, 9 August 2010
r70743Follow up to r70741jeroendedauw06:25, 9 August 2010

Comments

#Comment by 😂 (talk | contribs)   17:19, 29 October 2010

Why not just move the hook, don't see a need to deprecate it necessarily.

#Comment by Jeroen De Dauw (talk | contribs)   17:26, 29 October 2010

Because the new one has different bevahiour then the old one, to allow using it at places different then Special:Version. An example of this is Special:Extensions on which I was working.

#Comment by 😂 (talk | contribs)   16:32, 18 January 2011

No need to break back-compat here for 0 functional change.

And if you really want to put it in a static, pass SpecialPage::getTitleFor( 'Version' )

#Comment by Jeroen De Dauw (talk | contribs)   17:01, 18 January 2011

Its does not break bc, and the change is functional...

#Comment by 😂 (talk | contribs)   17:07, 18 January 2011

The point is you're requiring a user to use two hooks instead of just one. That's breaking current behavior (back-compat wasn't the correct word, you're right). Passing the special page is trivial, and you're free to ignore it if you don't want to use it.

You're basically changing it because you don't like the hook name.

#Comment by Jeroen De Dauw (talk | contribs)   17:14, 18 January 2011

Ah, thanks for clarifying to me why I'm doing things!

I made the change because it does not make a lot of sense to go parse a special page class just to access this code. It might be trivial resource wise, but it's bad design. Please point me to where my change is causing problems. Believe me or not, I did consider the impact of the change before making it, and had a look at affected code.

#Comment by 😂 (talk | contribs)   17:32, 18 January 2011

To be fair, only two extensions in SVN (and 4 listed on mw.org, all old) even use the hook, so nevermind about breaking stuff.

You could probably just remove SpecialVersionExtensionTypes entirely in that case.

I also agree with Ilmari below \/ \/

#Comment by Ilmari Karonen (talk | contribs)   17:20, 18 January 2011

The whole design is sort of broken, in the sense that it assumes that nothing but Special:Version cares about the extension types. Rather than just making the hook slightly more general, I'd suggest revisiting the underlying design. For example, all the current hook provides is a way to map new extension types into interface messages. If we instead renamed the interface messages consistently, e.g. so that the extension type "foo" would be automatically mapped to the message "extension-type-foo", then the whole hook would become completely unnecessary. (Of course, we could still retain it for backwards compatibility, at least for a while.)

#Comment by Jeroen De Dauw (talk | contribs)   17:25, 18 January 2011

I very much agree with this. If no one cares to do this, I'm willing to give it a go sometime soonish :)

#Comment by 😂 (talk | contribs)   18:10, 7 February 2011

This is fine. We agreed on some cleanups above, but this doesn't block deployment.

Status & tagging log