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.

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: allow get_filenames() to follow symlinks #8225

Conversation

colethorsen
Copy link
Contributor

@colethorsen colethorsen commented Nov 16, 2023

Description

While symlinks that are going directly to a file are followed, the RecursiveDirectoryIterator does not follow folder based symlinks without setting the proper flag.

new RecursiveDirectoryIterator($sourceDir, RecursiveDirectoryIterator::SKIP_DOTS),

to

new RecursiveDirectoryIterator($sourceDir, RecursiveDirectoryIterator::SKIP_DOTS | FilesystemIterator::FOLLOW_SYMLINKS),

Originally posted by @colethorsen in #8216 (comment)

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis
Copy link
Member

kenjis commented Nov 16, 2023

There is no description for symlinks in the user guides.
http://www.codeigniter.com/userguide3/helpers/file_helper.html?highlight=get_filenames#get_filenames
https://codeigniter4.github.io/CodeIgniter4/helpers/filesystem_helper.html?highlight=get_filenames#get_filenames

In CI3:

$ ls -l
total 16
-rw-r--r--  1 kenji  staff  706 Nov 17 06:27 Welcome.php
-rw-r--r--  1 kenji  staff  141 Mar 17  2023 index.html
lrwxr-xr-x  1 kenji  staff    8 Nov 17 06:29 views -> ../views
	public function index()
	{
		$this->load->helper('file');
		var_dump(get_filenames(__DIR__));
	}
array(17) {
  [0] =>
  string(10) "index.html"
  [1] =>
  string(10) "index.html"
  [2] =>
  string(19) "welcome_message.php"
  [3] =>
  string(10) "index.html"
  [4] =>
  string(10) "index.html"
  [5] =>
  string(13) "error_php.php"
  [6] =>
  string(17) "error_general.php"
  [7] =>
  string(13) "error_404.php"
  [8] =>
  string(19) "error_exception.php"
  [9] =>
  string(12) "error_db.php"
  [10] =>
  string(10) "index.html"
  [11] =>
  string(13) "error_php.php"
  [12] =>
  string(17) "error_general.php"
  [13] =>
  string(13) "error_404.php"
  [14] =>
  string(19) "error_exception.php"
  [15] =>
  string(12) "error_db.php"
  [16] =>
  string(11) "Welcome.php"
}

@kenjis
Copy link
Member

kenjis commented Nov 16, 2023

In CI4 develop:

$ ls -l
total 16
-rw-r--r--  1 kenji  staff  1579 Oct 27 16:48 BaseController.php
-rw-r--r--  1 kenji  staff   161 Nov 16 10:10 Home.php
lrwxr-xr-x  1 kenji  staff     8 Nov 17 06:31 Views -> ../Views
    public function index()
    {
        helper('filesystem');
        dd(get_filenames(APPPATH . 'Controllers/'));
    }
┌──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ get_filenames(...)                                                                                                                                       │
└──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
array (3) [
    0 => string (18) "BaseController.php"
    1 => string (8) "Home.php"
    2 => string (5) "Views"
]
════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════

@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Nov 16, 2023
@kenjis kenjis changed the title allow get_filenames to follow symlinks fix: allow get_filenames to follow symlinks Nov 16, 2023
@kenjis
Copy link
Member

kenjis commented Nov 16, 2023

@colethorsen Thank you. This is a bug. Symlink folders also should be followed.

But this changes the behavior, so can you add a short description for it in the changelog?
https://github.com/codeigniter4/CodeIgniter4/blob/develop/user_guide_src/source/changelogs/v4.4.4.rst#breaking
And add a note something like "Prior to v4.4.4, due to a bug, this function did not follow symlink folders." in the get_filenames():
https://github.com/codeigniter4/CodeIgniter4/blob/develop/user_guide_src/source/helpers/filesystem_helper.rst#available-functions

Also we expect all code changes or bug-fixes to be accompanied by one or more tests added to our test suite to prove the code works. Can you add test code?

@kenjis kenjis added breaking change Pull requests that may break existing functionalities docs needed Pull requests needing documentation write-ups and/or revisions. tests needed Pull requests that need tests labels Nov 16, 2023
@kenjis kenjis changed the title fix: allow get_filenames to follow symlinks fix: allow get_filenames() to follow symlinks Nov 16, 2023
@colethorsen
Copy link
Contributor Author

Also we expect all code changes or bug-fixes to be accompanied by one or more tests added to our test suite to prove the code works. Can you add test code?

It appears that vfsStream does not support testing symlinks bovigo/vfsStream#89 if you have any thoughts on how this should be tested alternatively let me know.

@kenjis kenjis removed the docs needed Pull requests needing documentation write-ups and/or revisions. label Nov 16, 2023
@kenjis
Copy link
Member

kenjis commented Nov 16, 2023

--- a/tests/system/Helpers/FilesystemHelperTest.php
+++ b/tests/system/Helpers/FilesystemHelperTest.php
@@ -396,6 +396,31 @@ final class FilesystemHelperTest extends CIUnitTestCase
         $this->assertSame([], get_filenames(SUPPORTPATH . 'Files/shaker/'));
     }
 
+    public function testGetFilenamesWithSymlinks(): void
+    {
+        $targetDir = APPPATH . 'Language';
+        $linkDir   = APPPATH . 'Controllers/Language';
+        unlink($linkDir);
+        symlink($targetDir, $linkDir);
+
+        $targetFile = APPPATH . 'Common.php';
+        $linkFile   = APPPATH . 'Controllers/Common.php';
+        unlink($linkFile);
+        symlink($targetFile, $linkFile);
+
+        $this->assertSame([
+            0 => 'BaseController.php',
+            1 => 'Common.php',
+            2 => 'Home.php',
+            3 => 'Language',
+            4 => 'Validation.php',
+            5 => 'en',
+        ], get_filenames(APPPATH . 'Controllers'));
+
+        unlink($linkDir);
+        unlink($linkFile);
+    }
+
     public function testGetDirFileInfo(): void
     {
         $file = SUPPORTPATH . 'Files/baker/banana.php';

@kenjis
Copy link
Member

kenjis commented Nov 22, 2023

Can you add a test?

@kenjis kenjis added the waiting for info Issues or pull requests that need further clarification from the author label Nov 22, 2023
Copy link

github-actions bot commented Dec 2, 2023

👋 Hi, @colethorsen!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

@github-actions github-actions bot added the stale Pull requests with conflicts label Dec 2, 2023
@kenjis
Copy link
Member

kenjis commented Dec 7, 2023

Closed by #8298

@kenjis kenjis closed this Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them stale Pull requests with conflicts tests needed Pull requests that need tests waiting for info Issues or pull requests that need further clarification from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants