r99381 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r99380‎ | r99381 | r99382 >
Date:22:50, 9 October 2011
Author:aaron
Status:ok (Comments)
Tags:
Comment:
* Deferred defineSpecialPages() to setup function as it needs to account for LocalSettings.php.
* Added FlaggedRevs:ready() function as a sanity check against too early initialization of FlaggedRevs config.
* Replaced load() exceptions with die() to avoid exceptions within exception.
* Cut down on FlaggedRevs class calls.
Modified paths:
  • /trunk/extensions/FlaggedRevs/FlaggedRevs.php (modified) (history)
  • /trunk/extensions/FlaggedRevs/dataclasses/FlaggedRevs.class.php (modified) (history)
  • /trunk/extensions/FlaggedRevs/presentation/FlaggedRevsUI.setup.php (modified) (history)

Diff [purge]

Index: trunk/extensions/FlaggedRevs/FlaggedRevs.php
——@@ -189,13 +189,10 @@
190190 FlaggedRevsUISetup::defineLogBasicDescription( $wgLogNames, $wgLogHeaders, $wgFilterLogTypes );
191191 FlaggedRevsUISetup::defineLogActionHanders( $wgLogActions, $wgLogActionsHandlers );
192192
193 -# Actually register special pages
194 -FlaggedRevsUISetup::defineSpecialPages( $wgSpecialPages, $wgSpecialPageGroups );
195 -
196193 # JS/CSS modules and message bundles used by JS scripts
197194 FlaggedRevsUISetup::defineResourceModules( $wgResourceModules );
198195
199 -# ####### EVENT-HANDLER FUNCTIONS #########
 196+# ####### HOOK CALLBACK FUNCTIONS #########
200197
201198 # ######## API ########
202199 # Add flagging data to ApiQueryRevisions
——@@ -273,13 +270,18 @@
274271 $wgHooks['UserGetRights'][] = 'FlaggedRevsHooks::onUserGetRights';
275272 }
276273
277 -# ####### END HOOK TRIGGERED FUNCTIONS #########
 274+# ####### END HOOK CALLBACK FUNCTIONS #########
278275
279276 // Note: avoid calls to FlaggedRevs class here for performance
280277 function efLoadFlaggedRevs() {
 278+ # LocalSettings.php loaded, safe to load config
 279+ FlaggedRevs::ready();
 280+
281281 # Conditional autopromote groups
282282 efSetFlaggedRevsAutopromoteConfig();
283283
 284+ # Register special pages (some are conditional)
 285+ efSetFlaggedRevsSpecialPages();
284286 # Conditional API modules
285287 efSetFlaggedRevsConditionalAPIModules();
286288 # Load hooks that aren't always set
——@@ -356,6 +358,11 @@
357359 }
358360 }
359361
 362+function efSetFlaggedRevsSpecialPages() {
 363+ global $wgSpecialPages, $wgSpecialPageGroups;
 364+ FlaggedRevsUISetup::defineSpecialPages( $wgSpecialPages, $wgSpecialPageGroups );
 365+}
 366+
