HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Recursive file copy function

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
recursivefilefunctioncopy

Problem

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?

Solution

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.

Code Snippets

$sSrc = (substr($sSrc,-1)=='/' ? substr($sSrc,0,-1) : $sSrc);
$sDst = (substr($sDst,-1)=='/' ? substr($sDst,0,-1) : $sDst);
$sSrc = rtrim($sSrc,'/');
$sDst = rtrim($sDst,'/');
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!
}

Context

StackExchange Code Review Q#85003, answer score: 3

Revisions (0)

No revisions yet.