-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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.
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
fix: directory_mirror() throws an error if destination directory exists #5493
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect. It works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice fix @kenjis
@@ -91,7 +91,9 @@ function directory_mirror(string $originDir, string $targetDir, bool $overwrite | |||
$target = $targetDir . substr($origin, $dirLen); | |||
|
|||
if ($file->isDir()) { | |||
mkdir($target, 0755); | |||
if (! is_dir($target)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make this not nested if? I mean you can combine this with the outer if
by a &&
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--- a/system/Helpers/filesystem_helper.php
+++ b/system/Helpers/filesystem_helper.php
@@ -90,10 +90,8 @@ if (! function_exists('directory_mirror')) {
$origin = $file->getPathname();
$target = $targetDir . substr($origin, $dirLen);
- if ($file->isDir()) {
- if (! is_dir($target)) {
- mkdir($target, 0755);
- }
+ if ($file->isDir() && ! is_dir($target)) {
+ mkdir($target, 0755);
} elseif (! is_file($target) || ($overwrite && is_file($target))) {
copy($origin, $target);
}
$ ./phpunit tests/system/Helpers/FilesystemHelperTest.php
PHPUnit 9.5.10 by Sebastian Bergmann and contributors.
Runtime: PHP 8.0.14
Configuration: /Users/kenji/work/codeigniter/CodeIgniter4/phpunit.xml
.......E............................. 37 / 37 (100%)
Time: 00:00.256, Memory: 12.00 MB
There was 1 error:
1) CodeIgniter\Helpers\FilesystemHelperTest::testDirectoryMirrorSkipExistingFolder
ErrorException: copy(): The first argument to copy() function cannot be a directory
/Users/kenji/work/codeigniter/CodeIgniter4/system/Helpers/filesystem_helper.php:96
/Users/kenji/work/codeigniter/CodeIgniter4/tests/system/Helpers/FilesystemHelperTest.php:181
ERRORS!
Tests: 37, Assertions: 87, Errors: 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulbalandan If you want to remove if in if, this is the code.
But I'm not sure it is correct.
--- a/system/Helpers/filesystem_helper.php
+++ b/system/Helpers/filesystem_helper.php
@@ -90,10 +90,12 @@ if (! function_exists('directory_mirror')) {
$origin = $file->getPathname();
$target = $targetDir . substr($origin, $dirLen);
- if ($file->isDir()) {
- if (! is_dir($target)) {
- mkdir($target, 0755);
- }
+ if ($file->isDir() && ! is_dir($target)) {
+ mkdir($target, 0755);
+ } elseif ($file->isDir() && is_dir($target)) {
+ continue;
+ } elseif (is_dir($target)) {
+ continue;
} elseif (! is_file($target) || ($overwrite && is_file($target))) {
copy($origin, $target);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this:
diff --git a/system/Helpers/filesystem_helper.php b/system/Helpers/filesystem_helper.php
index 89d4d507f..104b739a3 100644
--- a/system/Helpers/filesystem_helper.php
+++ b/system/Helpers/filesystem_helper.php
@@ -90,11 +90,11 @@ if (! function_exists('directory_mirror')) {
$origin = $file->getPathname();
$target = $targetDir . substr($origin, $dirLen);
- if ($file->isDir()) {
- if (! is_dir($target)) {
- mkdir($target, 0755);
- }
- } elseif (! is_file($target) || ($overwrite && is_file($target))) {
+ if ($file->isDir() && ! is_dir($target)) {
+ mkdir($target, 0755);
+ }
+
+ if ($file->isFile() && (! is_file($target) || $overwrite)) {
copy($origin, $target);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the conditions of the if statements does not seem to make it easier to understand.
Description
Fixes #5478
Checklist: