Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CFileHelper::copyDirectory recursive mkdir. #1146

Merged
merged 5 commits into from
Dec 1, 2012
Merged

CFileHelper::copyDirectory recursive mkdir. #1146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f763dba
* copyDirectory creates dstDir recursively.
Aug 6, 2012
8381505
* recursive dst creation at copyDirectory.
Aug 7, 2012
0aeef5e
* CSC
Aug 9, 2012
b892a0b
+ CFileHelperTest
Oct 12, 2012
2888b09
Merge remote-tracking branch 'upstream/master' into CFileHelper_recur…
Oct 19, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 30 additions & 7 deletions framework/utils/CFileHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public static function getExtension($path)

/**
* Copies a directory recursively as another.
* If the destination directory does not exist, it will be created.
* If the destination directory does not exist, it will be created recursively.
* @param string $src the source directory
* @param string $dst the destination directory
* @param array $options options for directory copy. Valid options are:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

documentation for mode options is missing in this method, please copy them from protected method copyDirectoryRecursive.

     * <li>newDirMode - the permission to be set for newly copied directories (defaults to 0777);</li>
     * <li>newFileMode - the permission to be set for newly copied files (defaults to the current environment setting).</li>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just noticed that all the options described in the docs of copyDirectory are completely ignored...!? Has nobody noticed that until now, or am I overlooking something? @yiisoft/core-developers

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ignored?
they get extracted on the line 57

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh damn, thanks for opening my eyes @mdomba :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are working fine as @mdomba mentioned. Used this method just recently.

Expand All @@ -55,6 +55,9 @@ public static function copyDirectory($src,$dst,$options=array())
$exclude=array();
$level=-1;
extract($options);
if(!is_dir($dst))
self::mkdir($dst, $options, true);

self::copyDirectoryRecursive($src,$dst,'',$fileTypes,$exclude,$level,$options);
}

Expand Down Expand Up @@ -110,11 +113,8 @@ public static function findFiles($dir,$options=array())
protected static function copyDirectoryRecursive($src,$dst,$base,$fileTypes,$exclude,$level,$options)
{
if(!is_dir($dst))
mkdir($dst);
if(isset($options['newDirMode']))
@chmod($dst,$options['newDirMode']);
else
@chmod($dst,0777);
self::mkdir($dst, $options, false);

$folder=opendir($src);
while(($file=readdir($folder))!==false)
{
Expand All @@ -128,7 +128,7 @@ protected static function copyDirectoryRecursive($src,$dst,$base,$fileTypes,$exc
{
copy($path,$dst.DIRECTORY_SEPARATOR.$file);
if(isset($options['newFileMode']))
@chmod($dst.DIRECTORY_SEPARATOR.$file, $options['newFileMode']);
chmod($dst.DIRECTORY_SEPARATOR.$file, $options['newFileMode']);
}
elseif($level)
self::copyDirectoryRecursive($path,$dst.DIRECTORY_SEPARATOR.$file,$base.'/'.$file,$fileTypes,$exclude,$level-1,$options);
Expand Down Expand Up @@ -263,4 +263,27 @@ public static function getMimeTypeByExtension($file,$magicFile=null)
}
return null;
}

/**
* Shared environment safe version of mkdir. Supports recursive creation.
* For avoidance of umask side-effects chmod is used.
*
* @static
* @param string $dst path to be created
* @param array $options newDirMode element used, must contain access bitmask.
* @param boolean $recursive
* @return boolean result of mkdir
* @see mkdir
*/
private static function mkdir($dst, array $options, $recursive)
{
$prevDir = dirname($dst);
if ($recursive && !is_dir($dst) && !is_dir($prevDir)) self::mkdir(dirname($dst), $options, true);

$mode = isset($options['newDirMode']) ? $options['newDirMode'] : 0777;
$res = mkdir($dst, $mode);
chmod($dst, $mode);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doing chmod here? What is "concurrent compatibility"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've read on PHP.net that using mkdir mode can cause troubles with fpm. If I'm understanding your question right.

Alexander Makarov notifications@github.com wrote:

  • /**
  • \* Creates directory for $dst path with $mode and $recursive creation is allowed.

  • \* For concurrent compatibility chmod is used instead of mkdir's $mode.

  • *

  • \* @static

  • \* @param string $dst path to be created

  • \* @param integer $mode access bitmask

  • \* @param boolean $recursive

  • \* @return boolean result of mkdir

  • \* @see mkdir

  • */
  • private static function mkdir($dst, $mode, $recursive)
  • {
  •    $res = mkdir($dst, $mode, $recursive);

  •    chmod($dst, $mode);

Why doing chmod here?

Reply to this email directly or view it on GitHub:
https://github.com/yiisoft/yii/pull/1146/files#r1810581

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What kind of troubles exactly? mkdir respects sticky flag so probably chmod is necessary but you're calling it once and not for each dir in the path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, troubles was with umask: http://www.php.net/manual/en/function.umask.php (ive used it in other revisions).
I guess Ive just copied this from other yii code, suggesting that this is some kind of style.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, chmod is not affected with umask. so there are no side effects

return $res;
}

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In lines 266-268,282 please use integer and boolean for @param, as this is what the rest of Yii's PHPdoc uses for these types.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the new methods mkdir() and getModeFromOptions(). They make simple code harder to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not agree, it removes code duplication, and we have a constant here. It must be in one place.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree big chunk of duplicated code or code that are repeated many times should should be refactored with methods.
However, for simple code like the example above, writing them as methods just makes the code less easy to read (readers have to jump back and forth to understand the code) and less efficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reader dont have to jump if method named right (exposes its intent). I can rename it to getAccessMode() for more readability and shortness. mkdir() and getAccessMode() I think must be in separate methods because its more error prone to duplicate that code (you can assume that this algorithm used once, and forget to look for copy of it) and makes class code shorter (more readable).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you argue? This piece of code is not required to provide the method.

you can assume that this algorithm used once, and forget to look for copy of it

Enough already to justify a bad refactoring, covering everything forever oblivious programmers. It is a myth.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Friendly. He argues because he believes strongly in his opinion. He does have some valid points, it's just that people value them and other aspects differently.

Either case it is the core dev team that has the last word, naturally. Either you like what he did or reject it or pick it up and refactor it the way you want it to be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@creocoder please tell me more about "good refactoring".
Like @rawtaz said i have my points on this refactroings, which im explaining to @qiangxue. Youre just acting like smug. If your have nothing better to say, better stay silent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@senz Good books about Refactoring will tell you more. Refactoring its not about idiotic duplicates eliminating. Its smart process. You have get:

I don't like the new methods mkdir() and getModeFromOptions().

You say:

I do not agree

You get more explain:

I totally agree big chunk of duplicated code or code that are repeated many times should should be refactored with methods...

You say:

I do not agree (not literally)

What do you expect on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@creocoder I dont want to continue this arguing. You are not being constructive. Just giving insults.
@cebe is assigned here. So Im waiting his thoughts.

105 changes: 84 additions & 21 deletions tests/framework/utils/CFileHelperTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,18 @@
<?php

class CFileHelperTest extends CTestCase
{
public function setUp()
private $testDir;
private $testMode = 0770;
private $rootDir1 = "test1";
private $rootDir2 = "test2";
private $subDir = 'sub';
private $file = 'testfile';

protected function setUp()
{
if(!is_dir(Yii::getPathOfAlias('application.runtime.CFileHelper')) &&
!(@mkdir(Yii::getPathOfAlias('application.runtime.CFileHelper'))))
$this->testDir = Yii::getPathOfAlias('application.runtime.CFileHelper');
if(!is_dir($this->testDir) &&
!(@mkdir($this->testDir)))
$this->markTestIncomplete('Unit tests runtime directory should have writable permissions!');

// create temporary testing data files
Expand All @@ -14,25 +21,17 @@ public function setUp()
'mimeTypes2.php'=>"<?php return array('txt'=>'text/plain','txb'=>'another/mime2');",
);
foreach($filesData as $fileName=>$fileData)
if(!(@file_put_contents(Yii::getPathOfAlias('application.runtime.CFileHelper').$fileName,$fileData)))
if(!(@file_put_contents($this->testDir.$fileName,$fileData)))
$this->markTestIncomplete('Unit tests runtime directory should have writable permissions!');
}

public function tearDown()
protected function tearDown()
{
// clean up temporary testing data files
foreach(array('mimeTypes1.php','mimeTypes2.php') as $fileName)
if(is_file(Yii::getPathOfAlias('application.runtime.CFileHelper').$fileName))
@unlink(Yii::getPathOfAlias('application.runtime.CFileHelper').$fileName);

if(is_dir(Yii::getPathOfAlias('application.runtime.CFileHelper')))
@rmdir(Yii::getPathOfAlias('application.runtime.CFileHelper'));
if (is_dir($this->testDir)) $this->rrmdir($this->testDir);
}

public function testGetMimeTypeByExtension()
{
$runtimePath=Yii::getPathOfAlias('application.runtime.CFileHelper');

// run everything ten times in one test action to be sure that caching inside
// CFileHelper::getMimeTypeByExtension() is working the right way
for($i=0; $i<10; $i++)
Expand All @@ -41,13 +40,77 @@ public function testGetMimeTypeByExtension()
$this->assertNull(CFileHelper::getMimeTypeByExtension('test.txb'));
$this->assertEquals('text/plain',CFileHelper::getMimeTypeByExtension('test.txt'));

$this->assertEquals('application/json',CFileHelper::getMimeTypeByExtension('test.txa',$runtimePath.'mimeTypes1.php'));
$this->assertEquals('another/mime',CFileHelper::getMimeTypeByExtension('test.txb',$runtimePath.'mimeTypes1.php'));
$this->assertNull(CFileHelper::getMimeTypeByExtension('test.txt',$runtimePath.'mimeTypes1.php'));
$this->assertEquals('application/json',CFileHelper::getMimeTypeByExtension('test.txa',$this->testDir.'mimeTypes1.php'));
$this->assertEquals('another/mime',CFileHelper::getMimeTypeByExtension('test.txb',$this->testDir.'mimeTypes1.php'));
$this->assertNull(CFileHelper::getMimeTypeByExtension('test.txt',$this->testDir.'mimeTypes1.php'));

$this->assertNull(CFileHelper::getMimeTypeByExtension('test.txa',$this->testDir.'mimeTypes2.php'));
$this->assertEquals('another/mime2',CFileHelper::getMimeTypeByExtension('test.txb',$this->testDir.'mimeTypes2.php'));
$this->assertEquals('text/plain',CFileHelper::getMimeTypeByExtension('test.txt',$this->testDir.'mimeTypes2.php'));
}
}

public function testCopyDirectory_subDir_modeShoudBe0775()
{
$this->createTestStruct($this->testDir);
$src = $this->testDir . DIRECTORY_SEPARATOR . $this->rootDir1;
$dst = $this->testDir . DIRECTORY_SEPARATOR . $this->rootDir2;
CFileHelper::copyDirectory($src, $dst, array('newDirMode' => $this->testMode));

$subDir2Mode = $this->getMode($dst . DIRECTORY_SEPARATOR . $this->subDir );
$expectedMode = sprintf('%o', $this->testMode);
$this->assertEquals($expectedMode, $subDir2Mode, "Subdir mode is not {$expectedMode}");
}

public function testCopyDirectory_subDir_modeShoudBe0777()
{
$this->createTestStruct($this->testDir);
$src = $this->testDir . DIRECTORY_SEPARATOR . $this->rootDir1;
$dst = $this->testDir . DIRECTORY_SEPARATOR . $this->rootDir2;
CFileHelper::copyDirectory($src, $dst);

$this->assertNull(CFileHelper::getMimeTypeByExtension('test.txa',$runtimePath.'mimeTypes2.php'));
$this->assertEquals('another/mime2',CFileHelper::getMimeTypeByExtension('test.txb',$runtimePath.'mimeTypes2.php'));
$this->assertEquals('text/plain',CFileHelper::getMimeTypeByExtension('test.txt',$runtimePath.'mimeTypes2.php'));
$subDir2Mode = $this->getMode($dst . DIRECTORY_SEPARATOR . $this->subDir );
$expectedMode = sprintf('%o', 0777);
$this->assertEquals($expectedMode, $subDir2Mode, "Subdir mode is not {$expectedMode}");
}

private function createTestStruct($testDir)
{
$rootDir = $testDir . DIRECTORY_SEPARATOR . $this->rootDir1;
mkdir($rootDir);

$subDir = $testDir . DIRECTORY_SEPARATOR . $this->rootDir1 . DIRECTORY_SEPARATOR . $this->subDir;
mkdir($subDir);

$file = $testDir . DIRECTORY_SEPARATOR . $this->rootDir1 . DIRECTORY_SEPARATOR . $this->subDir . DIRECTORY_SEPARATOR . $this->file;
file_put_contents($file, '12321312');
}

private function getMode($file)
{
return substr(sprintf('%o', fileperms($file)), -4);
}

private function rrmdir($dir)
{
if ($handle = opendir($dir))
{
while (false !== ($entry = readdir($handle)))
{
if ($entry != "." && $entry != "..")
{
if (is_dir($dir . "/" . $entry) === true)
{
$this->rrmdir($dir . "/" . $entry);
}
else
{
unlink($dir . "/" . $entry);
}
}
}
closedir($handle);
rmdir($dir);
}
}
}