CFileHelper::copyDirectory recursive mkdir. #1146

Merged
merged 5 commits into from Dec 1, 2012

5 participants

@senz

In case of dstDir deeper than 1 level we must create it recursively as CFileHelper::copyDirectory docblock says:

Copies a directory recursively as another.
If the destination directory does not exist, it will be created.

@qiangxue
Yii Software LLC member

It's better to be done in copyDirectory() rather than copyDirectoryRecursive().
We also shouldn't care about umask() setting.

@cebe cebe was assigned Aug 6, 2012
@senz

@qiangxue ok, i will move whole block to copyDirectroy
umask affects mkdir, but ive just read about its problems with concurrency, so will change back to chmod.

Konstantin G Romanov * recursive dst creation at copyDirectory.
* dir access mode is set by chmod, again (note: if dst is created recursively, then only last dir will have newDirMode access).
8381505
@rawtaz rawtaz and 1 other commented on an outdated diff Aug 9, 2012
framework/utils/CFileHelper.php
@@ -56,6 +56,8 @@ public static function copyDirectory($src,$dst,$options=array())
$exclude=array();
$level=-1;
extract($options);
+ if(!is_dir($dst)) self::mkdir($dst, self::getModeFromOptions($options), true);
@rawtaz
rawtaz added a line comment Aug 9, 2012

Please add a newline between if() and the following statement.

@creocoder
creocoder added a line comment Aug 9, 2012

Remove space after commas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rawtaz rawtaz and 1 other commented on an outdated diff Aug 9, 2012
framework/utils/CFileHelper.php
@@ -110,12 +112,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);
+ if(!is_dir($dst)) self::mkdir($dst, self::getModeFromOptions($options), false);
@rawtaz
rawtaz added a line comment Aug 9, 2012

Please add a newline between if() and the following statement.

@creocoder
creocoder added a line comment Aug 9, 2012

Remove space after commas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rawtaz rawtaz commented on an outdated diff Aug 9, 2012
framework/utils/CFileHelper.php
@@ -258,4 +256,34 @@ public static function getMimeTypeByExtension($file,$magicFile=null)
}
return null;
}
+
+ /**
+ * 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 int $mode access bitmask
+ * @param bool $recursive
+ * @return bool result of mkdir
+ * @see \mkdir
@rawtaz
rawtaz added a line comment Aug 9, 2012

Is \mkdir Doxygen syntax or something? Other Yii code doesn't have the \ in @see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rawtaz rawtaz and 2 others commented on an outdated diff Aug 9, 2012
framework/utils/CFileHelper.php
+
+ /**
+ * 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 int $mode access bitmask
+ * @param bool $recursive
+ * @return bool result of mkdir
+ * @see \mkdir
+ */
+ private static function mkdir($dst, $mode, $recursive)
+ {
+ $res = mkdir($dst, $mode, $recursive);
+ @chmod($dst, $mode);
@rawtaz
rawtaz added a line comment Aug 9, 2012

Is @ really needed here? The fewer @ the better, in general.

@senz
senz added a line comment Aug 9, 2012

Silence operator was used here (and in some other places) before. Thought it was for a reason.
Have no arguments for it necessity.

@rawtaz
rawtaz added a line comment Aug 9, 2012

Understand :) Maybe @qiangxue knows?

@qiangxue
Yii Software LLC member
qiangxue added a line comment Aug 9, 2012

It's possible that chmod may fail.

@senz
senz added a line comment Aug 9, 2012

mkdir may fail too, as every other fs-related method. Mb framework user wants to know about fail (you can set 'scream' on newer php i think)?

@rawtaz
rawtaz added a line comment Aug 9, 2012

@qiangxue But if it fails it fails for a reason, shouldn't such a situation be alerting rather than "hidden"? I'm sure there's a reason that @ is/was there, I'm just curious about the reason for hiding the potential error.

@qiangxue
Yii Software LLC member
qiangxue added a line comment Aug 9, 2012

Some server configuration may not allow you to chmod or perform other file operations. Even if you know the reason, there's no way to fix them (because you don't have control of the server). Other than using @, I don't know other better solutions. With @, at least you have the option of changing error reporting flag so that the error is not inhibited by @.

@senz
senz added a line comment Aug 10, 2012

error_reporting not affects @ behavior, scream does http://php.net/manual/en/scream.examples-simple.php But its since 5.2 http://www.php.net/manual/en/scream.requirements.php

I think you should check chmod and other file operations in requirements script, and warn user. As @rawtaz said, user should know if something fails, and why, or debug will be a great pain, especially for newbie.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rawtaz rawtaz and 3 others commented on an outdated diff Aug 9, 2012
framework/utils/CFileHelper.php
+ $res = mkdir($dst, $mode, $recursive);
+ @chmod($dst, $mode);
+ return $res;
+ }
+
+ /**
+ * Returns dir access mode from options, if set, or default value (0777).
+ * @static
+ * @param array $options
+ * @return int
+ */
+ private static function getModeFromOptions(array $options)
+ {
+ return isset($options['newDirMode']) ? $options['newDirMode'] : 0777;
+ }
+
}
@rawtaz
rawtaz added a line comment Aug 9, 2012

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.

