r84555 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r84554‎ | r84555 | r84556 >
Date:20:44, 22 March 2011
Author:ashley
Status:ok (Comments)
Tags:
Comment:
findhooks.php: also check for Hooks::run, not just wfRunHooks. Hooks class has existed since r80435 but IIRC nothing in core calls it (yet). Also added some braces and tweaked docs.
Modified paths:
  • /trunk/phase3/maintenance/findhooks.php (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/findhooks.php
——@@ -39,8 +39,8 @@
4040 class FindHooks extends Maintenance {
4141 public function __construct() {
4242 parent::__construct();
43 - $this->mDescription = "Find hooks that are undocumented, missing, or just plain wrong";
44 - $this->addOption( 'online', 'Check against mediawiki.org hook documentation' );
 43+ $this->mDescription = 'Find hooks that are undocumented, missing, or just plain wrong';
 44+ $this->addOption( 'online', 'Check against MediaWiki.org hook documentation' );
4545 }
4646
4747 public function getDbType() {
——@@ -91,11 +91,13 @@
9292 $this->printArray( 'Unclear hook calls', $bad );
9393
9494 if ( count( $todo ) == 0 && count( $deprecated ) == 0 && count( $bad ) == 0 )
 95+ {
9596 $this->output( "Looks good!\n" );
 97+ }
9698 }
9799
98100 /**
99 - * Get the hook documentation, either locally or from mediawiki.org
 101+ * Get the hook documentation, either locally or from MediaWiki.org
100102 * @return array of documented hooks
101103 */
102104 private function getHooksFromDoc( $doc ) {
——@@ -139,7 +141,7 @@
140142 private function getHooksFromFile( $file ) {
141143 $content = file_get_contents( $file );
142144 $m = array();
143 - preg_match_all( '/wfRunHooks\(\s*([\'"])(.*?)\1/', $content, $m );
 145+ preg_match_all( '/(wfRunHooks|Hooks\:\:run)\(\s*([\'"])(.*?)\1/', $content, $m );
144146 return $m[2];
145147 }
146148
——@@ -201,15 +203,19 @@
202204
203205 /**
204206 * Nicely output the array
205 - * @param $msg A message to show before the value
206 - * @param $arr An array
207 - * @param $sort Boolean : wheter to sort the array (Default: true)
 207+ * @param $msg String: a message to show before the value
 208+ * @param $arr Array: an array
 209+ * @param $sort Boolean: whether to sort the array (Default: true)
208210 */
209211 private function printArray( $msg, $arr, $sort = true ) {
210 - if ( $sort ) asort( $arr );
211 - foreach ( $arr as $v ) $this->output( "$msg: $v\n" );
 212+ if ( $sort ) {
 213+ asort( $arr );
 214+ }
 215+ foreach ( $arr as $v ) {
 216+ $this->output( "$msg: $v\n" );
 217+ }
212218 }
213219 }
214220
215 -$maintClass = "FindHooks";
 221+$maintClass = 'FindHooks';
216222 require_once( RUN_MAINTENANCE_IF_MAIN );

Follow-up revisions

RevisionCommit summaryAuthorDate
r89731* fixed regex to work...ialex17:43, 8 June 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r80435Add new Hooks class, because $wgHooks globals are evil. ...soxred9303:35, 17 January 2011

Comments

#Comment by Hashar (talk | contribs)   20:50, 22 March 2011

tagged myself as a remember to review this revision. Feel free to review it, and remove the tag 'hashar' when this revision is OK.

#Comment by IAlex (talk | contribs)   21:02, 22 March 2011 
 		if ( count( $todo ) == 0 && count( $deprecated ) == 0 && count( $bad ) == 0 )
+		{

Our convention is to put the opening brace on the same line as the condition (ref) unless it's on multiple lines (ref).

#Comment by Jack Phoenix (talk | contribs)   21:11, 22 March 2011

I didn't add the brace on the same line as the condition because the line would've been over 80 characters (it still currently is). I probably should've split it up like this: 

if (
	count( $todo ) == 0 &&
	count( $deprecated ) == 0 &&
	count( $bad ) == 0
)
{
...
}

Status & tagging log