r84996 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r84995‎ | r84996 | r84997 >
Date:01:49, 30 March 2011
Author:nelson
Status:ok (Comments)
Tags:todo 
Comment:
missing closedir() and ... protected the readdir against opendir() failing
Modified paths:
  • /trunk/phase3/includes/filerepo/FSRepo.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/filerepo/FSRepo.php
——@@ -622,8 +622,11 @@
623623 continue;
624624 }
625625 $dir = opendir( $path );
626 - while ( false !== ( $name = readdir( $dir ) ) ) {
627 - call_user_func( $callback, $path . '/' . $name );
 626+ if ($dir) {
 627+ while ( false !== ( $name = readdir( $dir ) ) ) {
 628+ call_user_func( $callback, $path . '/' . $name );
 629+ }
 630+ closedir( $dir );
628631 }
629632 }
630633 }

Comments

#Comment by Brion VIBBER (talk | contribs)   00:09, 22 June 2011

In the preceding line we're already checking that the path exists and is a directory; if the opendir() then fails this probably indicates a file permissions error or similar problem which probably ought to be reported rather than covered over.

The opendir() call failing will already probably spew a PHP warning to error log (or display if enabled), so this shouldn't make the situation any worse (but will make the output a bit less verbose, which is probably nice) -- so marking as ok but adding a todo flag in case someone wants to clean it up.

Similarly, if the path exists but isn't a directory we should probably record or report this, as that'll cause some funkiness.

Status & tagging log