r81229 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r81228‎ | r81229 | r81230 >
Date:05:30, 31 January 2011
Author:yaron
Status:deferred (Comments)
Tags:
Comment:
Improved comments, created a version number ("1.0", for lack of a better number)
Modified paths:
  • /trunk/extensions/ConfirmEdit/ConfirmEdit.php (modified) (history)

Diff [purge]

Index: trunk/extensions/ConfirmEdit/ConfirmEdit.php
——@@ -1,11 +1,13 @@
22 <?php
33
44 /**
5 - * Experimental captcha plugin framework.
6 - * Not intended as a real production captcha system; derived classes
7 - * can extend the base to produce their fancy images in place of the
8 - * text-based test output here.
 5+ * ConfirmEdit MediaWiki extension.
96 *
 7+ * This is a framework that holds a variety of CAPTCHA tools. The
 8+ * default one, 'SimpleCaptcha', is not intended as a production-
 9+ * level CAPTCHA system, and another one of the options provided
 10+ * should be used in its place for any real usages.
 11+ *
1012 * Copyright (C) 2005-2007 Brion Vibber <brion@wikimedia.org>
1113 * http://www.mediawiki.org/
1214 *
——@@ -38,8 +40,9 @@
3941 $wgExtensionCredits['other'][] = array(
4042 'path' => __FILE__,
4143 'name' => 'ConfirmEdit',
42 - 'author' => 'Brion Vibber',
 44+ 'author' => 'Brion Vibber and others',
4345 'url' => 'http://www.mediawiki.org/wiki/Extension:ConfirmEdit',
 46+ 'version' => '1.0',
4447 'descriptionmsg' => 'captcha-desc',
4548 );
4649

Follow-up revisions

RevisionCommit summaryAuthorDate
r81239Followup r81231, r81229...reedy09:50, 31 January 2011

Comments

#Comment by Reedy (talk | contribs)   07:27, 31 January 2011

Shouldn't it be 'author' => array( 'Brion Vibber', 'others' ),

#Comment by Yaron Koren (talk | contribs)   14:40, 31 January 2011

I would say no, because it'll lead to awkward constructions like "Brion Vibber y others" (in Spanish), etc. Maybe it's better to just add in the full authors list, though.

#Comment by Platonides (talk | contribs)   18:12, 31 January 2011

'author' => array( 'Brion Vibber', wfMsg( 'others' ) ), ?

#Comment by 😂 (talk | contribs)   18:14, 31 January 2011

We should not be encouraging wfMsg() in $wgExtensionCredits.

#Comment by Reedy (talk | contribs)   18:19, 31 January 2011

What are we changing it to then?

#Comment by 😂 (talk | contribs)   18:23, 31 January 2011

Well an array with 'others' is wrong for why Yaron mentions. wfMsg() in $wgExtensionCredits is crazy and an example of i18n gone wild.

I would suggest just listing the authors and leaving the "others" out of it.

#Comment by 😂 (talk | contribs)   18:27, 31 January 2011

As a followup. Yes we use wfMsg('others') in the credits on Special:Version. The difference is that is only called in about 3 different code paths, version extension setup which is called every single execution.

Hmmm, as an afterthought (just came to me): what if 'others' was treated as wfMsg('others') when building the output in Special:Version? It'd probably be about a 5 line change.

#Comment by 😂 (talk | contribs)   18:52, 31 January 2011

A few more than 5, but here's a patch: http://dpaste.org/gcCf/

Thoughts?

#Comment by Platonides (talk | contribs)   22:29, 31 January 2011

Doesn't look bad.

It needs documentation. That makes the value 'others' a magic one.

I'm not sure about version-poweredby-others message reuse.

There's the assumption that getCreditsForExtension() will only be called after getMediaWikiCredits() but it is not protected/private and even a reorder of execute() could break it.

#Comment by 😂 (talk | contribs)   22:40, 31 January 2011

If we commit it, we can add docs :)

That message was introduced for the "by John, Jane, Joe and others" for the version credits above...the context is pretty much the same here so I didn't think it'd be a big deal.

Yeah, should probably fix that order of operation assumption.

#Comment by Brion VIBBER (talk | contribs)   01:00, 5 February 2011

I've done this in r81549, but using '...' rather than the English magic string "others" as the replacement trigger.

#Comment by Duplicatebug (talk | contribs)   09:03, 5 February 2011

There are other extensions credits with "others":

  • AddMediaWizard/AddMediaWizard.php
  • DSMW/DSMW.php
  • FCKeditor/FCKeditor.php
  • Interwiki/Interwiki.php
  • Maps/Maps.php
  • SecurePoll/SecurePoll.php
  • SemanticForms/includes/SF_GlobalFunctions.php
  • SemanticMediaWik/includes/SMW_SetupLight.php
  • SemanticMediaWik/includes/SMW_Setup.php

Please update the extensions also. Thanks.

#Comment by Platonides (talk | contribs)   23:13, 5 February 2011

Done in r81579. However Maps (using 'maps-others' msg), and those using links are left as-is.

#Comment by 😂 (talk | contribs)   23:21, 5 February 2011

I think using wfMsg() in $wgExtensionCredits in Maps is wrong and should use this core feature.

#Comment by Platonides (talk | contribs)   23:23, 5 February 2011

Yes, but he also makes a link. The ... feature does not take into account things like [http://mwextension.com/team other]

#Comment by 😂 (talk | contribs)   23:23, 5 February 2011

Yes I know. I still think wfMsg() is wrong :(

Status & tagging log