360367 function efSetFlaggedRevsConditionalRights() {
361368 global $wgGroupPermissions, $wgFlaggedRevsProtection;
362369 if ( $wgFlaggedRevsProtection ) {
Index: trunk/extensions/FlaggedRevs/dataclasses/FlaggedRevs.class.php
——@@ -21,13 +21,24 @@
2222 # Autoreview config
2323 protected static $autoReviewConfig = 0;
2424
 25+ protected static $canLoad = false;
2526 protected static $loaded = false;
2627
27 - public static function load() {
 28+ /**
 29+ * Signal that LocalSettings.php is loaded
 30+ */
 31+ public static function ready() {
 32+ self::$canLoad = true;
 33+ }
 34+
 35+ protected static function load() {
2836 global $wgFlaggedRevsTags, $wgFlaggedRevTags;
2937 if ( self::$loaded ) {
3038 return true;
3139 }
 40+ if ( !self::$canLoad ) { // sanity
 41+ wfDebugDieBacktrace( 'FlaggedRevs config loaded too soon! Possibly before LocalSettings.php!' );
 42+ }
3243 self::$loaded = true;
3344 $flaggedRevsTags = null;
3445 if ( isset( $wgFlaggedRevTags ) ) {
——@@ -46,7 +57,7 @@
4758 # Sanity checks
4859 $safeTag = htmlspecialchars( $tag );
4960 if ( !preg_match( '/^[a-zA-Z]{1,20}$/', $tag ) || $safeTag !== $tag ) {
50 - throw new MWException( 'FlaggedRevs given invalid tag name!' );
 61+ die( 'FlaggedRevs given invalid tag name!' );
5162 }
5263 # Define "quality" and "pristine" reqs
5364 if ( is_array( $levels ) ) {
——@@ -73,7 +84,7 @@
7485 }
7586 # Sanity checks
7687 if ( !is_integer( $minQL ) || !is_integer( $minPL ) ) {
77 - throw new MWException( 'FlaggedRevs given invalid tag value!' );
 88+ die( 'FlaggedRevs given invalid tag value!' );
7889 }
7990 if ( $minQL > $ratingLevels ) {
8091 self::$qualityVersions = false;
——@@ -101,9 +112,9 @@
102113 global $wgFlaggedRevsNamespaces, $wgFlaggedRevsPatrolNamespaces;
103114 foreach ( $wgFlaggedRevsNamespaces as $ns ) {
104115 if ( MWNamespace::isTalk( $ns ) ) {
105 - throw new MWException( 'FlaggedRevs given talk namespace in $wgFlaggedRevsNamespaces!' );
 116+ die( 'FlaggedRevs given talk namespace in $wgFlaggedRevsNamespaces!' );
106117 } elseif ( $ns == NS_MEDIAWIKI ) {
107 - throw new MWException( 'FlaggedRevs given NS_MEDIAWIKI in $wgFlaggedRevsNamespaces!' );
 118+ die( 'FlaggedRevs given NS_MEDIAWIKI in $wgFlaggedRevsNamespaces!' );
108119 }
109120 }
110121 self::$reviewNamespaces = $wgFlaggedRevsNamespaces;
Index: trunk/extensions/FlaggedRevs/presentation/FlaggedRevsUI.setup.php
——@@ -1,6 +1,8 @@
22 <?php
33 /**
4 - * Class containing hooked functions for a FlaggedRevs environment
 4+ * Class containing UI setup functions for a FlaggedRevs environment.
 5+ * This depends on config variables in LocalSettings.php.
 6+ * Note: avoid FlaggedRevs class calls here for performance (like load.php).
57 */
68 class FlaggedRevsUISetup {
79 /**
——@@ -89,9 +91,10 @@
9092 * @return void
9193 */
9294 public static function defineSpecialPages( array &$pages, array &$groups ) {
93 - global $wgUseTagFilter;
 95+ global $wgFlaggedRevsProtection, $wgFlaggedRevsNamespaces, $wgUseTagFilter;
 96+
9497 // Show special pages only if FlaggedRevs is enabled on some namespaces
95 - if ( FlaggedRevs::getReviewNamespaces() ) {
 98+ if ( count( $wgFlaggedRevsNamespaces ) ) {
9699 $pages['RevisionReview'] = 'RevisionReview'; // unlisted
97100 $pages['ReviewedVersions'] = 'ReviewedVersions'; // unlisted
98101 $pages['PendingChanges'] = 'PendingChanges';
——@@ -101,7 +104,7 @@
102105 $pages['ProblemChanges'] = 'ProblemChanges';
103106 $groups['ProblemChanges'] = 'quality';
104107 }
105 - if ( !FlaggedRevs::useOnlyIfProtected() ) {
 108+ if ( !$wgFlaggedRevsProtection ) {
106109 $pages['ReviewedPages'] = 'ReviewedPages';
107110 $groups['ReviewedPages'] = 'quality';
108111 $pages['UnreviewedPages'] = 'UnreviewedPages';
——@@ -112,7 +115,7 @@
113116 $pages['ValidationStatistics'] = 'ValidationStatistics';
114117 $groups['ValidationStatistics'] = 'quality';
115118 // Protect levels define allowed stability settings
116 - if ( FlaggedRevs::useProtectionLevels() ) {
 119+ if ( $wgFlaggedRevsProtection ) {
117120 $pages['StablePages'] = 'StablePages';
118121 $groups['StablePages'] = 'quality';
119122 } else {

Comments

#Comment by Nikerabbit (talk | contribs)   16:40, 11 October 2011

Is die() really better?

#Comment by Aaron Schulz (talk | contribs)   21:57, 11 October 2011

Better than failing due to something without output page within the error itself (which always happened when the exception was hit).

Status & tagging log