User:MaxSem/CR/ZeroRated

Reviewed as of r107135

i18n

• 'zero-rated-mobile-access-desc' => 'Zero Rated Mobile Access', - It's just extension name with spaces added, it doesn't explain what this extension does.
  • 'zero-rated-mobile-access-desc' => 'Zero Rated Mobile Access extension provides features for mobile providers that provide free access',...
• 'zero-rated-mobile-access-desc' => '{{Identical|Show}}', - Should be {{desc}}

ZeroRatedMobileAccessTemplate.php

Class-only files need no inclusion guard, I've fixed that in r107105

ZeroRatedMobileAccess.php

• DIRECTORY_SEPARATOR isn't needed as PHP automatically converts to proper slash style. The only situation when directory separator matters is when file name gets passed to an external program, which isn't the case.
  • Well, $cwd = dirname( __FILE__ ) === string(53) "/home/preilly/phase3/extensions/ZeroRatedMobileAccess"
  • and $cwd = dirname( __FILE__ ) . DIRECTORY_SEPARATOR === string(54) "/home/preilly/phase3/extensions/ZeroRatedMobileAccess/"
• It is advised to separate extension implementation from registration and initialization, suggest moving ExtZeroRatedMobileAccess class to a separate file.
  • created ZeroRatedMobileAccess.body.php
• This class is mostly static, with the only non-static function, beforePageDisplayHTML, using only static resources. Recommend making it static too and avoid initializing the class on every request at all. This will also help in accomplishing the above point.
  • This won't always be the case.
• self::$renderZeroRatedLandingPage = $wgRequest->getInt( 'renderZeroRatedLandingPage' ); Recommend using getFuzzyBool() and pass 'true' to it for coolness.
• self::$renderZeroRatedLandingPage = $wgRequest->getInt( 'renderZeroRatedLandingPage' ); Hope such long parameter name isn't going to stick:P
  • self::$renderZeroRatedLandingPage = $wgRequest->getFuzzyBool( 'renderZeroRatedLandingPage' );
  • if ( self::$renderZeroRatedLandingPage === true ) {...
  • querystring parameter is temporary
• $ip = ( $wgRequest->getVal( 'ip') ) ? $wgRequest->getVal( 'ip' ) : wfGetIP(); You can simply do $ip = $wgRequest->getVal( 'ip', wfGetIP() );
  • $ip = $wgRequest->getVal( 'ip', wfGetIP() );
• Undefined variable: lines in D:\Projects\MediaWiki\extensions\ZeroRatedMobileAccess\ZeroRatedMobileAccess.php on line 149
  • $lines = array();
• Undefined index: RUSSIAN FEDERATION in D:\Projects\MediaWiki\extensions\ZeroRatedMobileAccess\ZeroRatedMobileAccess.php on line 84
  • $languagesForCountry = ( isset( $languageOptions[self::getFullCountryNameFromCode( $country )] ) ) ? $languageOptions[self::getFullCountryNameFromCode( $country )] : null;
• $title = Title::newFromText( "Language_options", NS_MEDIAWIKI ); - this message should be prefixed with extension-specific prefix and be present in message file.
  • 'zero-rated-mobile-access-language-options-wiki-page' => 'Language_options',...
  • $languageOptionsWikiPage = wfMsg( 'zero-rated-mobile-access-language-options-wiki-page' );
  • $title = Title::newFromText( $languageOptionsWikiPage, NS_MEDIAWIKI );
• Why profile array creation in getFullCountryNameFromCode()? I've just made it static to make sure that multiple calls have a minimum overhead. Also, you need to check if country code is not a multi-megabyte string POSTed just to make strtoupper() choke twice on it:)
  • $code = ( strlen( $code ) === 2 ) ? strtoupper( $code ) : null;
  • return ( $code && isset( $countries[$code] ) ) ? $countries[$code] : null;