yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
14.24k stars 6.91k forks source link

Is BaseFileHelper::normalizePath not handling the current path (.) correctly? #3522

Closed skotos closed 10 years ago

skotos commented 10 years ago

When I execute the following I get an error...

vendor/bin/apidoc api . /tmp/docs

I've traced this to the \yii\helpers\BaseFileHelper::normalizePath function turning "." into a blank string. Should it instead leave a simple "." as a reference to the current directory?

The following patch would accomplish this, but it would also mean normalizePath('./test') === "./test" (not "test"). What would be best for the framework?

diff --git a/framework/helpers/BaseFileHelper.php b/framework/helpers/BaseFileHelper.php
index 0974d8d..5cc30e5 100644
--- a/framework/helpers/BaseFileHelper.php
+++ b/framework/helpers/BaseFileHelper.php
@@ -49,8 +49,10 @@ class BaseFileHelper
         $parts = [];
         foreach (explode($ds, $path) as $part) {
             if ($part === '..' && !empty($parts) && end($parts) !== '..') {
-                array_pop($parts);
-            } elseif ($part === '.' || $part === '' && !empty($parts)) {
+                if (array_pop($parts) === ".") {
+                    $parts[] = '..';
+                }
+            } elseif (( $part === '.' || $part === '' ) && !empty($parts)) {
                 continue;
             } else {
                 $parts[] = $part;
diff --git a/tests/unit/framework/helpers/FileHelperTest.php b/tests/unit/framework/helpers/FileHelperTest.php
index b8822df..7d6ce9b 100644
--- a/tests/unit/framework/helpers/FileHelperTest.php
+++ b/tests/unit/framework/helpers/FileHelperTest.php
@@ -369,10 +369,13 @@ class FileHelperTest extends TestCase
         $this->assertEquals("..{$ds}c", FileHelper::normalizePath('//a/.\\b//..//..//../../c'));

         // relative paths
+        $this->assertEquals(".", FileHelper::normalizePath('.'));
+        $this->assertEquals(".", FileHelper::normalizePath('./'));
         $this->assertEquals("..{$ds}..{$ds}a", FileHelper::normalizePath('../..\\a'));
         $this->assertEquals("..{$ds}..{$ds}a", FileHelper::normalizePath('../..\\a/../a'));
         $this->assertEquals("..{$ds}..{$ds}b", FileHelper::normalizePath('../..\\a/../b'));
         $this->assertEquals("..{$ds}a", FileHelper::normalizePath('./..\\a'));
+        $this->assertEquals("..{$ds}a", FileHelper::normalizePath('././..\\a'));
         $this->assertEquals("..{$ds}a", FileHelper::normalizePath('./..\\a/../a'));
         $this->assertEquals("..{$ds}b", FileHelper::normalizePath('./..\\a/../b'));
     }
skotos commented 10 years ago

If you like the above patch, I can do a pull request on it.

samdark commented 10 years ago

@skotos I think normalizePath('./test') should still give you test.

skotos commented 10 years ago

That's what I thought. I'll submit a pull request after I tweak it.

skotos commented 10 years ago

Sorry, I accidently created the Pull Request as #3529

cebe commented 10 years ago

I've traced this to the \yii\helpers\BaseFileHelper::normalizePath function turning "." into a blank string. Should it instead leave a simple "." as a reference to the current directory?

isn't a blank string and . equivalent for current dir?

samdark commented 10 years ago

Not in case when a command requires input. If it's . it is considered an argument with current dir as a values. If it's empty it's considered as missing argument.

cebe commented 10 years ago

okay but what is the problem then? normalizePath is called after the arguments have been given.

skotos commented 10 years ago

$ php -r "var_dump(is_dir('.'));" (true) $ php -r "var_dump(is_dir(''));" (false)

$ php -r "var_dump(scandir(''));"
PHP Warning:  scandir(): Directory name cannot be empty in Command line code on line 1
bool(false)
$ php -r "var_dump(scandir('.'));"
array(4) {
  [0]=>
  string(1) "."
  [1]=>
  string(2) ".."
  [2]=>
  string(3) "bar"
  [3]=>
  string(3) "foo"
}
cebe commented 10 years ago

thanks, its clear now.