1
\$\begingroup\$

I've set up some PHP to delete a directory, it's contents, and any subdirectory and its contents... I'm new to PHP so I'm most definitely doing something WRONG or am doing something in the most inefficient way.

Looking for some references or suggestion on how to do this better...

Using PHP 5.3.8.

First Draft

if ($handle = opendir($main_dir)) {
    while (false !== ($entry = readdir($handle))) { 
        $absolute_path = $main_dir.'/'.$entry;
        if ($entry != "." && $entry != "..") {      
            chmod($absolute_path, 0755);
            unlink($absolute_path);

            //check if any folders exist, then delete files within
            if (file_exists($absolute_path) && is_dir($absolute_path)) {
                if ($child_handle = opendir($absolute_path)) {
                    while (false !== ($child_entry = readdir($child_handle))) {             
                    $child_absolute_path = $absolute_path.'/'.$child_entry;
                        if ($child_entry != "." && $child_entry != "..") {              
                            unlink($child_absolute_path);
                        }
                    }
                    closedir($child_handle);
                }
            }
            rmdir($absolute_path);
        }
    }
    closedir($handle);
}
rmdir($main_dir);

how about something like this...

Second Draft

 $dir = "path/to/dir";
 chmod($dir, 0755);
 $di = new RecursiveDirectoryIterator($dir, FilesystemIterator::SKIP_DOTS);
 $ri = new RecursiveIteratorIterator($di, RecursiveIteratorIterator::CHILD_FIRST);
 foreach ( $ri as $file ) {
     $file->isDir() ?  rmdir($file) : unlink($file);
 }
 rmdir($dir);

Any thoughts? Much appreciated!

\$\endgroup\$
2
  • \$\begingroup\$ What version of PHP do you use/can you use? You should tag it if possible as there is a lot of differences between 5.4 and 5.0 in regards to cleanliness of code. \$\endgroup\$
    – Steven
    Oct 19, 2012 at 2:03
  • \$\begingroup\$ @Steven Im using 5.3.8, I've updated to include. \$\endgroup\$
    – terry
    Oct 19, 2012 at 2:14

3 Answers 3

1
\$\begingroup\$
function empty_dir($dir) {
    $iterator = new RecursiveIteratorIterator(new RecursiveDirectoryIterator($dir),
                                              RecursiveIteratorIterator::CHILD_FIRST);
    foreach ($iterator as $path) {
      if ($path->isDir()) {
         rmdir($path->__toString());
      } else {
         unlink($path->__toString());
      }
    }
    rmdir($dir);
}

from: http://php.net/manual/en/class.recursivedirectoryiterator.php

\$\endgroup\$
1
  • 3
    \$\begingroup\$ why are using '$path->__toString()'? \$\endgroup\$
    – terry
    Oct 19, 2012 at 3:31
0
\$\begingroup\$

Well one way would be to use the Recursive functionality...

chmod($main_dir, 0755) // Not sure why you keep chmodding stuff
// Recursively loop through directory, starting with the children
$dir = new RecursiveIteratorIterator(new RecursiveDirectoryIterator($main_dir),RecursiveIteratorIterator::CHILD_FIRST);

foreach($dir as $file) {
    if (in_array($file->getBasename(), array('.', '..'))) {
        continue; // Skip if it is . or ..
    } elseif ($file->isDir()) {
        rmdir($file->getPathname());
    } elseif ($file->isFile() || $file->isLink()) {
        unlink($file->getPathname());
    }
}
rmdir($main_dir);

Otherwise you could look at https://stackoverflow.com/questions/1334398/how-to-delete-a-folder-with-contents-using-php

Those are both good ways to do it.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks! ps. I was using chmod cuz I read somewhere something like... 'blah, blah, blah... use chmod to make sure you can access the directory before deleting to avoid errors.' \$\endgroup\$
    – terry
    Oct 19, 2012 at 2:41
  • \$\begingroup\$ What is the purpose of using isLink() ad not just isFile()? \$\endgroup\$
    – terry
    Oct 19, 2012 at 3:15
  • \$\begingroup\$ Do you think it is necessary or is it a best practice kinda thing to include isLink() ? What I have read isLink() is only applicable to Windows > Vista. \$\endgroup\$
    – terry
    Oct 19, 2012 at 9:58
0
\$\begingroup\$

Seen as both of the other answers seem to just be giving you code without much explanation, I thought I'd try and help you understand what they are saying.

Draft 1

I notice that you have found the documentation. Its good to read and use the documentation, but sometimes the code they use really isn't of the best quality. I don't really know what to tell you to remedy this except to always look in multiple places and become familiar with PHP best practices. My first suggestion would be NOT to assign variables inside of statements. Define the variable then check it.

$handle = opendir( $main_dir );
if( $handle ) {
    //etc...
}

This is counter to how the PHP documentation says to do it, but this is considered best practice. There are a few different reasons for this. First, it becomes much more clear what is being done here because there is no more ambiguity as to whether you actually meant to assign that variable or compare it. Sometimes that is obvious from the context, but sometimes not. Best to avoid the confusion altogether. Second, there is less room for error. If you get in the habit of not assigning variables in a statement you are less likely to accidentally use an assignment when you meant to use a comparison. And third, it requires less parenthesis, which immediately makes it easier to read. Don't believe me? Take a look at that while statement in your code. That's not as bad as it could be, I have seen worse, but the nested parenthesis really does cause eye strain, so much in fact that you may have already noticed something odd about my code. My style is to add spaces between parenthesis. Don't mind this, it is merely my style and not a comment on how "best" to write it.

The only time assigning a variable in a statement is "acceptable" is when setting up an iterable in a loop.

