[Filesystem] Reduce complexity of ->remove()

Author: nicolas-grekas

src/Symfony/Component/Filesystem/Filesystem.php

@@ -143,34 +143,23 @@ public function remove($files)
         $files = iterator_to_array($this->toIterator($files));
         $files = array_reverse($files);
         foreach ($files as $file) {
-            if (!$this->exists($file) && !is_link($file)) {
-                continue;
-            }
-
-            if (is_dir($file) && !is_link($file)) {
+            if (is_link($file)) {
+                // Workaround https://bugs.php.net/52176
+                if (!@unlink($file) && !@rmdir($file)) {
+                    $error = error_get_last();
+                    throw new IOException(sprintf('Failed to remove symlink "%s": %s.', $file, $error['message']));
+                }
+            } elseif (is_dir($file)) {
                 $this->remove(new \FilesystemIterator($file));
 
-                if (true !== @rmdir($file)) {
-                    throw new IOException(sprintf('Failed to remove directory %s', $file));
+                if (!@rmdir($file)) {
+                    $error = error_get_last();
+                    throw new IOException(sprintf('Failed to remove directory "%s": %s.', $file, $error['message']));
                 }
-            } else {
-                // https://bugs.php.net/bug.php?id=52176
-                if ('\\' === DIRECTORY_SEPARATOR && is_dir($file)) {
-                    if (true !== @rmdir($file)) {
-                        throw new IOException(sprintf('Failed to remove file %s', $file));
-                    }
-                } else {
-                    if (true !== @unlink($file)) {
-                        // handle broken symlinks on Windows systems
-                        if (is_link($file) && false === @readlink($file)) {
-                            if (true !== @rmdir($file)) {
-                                throw new IOException(sprintf('Failed to remove broken symlink "%s".', $file), 0, null, $file);
-                            }
-                        } else {
-                            $error = error_get_last();
-                            throw new IOException(sprintf('Failed to remove file "%s": %s.', $file, $error['message']));
-                        }
-                    }
+            } elseif ($this->exists($file)) {
+                if (!@unlink($file)) {
+                    $error = error_get_last();
+                    throw new IOException(sprintf('Failed to remove file "%s": %s.', $file, $error['message']));
                 }
             }
         }

src/Symfony/Component/Filesystem/Tests/FilesystemTest.php

@@ -281,7 +281,7 @@ public function testRemoveCleansFilesAndDirectoriesIteratively()
 
         $this->filesystem->remove($basePath);
 
-        $this->assertTrue(!is_dir($basePath));
+        $this->assertFileNotExists($basePath);
     }
 
     public function testRemoveCleansArrayOfFilesAndDirectories()
@@ -297,8 +297,8 @@ public function testRemoveCleansArrayOfFilesAndDirectories()
 
         $this->filesystem->remove($files);
 
-        $this->assertTrue(!is_dir($basePath.'dir'));
-        $this->assertTrue(!is_file($basePath.'file'));
+        $this->assertFileNotExists($basePath.'dir');
+        $this->assertFileNotExists($basePath.'file');
     }
 
     public function testRemoveCleansTraversableObjectOfFilesAndDirectories()
@@ -314,8 +314,8 @@ public function testRemoveCleansTraversableObjectOfFilesAndDirectories()
 
         $this->filesystem->remove($files);
 
-        $this->assertTrue(!is_dir($basePath.'dir'));
-        $this->assertTrue(!is_file($basePath.'file'));
+        $this->assertFileNotExists($basePath.'dir');
+        $this->assertFileNotExists($basePath.'file');
     }
 
     public function testRemoveIgnoresNonExistingFiles()
@@ -330,7 +330,7 @@ public function testRemoveIgnoresNonExistingFiles()
 
         $this->filesystem->remove($files);
 
-        $this->assertTrue(!is_dir($basePath.'dir'));
+        $this->assertFileNotExists($basePath.'dir');
     }
 
     public function testRemoveCleansInvalidLinks()
@@ -342,11 +342,19 @@ public function testRemoveCleansInvalidLinks()
         mkdir($basePath);
         mkdir($basePath.'dir');
         // create symlink to nonexistent file
-        @symlink($basePath.'file', $basePath.'link');
+        @symlink($basePath.'file', $basePath.'file-link');
+
+        // create symlink to dir using trailing forward slash
+        $this->filesystem->symlink($basePath.'dir/', $basePath.'dir-link');
+        $this->assertTrue(is_dir($basePath.'dir-link'));
+
+        // create symlink to nonexistent dir
+        rmdir($basePath.'dir');
+        $this->assertFalse(is_dir($basePath.'dir-link'));
 
         $this->filesystem->remove($basePath);
 
-        $this->assertTrue(!is_dir($basePath));
+        $this->assertFileNotExists($basePath);
     }
 
     public function testFilesExists()
@@ -1062,10 +1070,6 @@ private function getFileGroup($filepath)
      */
     private function markAsSkippedIfSymlinkIsMissing($relative = false)
     {
-        if (!function_exists('symlink')) {
-            $this->markTestSkipped('symlink is not supported');
-        }
-
         if (false === self::$symlinkOnWindows) {
             $this->markTestSkipped('symlink requires "Create symbolic links" privilege on Windows');
         }