r112300 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r112299‎ | r112300 | r112301 >
Date:05:14, 24 February 2012
Author:tstarling
Status:ok (Comments)
Tags:payments-upgrade-1.19 
Comment:
Fixes for r94907/r94908: change in fallback behaviour allowing infinite loops. Tested SecurePoll and LandingCheck, not DonationInterface.
Modified paths:
  • /trunk/extensions/DonationInterface/globalcollect_gateway/globalcollect.adapter.php (modified) (history)
  • /trunk/extensions/LandingCheck/SpecialLandingCheck.php (modified) (history)
  • /trunk/extensions/SecurePoll/includes/pages/Page.php (modified) (history)

Diff [purge]

Index: trunk/extensions/DonationInterface/globalcollect_gateway/globalcollect.adapter.php
——@@ -1694,12 +1694,14 @@
16951695
16961696 switch ( $type ) {
16971697 case 'request':
1698 - $count = 0;
1699 - //Count's just there making sure we don't get stuck here.
1700 - while ( !in_array( $language, $this->getAvailableLanguages() ) && $count < 3 ){
1701 - // Get the fallback language
1702 - $language = Language::getFallbackFor( $language );
1703 - $count += 1;
 1698+ if ( !in_array( $language, $this->getAvailableLanguages() ) ) {
 1699+ $fallbacks = Language::getFallbacksFor( $language );
 1700+ foreach ( $fallbacks as $fallback ) {
 1701+ if ( in_array( $fallback, $this->getAvailableLanguages() ) ) {
 1702+ $language = $fallback;
 1703+ break;
 1704+ }
 1705+ }
17041706 }
17051707
17061708 if ( !in_array( $language, $this->getAvailableLanguages() ) ){
Index: trunk/extensions/SecurePoll/includes/pages/Page.php
——@@ -38,16 +38,14 @@
3939 }
4040 $wgLang = Language::factory( $userLang );
4141
42 - $languages = array( $userLang );
43 - $fallback = $userLang;
44 - while ( $fallback = Language::getFallbackFor( $fallback ) ) {
45 - $languages[] = $fallback;
 42+ $languages = array_merge(
 43+ array( $userLang ),
 44+ Language::getFallbacksFor( $userLang ) );
 45+
 46+ if ( !in_array( $election->getLanguage(), $languages ) ) {
 47+ $languages[] = $election->getLanguage();
4648 }
47 - if ( $fallback != $election->getLanguage() ) {
48 - $fallback = $election->getLanguage();
49 - $languages[] = $fallback;
50 - }
51 - if ( $fallback != 'en' ) {
 49+ if ( !in_array( 'en', $languages ) ) {
5250 $languages[] = 'en';
5351 }
5452 $this->context->setLanguages( $languages );
Index: trunk/extensions/LandingCheck/SpecialLandingCheck.php
——@@ -178,10 +178,9 @@
179179 $landingPage . '/' . $language
180180 );
181181 // Add fallback languages
182 - $code = $language;
183 - while ( $code !== 'en' ) {
184 - $code = Language::getFallbackFor( $code );
185 - $targetTexts[] = $landingPage . '/' . $code;
 182+ $fallbacks = Language::getFallbacksFor( $language );
 183+ foreach ( $fallbacks as $fallback ) {
 184+ $targetTexts[] = $landingPage . '/' . $fallback;
186185 }
187186 }
188187

Sign-offs

UserFlagDate
Nikerabbitinspected08:47, 25 February 2012

Follow-up revisions

RevisionCommit summaryAuthorDate
r112301MFT r112300: fallback infinite loopststarling05:16, 24 February 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r94907Attempt to fix Bug 30216 - Improve language fallback loop detection....nikerabbit16:41, 18 August 2011
r94908* (bug 30217) Make pt-br a fallback of pt...nikerabbit16:43, 18 August 2011

Comments

#Comment by MZMcBride (talk | contribs)   19:25, 24 February 2012

Given that this issue caused out-of-memory errors on wikimediafoundation.org, are there associated unit tests to ensure that Language::getFallbackFor() (or Language::getFallbacksFor()) don't cause infinite loops going forward?

#Comment by Nikerabbit (talk | contribs)   19:33, 24 February 2012

And how would you test that?

You can still cause infinite loop by doing: 

$code = 'pt';
while ( $code !== 'en' ) {
    $code = Language::getFallbackFor( $code );
}

Which is also the reason getFallbackFor is deprecated. I don't think there is any non-hacky way to make that code not to go into infinite loop.

#Comment by MZMcBride (talk | contribs)   03:16, 25 February 2012

I'm not sure how you'd test it. I just found it strange that there weren't associated tests to make sure the issue doesn't re-appear. But if there isn't an easy way to test the behavior, then the lack of tests makes sense.

Status & tagging log