r78179 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r78178‎ | r78179 | r78180 >
Date:13:18, 10 December 2010
Author:nikerabbit
Status:resolved (Comments)
Tags:
Comment:
Fix regression in r70657. Misplaced else condition was causing cache misses
to try to load the message text and cache the result individually. However
in the default configuration cache miss always means that the message doesn't
exists in the database.
Modified paths:
  • /trunk/phase3/includes/MessageCache.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/MessageCache.php
@@ -644,23 +644,22 @@
645645 return false;
646646 } elseif( $entry === '!TOO BIG' ) {
647647 // Fall through and try invididual message cache below
 648+ }
 649+ } else {
 650+ // XXX: This is not cached in process cache, should it?
 651+ $message = false;
 652+ wfRunHooks('MessagesPreLoad', array( $title, &$message ) );
 653+ if ( $message !== false ) {
 654+ return $message;
 655+ }
648656
649 - } else {
650 - // XXX: This is not cached in process cache, should it?
651 - $message = false;
652 - wfRunHooks('MessagesPreLoad', array( $title, &$message ) );
653 - if ( $message !== false ) {
654 - return $message;
655 - }
656 -
657 - /* If message cache is in normal mode, it is guaranteed
658 - * (except bugs) that there is always entry (or placeholder)
659 - * in the cache if message exists. Thus we can do minor
660 - * performance improvement and return false early.
661 - */
662 - if ( !$wgAdaptiveMessageCache ) {
663 - return false;
664 - }
 657+ /* If message cache is in normal mode, it is guaranteed
 658+ * (except bugs) that there is always entry (or placeholder)
 659+ * in the cache if message exists. Thus we can do minor
 660+ * performance improvement and return false early.
 661+ */
 662+ if ( !$wgAdaptiveMessageCache ) {
 663+ return false;
665664 }
666665 }
667666

Follow-up revisions

RevisionCommit summaryAuthorDate
r79108Set $wgAdaptiveMessageCache for parserTests until NikeRabbit fixes r78179platonides15:54, 28 December 2010
r85080MFT r78108, r78179, r78344, r78347, r78350, r78365, r78380, r78425, r78539, r...demon19:09, 31 March 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r70657Allow negative caching in process cache and use it, return early in replace()...nikerabbit00:28, 8 August 2010

Comments

#Comment by Platonides (talk | contribs)   12:46, 11 December 2010

Breaks the test <ref> with custom group link from Cite extension.

(on some setups only? :s)

#Comment by Platonides (talk | contribs)   12:46, 11 December 2010

Breaks the test <ref> with custom group link from Cite extension.

(on some setups only? :s)

#Comment by Platonides (talk | contribs)   15:51, 28 December 2010

Also gives problems with BadImages check. First call to getMsgFromNamespace() returns the right (modified) message. Then it returns false.

#Comment by Nikerabbit (talk | contribs)   10:30, 7 January 2011

I have no idea what tests are doing with message cache, but I suppose the previous code only masked the problem.

#Comment by Tim Starling (talk | contribs)   02:48, 8 February 2011

Needs backporting to 1.17, regardless of whether it breaks tests.

#Comment by Catrope (talk | contribs)   21:10, 14 February 2011

Does it need to go into 1.17wmf1 as well?

#Comment by MarkAHershberger (talk | contribs)   20:37, 30 March 2011

Does this still cause problems for anyone even without r79108?

#Comment by MarkAHershberger (talk | contribs)   00:22, 31 March 2011

re Platonides comment on r79108, marking this back to new.

Status & tagging log