//most common
for( $i = 0; $i < 10; $i++ ) {
//less common, but still acceptable
while( ($buffer = fgets( $handle, 4096 ) ) !== false) {

The second example gets a little iffy, but is still usually acceptable. If you were to try abstracting that variable you would immediately see how much more complex your code would get. You'd have to rewind the initial pointer, manually step through the contents with next and before long you'd realize that the first way was much simpler and much easier to read in comparison. Which is why we still do it. I challenge you to try this out for yourself though. Nothing like seeing this in action to get a full understanding of it. The first example is easier to abstract, and sometimes is, if not starting from zero, but it is also part of the syntax and is considered acceptable. In fact you might get weird looks if you tried to abstract that unnecessarily.

Yoda statements, FALSE == $var instead of $var == FALSE, become unnecessary if you follow the above advice. Yoda statements were initially intended for preventing accidental assignments, because $var = FALSE is valid while FALSE = $var in not. Getting in the habit of not assigning variables in statements means it becomes easier to spot accidental assignments. In some languages this feature has been removed for this very reason, though sadly not in PHP. However, this is a stylistic choice and is entirely up to you. I only mention this because I noticed that this too is from the documentation and I wanted to make sure you understood the reasoning for it and the alternative.

while( FALSE !== ( $entry = readdir( $handle ) ) ) {
//is the same as
while( ( $entry = readdir( $handle ) ) !== FALSE ) {

Let's take a look at the Arrow Anti-Pattern now. Anti-patterns are bad. This particular anti-pattern is easily distinguishable by heavy indentation that brings your code to points, like an arrow, or in extreme cases like a quiver full of arrows (also sometimes referred to as mountainous). Take a look at your code. It is clearly violating this anti-pattern. But how can we fix this?

The easiest way to refactor for this is to return early. This can be expressed by literally returning a value early, or by forcing our code to skip something (this is not the same as goto, never use goto). In loops we can use continue and break to manually manipulate the flow of iteration. For example, if we reverse our if statement we can force the iteration to continue, or skip over an element. This removes a level of indentation from the rest of our code in addition to making it a little more efficient.

if( $entry == '.' || $entry == '..' ) {
    continue;
}

If we found what we were looking for in a loop, we can break or return instead. If we break we only stop the loop and execution continues as if the loop had finished. From this point the last element we looped over is still available, though this could be deceiving because its not always known whether the element we are looking at is the one we want or simply the last element in the array. This is where a return would become useful. If we return from a function, execution stops and returns a value to whichever script called it. If we return from your code execution will just end.

if( $entry == 'found' ) {
    break;
    //or
    return $entry;
}

Sometimes it is also desirable to return in case something went wrong. For instance, in my first suggestion I said to abstract the $handle from the if statement. If we were to reverse that if statement we could issue a return statement in case initializing the handle failed. This would prevent the rest of the code from running.

if( ! $handle ) {
    return FALSE;
}

I believe your procedure for deletion is backwards. You chmod() the file to give you permissions on it, then you immediately delete it, then you finally check if that file is a directory. The file doesn't exist anymore. You've already deleted it. Check if that file is a directory first, then delete it. Though this is just a general bit of advice. Because unlink() also deletes directories and not just files the recursion is implied in the functionality and unnecessary to implement explicitly. However, to help you understand the recursion, I'm going to show you a version that deletes only the files and leaves the directory structure.

if( file_exists( $absolute_path ) && is_dir( $absolute_path ) ) {
    //loop over new directory
} else {
    chmod( $absolute_path, 0755 );
    unlink( $absolute_path );
}

Let's take a look at is_dir(). Here is the documentation for this function's return values:

Returns TRUE if the filename exists and is a directory, FALSE otherwise.

Therefore, explicitly checking if the file_exists() is unnecessary and redundant. You can remove this function from your statements.

if( is_dir( $absolute_path ) ) {

Since we are talking about functions anyways, lets discuss one of the core principles, "Don't Repeat Yourself" (DRY). As the name implies, your code should not repeat itself. This is the main problem with your code. It is not recursive, though you are trying to simulate recursion by repeating code, and therefore this would normally not work as expected. You just lucked out. Lets combine what I've already showed you to create a recursive function. Please, do not just copy and paste this. Manually type every line and try to understand how it works first. I have hidden the code in case you wish to try this for yourself first, which I encourage you to do. Reread the above if you need to. Every thing I'm about to reveal I discussed above. Mouse over the block below to reveal it.

    function recursiveDelete( $path ) {
        $handle = opendir( $path );

        if( ! $handle ) {
            return FALSE;
        }

        while( ( $entry = readdir( $handle ) ) !== FALSE ) {
            if( $entry == '.' || $entry == '..' ) {
                continue;
            }

            $file = "$path/$entry";

            if( is_dir( $file ) ) {
                recursiveDelete();
            } else {
                chmod( $file, 0755 );
                unlink( $file );
            }
        }
    }

There's more to DRY than just recursive functions. There's proper use of variables and loops as well, but going into every way you can keep your code DRY is a bit beyond the scope of this post. If you want a full explanation the first results on Google are likely to be extremely helpful.

Draft 2

There's not really anything here for me to check. As I imagine you probably got this from somewhere. However, I will explain the ternary to you.

$file->isDir() ? rmdir( $file ) : unlink( $file );
//is the same as
if( $file->isDir() ) {
    rmdir( $file );
} else {
    unlink( $file );
}

Ternary is a powerful tool. Though some people don't seem to like it much, I am not one of them. If used appropriately it can not only enhance a program's efficiency, but improve its legibility. The ternary shown above is an example of good ternary. When ternary starts making your code illegible or complex, then you should revert to using standard if/else statements. A general rule of thumb is to never nest ternary statements and never use ternary statements that break the 80th column. In case you are confused what that means, a column is the exactly one character, so 80 characters, including spacing.

Hope this helps!

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.