[wp-trac] Re: [WordPress Trac] #5768: Warning: closedir(): supplied argument is not a valid Directory resource in /wp-includes/theme.php on line 166

[WordPress Trac]

#5768: Warning: closedir(): supplied argument is not a valid Directory resource in
/wp-includes/theme.php on line 166
---------------------------------------------------------------+------------
 Reporter:  dustinsilva                                        |        Owner:  anonymous
     Type:  defect                                             |       Status:  reopened 
 Priority:  normal                                             |    Milestone:  2.6      
Component:  Optimization                                       |      Version:  2.3.2    
 Severity:  blocker                                            |   Resolution:           
 Keywords:  closedir, warning, not valid directory, theme.php  |  
---------------------------------------------------------------+------------
Changes (by DD32):

  * status:  closed => reopened
  * resolution:  fixed =>

Comment:

 Ignore that "Shouldnt've the change been applied here:" line, I mis-read
 the source code.

 But i should say i'm rather confused by the source-code. (And have made
 some points somewhere throughout this ramble, So just loook at the patch,
 and then here for why i've changed what i have)

 Line 135: The directory is attempted to be opened.
 {{{
         $themes_dir = @ opendir($theme_root);
 }}}
 Line 136-7: If the directory cannot be opened, $themes_dir = false,
 function should return.
 {{{
         if ( !$themes_dir )
                 return false;
 }}}

 Line 139-177: Loops through the $themes_dir folder, Sets $theme_dir to
 False when its finished.

 Line 178: WordPress attempts to close "$theme_dir", $theme_dir is now set
 to false, As it was the used in the 139-177 loop for the filename.

 Its this last line which had me confused.

 is_dir($theme_dir) should NEVER be true, as when the loop above it
 finishes, it MUST be false.

 I think line 178 was supposed to close the folder handle for that folder,
 And if we look back to line 135, thats ''$theme'''s'''_dir'', While this
 code attempts to close ''$theme_dir'';

 So, Line 178 should read:
 {{{
         @closedir( $themes_dir );
 }}}

 Correct, or have i missed something?


 Also, If we move furthur onto line 180-181:
 {{{
         if ( !$themes_dir || !$theme_files )
                 return $themes;
 }}}

 Previously ''$themes_dir'' was true as it was a open file handle.. And
 it'll never be false, as up on line 136, it returns if the themes_dir is
 false, and its not modified by the main loop at all. (And with the change
 given, it'll be false as it'll be a closed file handle)





 '''The CVS Stuff:'''

 Probably should've mentioned that in a different ticket, But it was a view
 on the code in the same area. I mentioned it as 'CVS' is the folder which
 the CVS setup uses to store info in, Whilst Subversion uses '.svn'
 instead.

 If we look at line 158-160: (note: Also 139-141 gets the same treatment)
 {{{
 if ( is_dir( $subdir . '/' . $theme_dir) && is_readable($subdir . '/' .
 $theme_dir) ) {
     if ( $theme_dir{0} == '.' )
         continue;
 }}}

 {{{$theme_dir{0} == '.'}}} will match '.', '..', '.svn' and anything else
 hidden. And its also checks that *after* its checked its a directory &
 readable, both checks which can be skipped by moving the code up a line.

 CVS: I can see that it might be a issue by removing that however now that
 i've written this load of BS out, as those who check their themes in/out
 of a CVS branch might hit issues..

 I've attached a patch with the changes (And i'll add one which ignores CVS
 directories too) - I apologise for the long writeup on the code, It was
 needed for me to understand the entire lot, as well as explaining where my
 mind was going with the code.

-- 
Ticket URL: <http://trac.wordpress.org/ticket/5768#comment:3>
WordPress Trac <http://trac.wordpress.org/>
WordPress blogging software