Recursive file copy function

Question

I created this function and I'm unsure if it is sufficient for "real world use", because all code snippets on the net I found for recursive copy functions used functions like

$hDir = opendir($sSrc)

which I don't think are necessary. That's why I like to have my func checked by some PHP professionals:

It recursively copies directories.

Params:

• $sSrc Path of directory, of which the content is to be copied
• $sDst Path of directory, into which the content of $sSrc is to be
  copied. Non existing directories are (recursively) created.
• $blnKeepPerm Optional argument specifying if destination objects
  must have the same permission as their source objects

Returns: True only if ALL elements are copied, otherwise false.

function rcopy ($sSrc, $sDst, $blnKeepPerm = 0) {
  if (! is_dir($sSrc)) return false;
  $sSrc = (substr($sSrc,-1)=='/' ? substr($sSrc,0,-1) : $sSrc);
  $sDst = (substr($sDst,-1)=='/' ? substr($sDst,0,-1) : $sDst);
  $blnRetval = true;
  if (! is_dir($sDst)) {
    if ($blnKeepPerm) {
      if (! @mkdir($sDst, fileperms($sSrc), true)) return false;
    } else {
      if (! @mkdir($sDst), (0777-umask()), true) return false;
    }
  }
  foreach (scandir($sSrc) as $sObj) {
    if ($sObj != '.' && $sObj != '..') {
      if (is_dir("$sSrc/$sObj")) {
        // the function returns true only if copying ALL of the 
        // files/dirs succeeded. To prevent overwriting of $blnRetVal
        // by a later rcopy-call, the return values here are ANDed:
        $blnRetval = $blnRetval & rcopy ("$sSrc/$sObj", "$sDst/$sObj", $blnKeepPerm);
      } else {
        if (@copy ("$sSrc/$sObj", "$sDst/$sObj")) {
          if ($blnKeepPerm) {
            @chmod ("$sDst/$sObj", fileperms("$sSrc/$sObj"));
          }
        } else  $blnRetval = false;
      }
    }
  }
  return $blnRetval;
}

What do you think? What could/should be made better?

Answer 1

1: You can replace:

$sSrc = (substr($sSrc,-1)=='/' ? substr($sSrc,0,-1) : $sSrc);
$sDst = (substr($sDst,-1)=='/' ? substr($sDst,0,-1) : $sDst);

with:

$sSrc = rtrim($sSrc,'/');
$sDst = rtrim($sDst,'/');

2: I would prefer the use of more normal variable names. There's no reason to use $blnKeepPerm if you can be clearer $copyPerms.

3: There is some repetition in your code.

4: The use of $blnRetval leads you to execute stuff after a failure.

In all there's really nothing wrong with this function, but it could be written more clearly. I'll give it a try:

function copyDir($sourceDir,$destDir,$copyPerms = FALSE) 
{
  // cleanup
  $sourceDir = rtrim($sourceDir,'/');
  $destDir   = rtrim($destDir,'/');
  // protect
  if (!is_dir($sourceDir)) return FALSE;
  // check destination dir
  if (!is_dir($destDir)) 
  {
    // create directory
    $permissions = $copyPerms ? fileperms($sourceDir) : 0777-umask();
    if (!@mkdir($destDir,$permissions,TRUE)) return FALSE;
  }
  // copy files and directories recursievely
  foreach (scandir($sourceDir) as $fileName) 
  {
    // do not copy current or parent directory
    if (!in_array($fileName,array('.','..'))) 
    {
      $sourcePath = "$sourceDir/$fileName";
      $destPath   = "$destDir/$fileName";
      // is it a file?
      if (!is_dir($sourcePath)) 
      {
        // oopy file
        if (!@copy($sourcePath,$destPath)) return FALSE;
        if ($copyPerms) && !@chmod($destPath,fileperms($sourcePath))) return FALSE;
      }
      // copy directory, here we recurse
      elseif (!copyDir($sourcePath,$destPath,$copyPerms)) return FALSE;
    }
  }
  return TRUE; // success!
}

I kept your code virtually intact. However, you can find other solutions online:

https://stackoverflow.com/questions/5707806/recursive-copy-of-directory

https://stackoverflow.com/questions/2050859/copy-entire-contents-of-a-directory-to-another-using-php

and many more.

Answer 2

I was busy doing other things, so I was off for a few days.

@KIKO Software: Thanks a lot for taking the time to review my function and to give me some input.

$sSrc = rtrim($sSrc,'/');

$sDst = rtrim($sDst,'/');

MUCH better than my code

$permissions = $copyPerms ? fileperms($sourceDir) : 0777-umask();

if (!@mkdir($destDir,$permissions,TRUE)) return FALSE;

MUCH better than my "if($blnKeepPerm){...}else{...}", and also the replacing of the often used Strings

"$sSrc/$sObj"

and

"$sDst/$sObj"

by simple/single variables makes sense.

if ($copyPerms) && !@chmod($destPath,fileperms($sourcePath))) return FALSE;

Also this shortening of my code makes sense.

The use of $blnRetval leads you to execute stuff after a failure

There's a missunderstanding here, I think: I did not want to make a function which is stopping after the first error. (That was an MS Windows behaviour for a long time and maybe still is, which drove me crazy too often.) My function is thought to finish the job, even if copying of one or several objects fails. The exit code at the end says if all worked well or if NOT all worked well. If necessary I can still implement e.g. an array storing all failed operations.

I would prefer the use of more normal variable names...

I think this is a question of preference: I like it if I can see at first sight if a variable like "$copyPerms" holds a bool value ($blnCopyPerms) or the name of a button ($sCopyPerms) or whatever type of value. (Maybe that in PHP the difference is of minor importance, but even in JS it is more important, and my programming roots are languages which strictly distinguish among different variable types, so for me it is also a habit of good practice.)

So my modified function is:

function copyDir ($sSrc, $sDst, $blnCopyPerms = false) {
  if (! is_dir($sSrc)) return false;
  $sSrc = rtrim($sSrc, '/');
  $sDst = rtrim($sDst, '/');
  $blnRetval = true;
  if (! is_dir($sDst)) {
    $iPermissions = $blnCopyPerms ? fileperms($sSrc) : 0777-umask();
    if (! @mkdir($sDst, $iPermissions, true)) return false;
  }
  foreach (scandir($sSrc) as $sObj) {
    if (!in_array($fileName, array('.','..'))) {
      $sSrcObj = "$sSrc/$sObj";
      $sDstObj = "$sDst/$sObj";
      if (is_dir($sSrcObj)) {
        $blnRetval = $blnRetval & rcopy ($sSrcObj, $sDstObj, $blnCopyPerms);
      } else {
        if (@copy ($sSrcObj, $sDstObj)) {
          if ($blnCopyPerms && !@chmod ($sDstObj, fileperms($sSrcObj)))  $blnRetval = false;
        } else  $blnRetval = false;
      }
    }
  }
  return $blnRetval;
}