r77623 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r77622‎ | r77623 | r77624 >
Date:00:21, 3 December 2010
Author:platonides
Status:ok (Comments)
Tags:
Comment:
Make $wgForceUIMsgAsContentMsg work a bit different for the messages we place in the upload form.
This is specially intended for commons, such that it no longer needs a bot changing the headers after each upload.

See http://commons.wikimedia.org/w/index.php?title=Commons%3AVillage_pump&amp;action=historysubmit&amp;diff=46619490&amp;oldid=46619456#Uploading_a_new_file.2C_and_proceeding_updates
Modified paths:
  • /trunk/phase3/includes/specials/SpecialUpload.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/specials/SpecialUpload.php
——@@ -486,21 +486,35 @@
487487 * Get the initial image page text based on a comment and optional file status information
488488 */
489489 public static function getInitialPageText( $comment = '', $license = '', $copyStatus = '', $source = '' ) {
490 - global $wgUseCopyrightUpload;
 490+ global $wgUseCopyrightUpload, $wgForceUIMsgAsContentMsg;
 491+ $wgForceUIMsgAsContentMsg = (array) $wgForceUIMsgAsContentMsg;
 492+
 493+ /* These messages are transcluded into the actual text of the description page.
 494+ * Thus, forcing them as content messages makes the upload to produce an int: template
 495+ * instead of hardcoding it there in the uploader language.
 496+ */
 497+ foreach( array( 'license-header', 'filedesc', 'filestatus', 'filesource' ) as $msgName ) {
 498+ if ( in_array( $msgName, $wgForceUIMsgAsContentMsg ) ) {
 499+ $msg[$msgName] = "{{int:$msgName}}";
 500+ } else {
 501+ $msg[$msgName] = wfMsgForContent( $msgName );
 502+ }
 503+ }
 504+
491505 if ( $wgUseCopyrightUpload ) {
492506 $licensetxt = '';
493507 if ( $license != '' ) {
494 - $licensetxt = '== ' . wfMsgForContent( 'license-header' ) . " ==\n" . '{{' . $license . '}}' . "\n";
 508+ $licensetxt = '== ' . $msg[ 'license-header' ] . " ==\n" . '{{' . $license . '}}' . "\n";
495509 }
496 - $pageText = '== ' . wfMsgForContent( 'filedesc' ) . " ==\n" . $comment . "\n" .
497 - '== ' . wfMsgForContent( 'filestatus' ) . " ==\n" . $copyStatus . "\n" .
 510+ $pageText = '== ' . $msg[ 'filedesc' ] . " ==\n" . $comment . "\n" .
 511+ '== ' . $msg[ 'filestatus' ] . " ==\n" . $copyStatus . "\n" .
498512 "$licensetxt" .
499 - '== ' . wfMsgForContent( 'filesource' ) . " ==\n" . $source;
 513+ '== ' . $msg[ 'filesource' ] . " ==\n" . $source;
500514 } else {
501515 if ( $license != '' ) {
502 - $filedesc = $comment == '' ? '' : '== ' . wfMsgForContent( 'filedesc' ) . " ==\n" . $comment . "\n";
 516+ $filedesc = $comment == '' ? '' : '== ' . $msg[ 'filedesc' ] . " ==\n" . $comment . "\n";
503517 $pageText = $filedesc .
504 - '== ' . wfMsgForContent( 'license-header' ) . " ==\n" . '{{' . $license . '}}' . "\n";
 518+ '== ' . $msg[ 'license-header' ] . " ==\n" . '{{' . $license . '}}' . "\n";
505519 } else {
506520 $pageText = $comment;
507521 }

Comments

#Comment by Bryan (talk | contribs)   18:19, 3 December 2010

I think you are taking a wrong approach. Now messages behave differently only on certain pages, based on a configuration variable that is actually meant to do something else.

I would introduce a new variable, like $wgForceMsgAsIntTransclusion, but then better named and move the Template:Int: logic a level higher up, in wfMsg*

#Comment by Platonides (talk | contribs)   21:59, 3 December 2010

I considered adding a wfMsg* function for that, but didn't enjoy the idea of adding yet another wfMsg function. I didn't see a good way to add that as a parameter to wfMsgForContent() either.

Since this was the only function where it would be used, I decided on that $msg artifact. If there's some other place where are placed in wikitext, it should obviously be moved down (wfMsgForHardcoding?).

I consider that it is the right configuration variable given its function (I don't know how that variable ended with such name, which would suggest to be doing actually the opposite).

$wgForceUIMsgAsContentMsg lists message names that are usually shown in the wiki language, in order to make them show in the language of the visiting user. That's exactly the same we want in the description pages. The fact that it gets hardcoded in the wikitext, is an 'internal' detail (albeit exposed everywhere). We could be storing the license in a different db field, showing that programmatically, in which case no-one would doubt that license-header should go into $wgForceUIMsgAsContentMsg. So it fits the variable function. Translation inside wikitext is done with int:, so the issue fix is to include the messages as ⧼message⧽. I don't think that point is contended. There could be a last issue against doing it this way: arguing that the previous $wgForceUIMsgAsContentMsg behavior is desirable, but I don't think that can make sense on any setup. You either want it the same on the whole wiki (99%) or translated for every user (the current change). Shown in the language of the upload user is never right.

#Comment by Catrope (talk | contribs)   23:40, 3 December 2010

The affected message keys should at least be in a $wg var, not hardcoded like they are here.

#Comment by Platonides (talk | contribs)   22:32, 4 December 2010

Hardcoded? The array contains the messages used on this function. It was just a convenience to avoid filling the function with in_array( 'filedesc', $wgForceUIMsgAsContentMsg ) ? 'Summary' : wfMsgForContent( 'filedesc' )...

#Comment by Catrope (talk | contribs)   23:49, 4 December 2010

I missed that, ignore me.

#Comment by Happy-melon (talk | contribs)   13:46, 17 December 2010 
+		$wgForceUIMsgAsContentMsg = (array) $wgForceUIMsgAsContentMsg;

Functions at this level should not be writing to globals, period. Change the value in DefaultSettings, or add an is_array() check, rather than casting.

#Comment by Platonides (talk | contribs)   16:47, 19 December 2010

$wgForceUIMsgAsContentMsg should always be an array. It's just a bit of paranoism, as all other $wgForceUIMsgAsContentMsg references seem to check it, too.

They can probably be all removed. That extra check was added in r6588 with no apparent reason.

Status & tagging log