@qiangxue
Yii Software LLC member
qiangxue added a line comment Aug 9, 2012

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

@senz
senz added a line comment Aug 9, 2012

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

@qiangxue
Yii Software LLC member
qiangxue added a line comment Aug 9, 2012

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.

@senz
senz added a line comment Aug 10, 2012

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).

@creocoder
creocoder added a line comment Aug 10, 2012

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.

@rawtaz
rawtaz added a line comment Aug 10, 2012

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.

@senz
senz added a line comment Aug 10, 2012

@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.

@creocoder
creocoder added a line comment Aug 10, 2012

@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?

@senz
senz added a line comment Aug 10, 2012

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Konstantin G Romanov * CSC 0aeef5e
@senz

@rawtaz where can i read projects CSC?

@samdark
Yii Software LLC member

@senz I'm going to put these together as a wiki page soon. Currently there's no such document.

@rawtaz

The current source code is the best option for now :) I use it quite a lot to search for patterns when I want to know how something is usually done, for example the PHPdoc stuff (whether to write boolean or bool, etc).

@samdark
Yii Software LLC member

https://github.com/yiisoft/yii/wiki/Core-framework-code-style here it is. Feel free to edit it adding facts from current source code and examples.

@rawtaz

Issue #1179 can be used for discussions about the new wiki page. Thanks for setting up the wiki page @samdark.

@samdark
Yii Software LLC member

Thanks.

@ghost ghost assigned samdark and cebe Sep 8, 2012
@samdark samdark and 1 other commented on an outdated diff Oct 10, 2012
framework/utils/CFileHelper.php
+
+ /**
+ * 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);
@samdark
Yii Software LLC member
samdark added a line comment Oct 10, 2012

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

@senz
senz added a line comment Oct 11, 2012
@samdark
Yii Software LLC member
samdark added a line comment Oct 11, 2012

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.

@senz
senz added a line comment Oct 12, 2012

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.

@senz
senz added a line comment Oct 12, 2012

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@creocoder creocoder and 2 others commented on an outdated diff Oct 11, 2012
framework/utils/CFileHelper.php
@@ -258,4 +258,34 @@ public static function getMimeTypeByExtension($file,$magicFile=null)
}
return null;
}
+
+ /**
+ * 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)
@creocoder
creocoder added a line comment Oct 11, 2012

I suggest remove this private method.

@samdark
Yii Software LLC member
samdark added a line comment Oct 11, 2012

Depends on if chmod is really needed and if it should be applied recursively.

@senz
senz added a line comment Oct 12, 2012

Whole point was support of recursion. so it is needed.

@samdark
Yii Software LLC member
samdark added a line comment Oct 12, 2012

But, as I can see, it's not applied recursively in this case:

CFileHelper::mkdir('/var/www/mydir', 0777, true);
@senz
senz added a line comment Oct 12, 2012

will fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@creocoder creocoder and 2 others commented on an outdated diff Oct 11, 2012
framework/utils/CFileHelper.php
+ * @see mkdir
+ */
+ private static function mkdir($dst, $mode, $recursive)
+ {
+ $res = mkdir($dst, $mode, $recursive);
+ chmod($dst, $mode);
+ return $res;
+ }
+
+ /**
+ * Returns dir access mode from options, if set, or default value (0777).
+ * @static
+ * @param array $options
+ * @return integer
+ */
+ private static function getModeFromOptions(array $options)
@creocoder
creocoder added a line comment Oct 11, 2012

I suggest remove this private method too.

@samdark
Yii Software LLC member
samdark added a line comment Oct 11, 2012

Agreee.

@senz
senz added a line comment Oct 12, 2012

i suggest to inline it in mkdir

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Konstantin G Romanov + CFileHelperTest
* newDirMode correctly setted on recursive creation of dst dir.
b892a0b
@samdark samdark was assigned Oct 12, 2012
@samdark
Yii Software LLC member

Thanks for the test. Will check it soon.

Konstantin G Romanov Merge remote-tracking branch 'upstream/master' into CFileHelper_recur…
…sive_copy_fix

Conflicts:
	tests/framework/utils/CFileHelperTest.php
2888b09
@cebe cebe commented on the diff Nov 21, 2012
framework/utils/CFileHelper.php
@@ -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:
@cebe
Yii Software LLC member
cebe added a line comment Nov 21, 2012

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>
@cebe
Yii Software LLC member
cebe added a line comment Nov 21, 2012

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

@mdomba
Yii Software LLC member
mdomba added a line comment Nov 21, 2012

ignored?
they get extracted on the line 57

@cebe
Yii Software LLC member
cebe added a line comment Nov 21, 2012

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

@samdark
Yii Software LLC member
samdark added a line comment Nov 21, 2012

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@samdark samdark merged commit 2888b09 into yiisoft:master Dec 1, 2012
@samdark
Yii Software LLC member

Thanks for working on it. Merged with some adjustments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment