Warning: closedir(): supplied argument is not a valid Directory resource in /wp-includes/theme.php on line 166

[dustinsilva]

Hello all! I recently upgraded to 2.3.2. I have 'warnings' enabled in apache and I get this warning while trying to change themes: 'Warning: closedir(): supplied argument is not a valid Directory resource in /wp-includes/theme.php on line 166'

Basically line 166 of file 'theme.php' tries to closedir($theme_dir); and I checked the value of $theme_dir at failure, and $theme_dir = ;

You wordpress Devs need to add 'if( is_dir($theme_dir))' to line 166, directly above @closedir(theme_dir)... incase $theme_dir isn't really a readable directory.

or do whatever you think is necessary, but this would be handy addition to this file.

Thank,
I hope this is clear.

Not sure what 'Milestone' is in ticket submission form.

[markjaquith]

(In [6744]) Make sure the theme_dir is a dir before we attempt to closedir(). fixes #5768

[DD32]

Shouldnt've the change been applied here: ​http://trac.wordpress.org/browser/trunk/wp-includes/theme.php#L135

It should fail to open the theme directory and then return false if its not a valid directory.. Perhaps a is_dir() should've been added there instead?

While i'm on that piece of code: ​http://trac.wordpress.org/browser/trunk/wp-includes/theme.php#L159

|| $theme_dir == 'CVS' )

Shouldn't that be checking for subversion stuff instead now-days?(if even needed)

[DD32]

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 $themes_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.

[DD32]

+ ignore cvs

[DD32]

Traction? Close as wontfix? Remove ancient CVS references?

[Denis-de-Bernardy]

let's close as invalid, as it's probably fixed in #7875