r64178 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r64177‎ | r64178 | r64179 >
Date:20:21, 25 March 2010
Author:happy-melon
Status:resolved (Comments)
Tags:
Comment:
Make MessageCache::get() return false if the requested message does not exist, and handle setting the '<$key>' one layer further down in wfMsgGetKey(). Allows us to get away from that horrible string comparison in wfEmptyMsg().
Modified paths:
  • /trunk/phase3/includes/GlobalFunctions.php (modified) (history)
  • /trunk/phase3/includes/MessageCache.php (modified) (history)
  • /trunk/phase3/languages/LanguageConverter.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/GlobalFunctions.php
@@ -750,7 +750,9 @@
751751 # If $wgMessageCache isn't initialised yet, try to return something sensible.
752752 if( is_object( $wgMessageCache ) ) {
753753 $message = $wgMessageCache->get( $key, $useDB, $langCode );
754 - if ( $transform ) {
 754+ if( $message === false ){
 755+ $message = '&lt;' . htmlspecialchars( $key ) . '&gt;';
 756+ } elseif ( $transform ) {
755757 $message = $wgMessageCache->transform( $message );
756758 }
757759 } else {
@@ -2267,12 +2269,12 @@
22682270 * looked up didn't exist but a XHTML string, this function checks for the
22692271 * nonexistance of messages by looking at wfMsg() output
22702272 *
2271 - * @param $msg String: the message key looked up
2272 - * @param $wfMsgOut String: the output of wfMsg*()
2273 - * @return Boolean
 2273+ * @param $key String: the message key looked up
 2274+ * @return Boolean True if the message *doesn't* exist.
22742275 */
2275 -function wfEmptyMsg( $msg, $wfMsgOut ) {
2276 - return $wfMsgOut === htmlspecialchars( "<$msg>" );
 2276+function wfEmptyMsg( $key ) {
 2277+ global $wgMessageCache;
 2278+ return $wgMessageCache->get( $key ) === false;
22772279 }
22782280
22792281 /**
Index: trunk/phase3/includes/MessageCache.php
@@ -500,7 +500,7 @@
501501
502502 if ( strval( $key ) === '' ) {
503503 # Shortcut: the empty key is always missing
504 - return '&lt;&gt;';
 504+ return false;
505505 }
506506
507507 $lang = wfGetLangObj( $langcode );
@@ -558,7 +558,7 @@
559559
560560 # Final fallback
561561 if( $message === false ) {
562 - return '&lt;' . htmlspecialchars($key) . '&gt;';
 562+ return false;
563563 }
564564
565565 # Fix whitespace
Index: trunk/phase3/languages/LanguageConverter.php
@@ -834,6 +834,11 @@
835835
836836 if ( strpos( $code, '/' ) === false ) {
837837 $txt = $wgMessageCache->get( 'Conversiontable', true, $code );
 838+ if( $txt === false ){
 839+ # FIXME: this method doesn't seem to be expecting
 840+ # this possible outcome...
 841+ $txt = '&lt;Conversiontable&gt;';
 842+ }
838843 } else {
839844 $title = Title::makeTitleSafe( NS_MEDIAWIKI,
840845 "Conversiontable/$code" );

Follow-up revisions

RevisionCommit summaryAuthorDate
r64182Work on Message.php:...happy-melon20:34, 25 March 2010
r64214Get message in wfEmptyMsg using the user's interface language; closer to the ...happy-melon11:21, 26 March 2010
r79717wfEmptyMsg() doesn't require a second parameter anymoreialex13:07, 6 January 2011
r83462Remove second parameters from wfEmptyMsg() calls...hashar17:10, 7 March 2011

Comments

#Comment by Happy-melon (talk | contribs)   20:22, 25 March 2010

The only other place $wgMessageCache->get() is called in core directly is in LanguageConverter, where it looks like it's not going to be much happier to get '<foo>' than 'false'. All instances in /extensions are tags, random forks of /phase3, and one place in LQT where there's no chance of getting bad keys.

#Comment by Siebrand (talk | contribs)   23:56, 25 March 2010

Has unwanted side effects. See for example the page edit form. This now displays "<editpage-tos-summary>" (observed at translatewiki.net).

#Comment by Happy-melon (talk | contribs)   10:32, 26 March 2010

Hmm... I can't duplicate, although I see it on translatewiki. Will investigate...

#Comment by Siebrand (talk | contribs)   11:47, 26 March 2010

No longer observed running r64214. Thanks.

#Comment by Simetrical (talk | contribs)   22:33, 28 March 2010

This seems completely broken. You're changing the semantics of wfEmptyMsg() so that it checks whether the message from $wgMessageCache is empty, not whether the given message is empty. In particular, wfEmptyMsg() could be called on interface messages or on content messages, and you have no way to tell which is which from this code. Am I missing something?

#Comment by Happy-melon (talk | contribs)   22:49, 28 March 2010

MessageCache::get() is not "get me the message for the parameters given"; it's "get me something I can use for this message, these parameters are what I'd like ideally". It will first try a customised message in the requested language, then the uncustomised message, then the fallback language, then the site default language, then finally give up in disgust and give you the angle brackets. The old wfEmptyMsg tested whether the result you got from some wfMsg function was that final fallback; all the wfMsg* functions call wfMsgGetKey(), which calls MessageCache::get(), which runs through that full fallback chain. Essentially, you can't test for an empty message in an interface language, because if it doesn't exist it will immediately fall back to the content language.

#Comment by Simetrical (talk | contribs)   00:01, 29 March 2010

Okay, fine. This still changes two functions that are used all over the place, directly or indirectly. What's the benefit? Only "Allows us to get away from that horrible string comparison in wfEmptyMsg()"? Was there anything wrong with it other than aesthetics? It's not like anyone has to read the code for that function very much. Refactoring is good if it reduces code duplication or increases maintainability or such, but here you're adding lines of code on the net. I don't see the point.

Also, you're adding extra calls to the message cache, and a quick test shows those are something like 20us each on my machine. A thousand extra wfEmptyMsg() per page load is a noticeable amount of CPU time.

On top of all this, we'll probably be getting a new message system for 1.17, which will hopefully not have such an ugly hack as wfEmptyMsg() anywhere in it.

#Comment by Happy-melon (talk | contribs)   10:50, 29 March 2010

On top of all this, we'll probably be getting a new message system for 1.17, which will hopefully not have such an ugly hack as wfEmptyMsg() anywhere in it.

The primary purpose of the change was to allow a clean implementation of Message::exists(), which I committed in the next revision.

#Comment by Simetrical (talk | contribs)   16:35, 29 March 2010

In that case I'm not sure I have any concrete objections left, although it still leaves me uneasy.

Status & tagging log