r78903 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r78902‎ | r78903 | r78904 >
Date:18:20, 23 December 2010
Author:soxred93
Status:reverted (Comments)
Tags:
Comment:
Per discussion on mailing list, modifying some uses of opendir()/readdir()/closedir() to use new sfFinder class.
Modified paths:
  • /trunk/phase3/includes/AutoLoader.php (modified) (history)
  • /trunk/phase3/includes/filerepo/ForeignAPIFile.php (modified) (history)
  • /trunk/phase3/includes/filerepo/LocalFile.php (modified) (history)
  • /trunk/phase3/languages/Language.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/filerepo/LocalFile.php
——@@ -599,19 +599,9 @@
600600 $dir = $this->getThumbPath();
601601
602602 if ( is_dir( $dir ) ) {
603 - $handle = opendir( $dir );
604 -
605 - if ( $handle ) {
606 - while ( false !== ( $file = readdir( $handle ) ) ) {
607 - if ( $file { 0 } != '.' ) {
608 - $files[] = $file;
609 - }
610 - }
611 -
612 - closedir( $handle );
613 - }
 603+ $files = array_map( 'basename', sfFinder::type('file')->maxdepth(0)->discard('.*')->in($dir) );
614604 }
615 -
 605+
616606 return $files;
617607 }
618608
Index: trunk/phase3/includes/filerepo/ForeignAPIFile.php
——@@ -168,16 +168,9 @@
169169 $files = array();
170170 $dir = $this->getThumbPath( $this->getName() );
171171 if ( is_dir( $dir ) ) {
172 - $handle = opendir( $dir );
173 - if ( $handle ) {
174 - while ( false !== ( $file = readdir($handle) ) ) {
175 - if ( $file{0} != '.' ) {
176 - $files[] = $file;
177 - }
178 - }
179 - closedir( $handle );
180 - }
 172+ $files = array_map( 'basename', sfFinder::type('file')->maxdepth(0)->discard('.*')->in($dir) );
181173 }
 174+
182175 return $files;
183176 }
184177
Index: trunk/phase3/includes/AutoLoader.php
——@@ -476,7 +476,10 @@
477477 # includes/libs
478478 'IEContentAnalyzer' => 'includes/libs/IEContentAnalyzer.php',
479479 'Spyc' => 'includes/libs/spyc.php',
480 -
 480+ 'sfFinder' => 'includes/libs/sfFinder.php',
 481+ 'sfGlobToRegex' => 'includes/libs/sfFinder.php',
 482+ 'sfNumberCompare' => 'includes/libs/sfFinder.php',
 483+
481484 # includes/media
482485 'BitmapHandler' => 'includes/media/Bitmap.php',
483486 'BitmapHandler_ClientOnly' => 'includes/media/Bitmap_ClientOnly.php',
Index: trunk/phase3/languages/Language.php
——@@ -476,14 +476,14 @@
477477
478478 global $IP;
479479 $names = array();
480 - $dir = opendir( "$IP/languages/messages" );
481 - while ( false !== ( $file = readdir( $dir ) ) ) {
 480+
 481+ foreach( sfFinder::type('file')->maxdepth(0)->in( "$IP/languages/messages" ) as $file ) {
482482 $code = self::getCodeFromFileName( $file, 'Messages' );
483483 if ( $code && isset( $allNames[$code] ) ) {
484484 $names[$code] = $allNames[$code];
485485 }
486486 }
487 - closedir( $dir );
 487+
488488 return $names;
489489 }
490490

Follow-up revisions

RevisionCommit summaryAuthorDate
r78904Followup to r78903: Forgot to svn addsoxred9318:22, 23 December 2010
r78905Followup to r78903: Add SVN propssoxred9318:24, 23 December 2010
r78906Followup to r78903: Add proper SVN infosoxred9318:28, 23 December 2010
r78928Revert r78903, r78904, r78905, r78906 (changing opendir() to use sfFinder cla...demon21:49, 23 December 2010

Comments

#Comment by 😂 (talk | contribs)   18:22, 23 December 2010

Forgot the svn add?

#Comment by Bryan (talk | contribs)   18:56, 23 December 2010

I thought consensus was that this was a bad idea.

#Comment by Simetrical (talk | contribs)   18:58, 23 December 2010

That was my impression too. opendir()/readdir()/closedir() are built-in PHP functions that you can look up in manual pages, and they also match the standard Unix API that's been around for decades. And this only saves a few lines of code per invocation. It makes things harder to understand, not easier.

#Comment by 😂 (talk | contribs)   18:58, 23 December 2010

Brion said "Tell you what -- 10 uses of opendir shouldnt take too long to replace; write the code and let's see how it actually looks."

I took that to mean write a patch, but I guess not :)

#Comment by Nikerabbit (talk | contribs)   19:02, 23 December 2010

FWIW I don't think this is necessarily bad thing, and certainly doesn't make the code harder to understand, but those few lines should be made shorter.

#Comment by Ilmari Karonen (talk | contribs)   19:47, 23 December 2010

I have to agree with Simetrical... this just doesn't seem to simplify the code much. Arguably, it doesn't simplify or shorten Language.php at all, and the improvement to filerepo is pretty minor compared to the amount of extra code pulled in. In fact, I could argue that the original version, while more verbose, was actually more readable than the new, especially to someone not already familiar with the sfFinder interface.

(Also, the duplicate getThumbnails() methods in LocalFile.php and ForeignAPIFile.php should really be merged, perhaps into a shared base class. That would eliminate almost as much code as this change did, without having to pull in a whole new external class.)

Status & tagging log