Skip to content

Conversation

@jack-worman
Copy link
Contributor

@jack-worman jack-worman commented Dec 12, 2025

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #62734
License MIT

Updated Finder::append() to ensure the iterable items are SplFileInfo (symfony's version).

@carsonbot carsonbot added this to the 6.4 milestone Dec 12, 2025
@carsonbot carsonbot changed the title Fix-GH-62734 Fix-GH-62734 Dec 12, 2025
@jack-worman jack-worman changed the title Fix-GH-62734 [Finder] Fix-GH-62734 Dec 12, 2025
Comment on lines +721 to +723
$file->getPathname(),
$file->getPath(),
$file->getPathname(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not believe this produces proper results.
While the Symfony\Finder\SplFileInfo interface would now be respected, produced content is different than when using only ->in(...).

I run following script in https://github.com/PHP-CS-Fixer/PHP-CS-Fixer repo:

<?php

declare(strict_types=1);

use Symfony\Component\Finder\Finder;

include 'vendor/autoload.php';

$FILE = './src/Linter/LinterInterface.php';
$f = new \SplFileInfo($FILE);

var_dump([ # 1
    "---" => "RAW_FILE",
    "getPathname" => $f->getPathname(),
    "getPath" => $f->getPath(),
]);

var_dump("/////");

$finder = Finder::create()
    ->in(__DIR__.'/src/')
    // ->append([$FILE]) << no, let it be found by `->in(...)`
;

$files = iterator_to_array($finder->getIterator());
$f = $files[realpath($FILE)];

var_dump([ # 2
    "::class" => get_class($f), // \Symfony\Component\Finder\SplFileInfo
    "getPathname" => $f->getPathname(),
    "getRelativePath" => $f->getRelativePath(),
    "getRelativePathname" => $f->getRelativePathname(),
]);

var_dump("/////");

$finder = Finder::create()
    ->in(__DIR__.'/src/')
    ->append([$FILE])
;

$files = iterator_to_array($finder->getIterator());
$f = $files[$FILE];

var_dump([ # 3
    "::class" => get_class($f), // \SplFileInfo
    "getPathname" => $f->getPathname(),
    "getRelativePath (mocked)" => $f->getPath(),
    "getRelativePathname (mocked)" => $f->getPathname(),
]);

as you can see, var_dump#3 is failing to recognise relativePath in context of `->in(DIR.'/src/'), while var_dump#2 is not having this problem

keradus@machine PHP-CS-Fixer_3 % php -f x.php
array(3) {
  ["---"]=>
  string(8) "RAW_FILE"
  ["getPathname"]=>
  string(32) "./src/Linter/LinterInterface.php"
  ["getPath"]=>
  string(12) "./src/Linter"
}
string(5) "/////"
array(4) {
  ["::class"]=>
  string(36) "Symfony\Component\Finder\SplFileInfo"
  ["getPathname"]=>
  string(76) "/Users/keradus/github/PHP-CS-Fixer_3/src/Linter/LinterInterface.php"
  ["getRelativePath"]=>
  string(6) "Linter"
  ["getRelativePathname"]=>
  string(26) "Linter/LinterInterface.php"
}
string(5) "/////"
array(4) {
  ["::class"]=>
  string(11) "SplFileInfo"
  ["getPathname"]=>
  string(32) "./src/Linter/LinterInterface.php"
  ["getRelativePath (mocked)"]=>
  string(12) "./src/Linter"
  ["getRelativePathname (mocked)"]=>
  string(32) "./src/Linter/LinterInterface.php"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When given just a string filepath or \SplFileInfo without any context, we cannot discern a relative path without guess work, that may not be intuitive to users.

There are problems with using ->in() to determine the relative path:

  1. ->in() does not have to be called (only ->append() used)
  2. ->in() can be passed multiple directories, so then we would have to arbitrarily choose which one to use for the relative path

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

->in() can be passed multiple directories, so then we would have to arbitrarily choose which one to use for the relative path

I would recommend mimicking current behaviour

$finder = Finder::create()
    ->in(__DIR__.'/src/')
    ->in(__DIR__.'/tests/')


$files = iterator_to_array($finder->getIterator());
$f = $files[realpath($FILE)];

$f->getRelativePath() // Linter/LinterInterface.php

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

->in() does not have to be called (only ->append() used)

agree this is the tricky part.
yet, if behaviour is different, in case we cannot unify it - it should be at least documented.

Copy link
Contributor Author

@jack-worman jack-worman Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think how it currently is makes the most sense, but I made a test that snapshots the current behavior. Please add comments telling me what the expected results should be.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this relative path aspect, it'd be best to preserve the existing behavior.
If you have a test case that fails after your patch, please attach it to the PR.
And let's figure out a fix to make it pass.

For the date-related failure, can you please add this?

--- a/src/Symfony/Component/Finder/Tests/Iterator/RealIteratorTestCase.php
+++ b/src/Symfony/Component/Finder/Tests/Iterator/RealIteratorTestCase.php
@@ -72,8 +72,9 @@ abstract class RealIteratorTestCase extends IteratorTestCase
         file_put_contents(self::toAbsolute('test.php'), str_repeat(' ', 800));
         file_put_contents(self::toAbsolute('test.py'), str_repeat(' ', 2000));

-        touch(self::toAbsolute('foo/bar.tmp'), strtotime('2005-10-15'));
-        touch(self::toAbsolute('test.php'), strtotime('2005-10-15'));
+        $oneYearAgo = strtotime('-1 year');
+        touch(self::toAbsolute('foo/bar.tmp'), $oneYearAgo);
+        touch(self::toAbsolute('test.php'), $oneYearAgo);

@OskarStark
Copy link
Contributor

Please add a meaningful PR title, thanks

@jack-worman jack-worman changed the title [Finder] Fix-GH-62734 [Finder] Fix Finder::append() breaking generic typing contract Dec 13, 2025
@jack-worman jack-worman changed the title [Finder] Fix Finder::append() breaking generic typing contract [Finder] Fix Finder::append() breaking generic typing contract Dec 13, 2025
@jack-worman
Copy link
Contributor Author

The DateRangeFilterIteratorTest::testAccept() failure seems unrelated. It fails for me even when I switch to the master 6.4 branch. Any ideas what is going on?

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I'm now seeing that testRelativePathOfAppendedItems is your failing test case 👀


$finder1->append($finder2);
$finder1->append($this->toAbsolute(['foo']));
$finder1->append(array_map(fn (string $item): \SplFileInfo => new \SplFileInfo($item), $this->toAbsolute(['toto'])));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$finder1->append(array_map(fn (string $item): \SplFileInfo => new \SplFileInfo($item), $this->toAbsolute(['toto'])));
$finder1->append(array_map(static fn ($item) => new \SplFileInfo($item), $this->toAbsolute(['toto'])));


$finder1->append($finder2);
$finder1->append($this->toAbsolute(['foo']));
$finder1->append(array_map(fn (string $item): \SplFileInfo => new \SplFileInfo($item), $this->toAbsolute(['toto'])));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$finder1->append(array_map(fn (string $item): \SplFileInfo => new \SplFileInfo($item), $this->toAbsolute(['toto'])));
$finder1->append(array_map(static fn ($item) => new \SplFileInfo($item), $this->toAbsolute(['toto'])));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicolas-grekas , maybe let's formalise this?
#62783

Comment on lines +1332 to +1341
self::$tmpDir . '/foo/bar.tmp' => 'bar.tmp',
// $finder2's ->in()
self::$tmpDir . '/qux' => 'qux',
// new \SplFileInfo('toto')
self::$tmpDir . '/toto' => self::$tmpDir . '/toto',
// 'foo'
self::$tmpDir . '/foo' => self::$tmpDir . '/foo',
],
array_map(
fn (SplFileInfo $item) => $item->getRelativePathname(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self::$tmpDir . '/foo/bar.tmp' => 'bar.tmp',
// $finder2's ->in()
self::$tmpDir . '/qux' => 'qux',
// new \SplFileInfo('toto')
self::$tmpDir . '/toto' => self::$tmpDir . '/toto',
// 'foo'
self::$tmpDir . '/foo' => self::$tmpDir . '/foo',
],
array_map(
fn (SplFileInfo $item) => $item->getRelativePathname(),
self::$tmpDir.'/foo/bar.tmp' => 'bar.tmp',
// $finder2's ->in()
self::$tmpDir.'/qux' => 'qux',
// new \SplFileInfo('toto')
self::$tmpDir.'/toto' => self::$tmpDir.'/toto',
// 'foo'
self::$tmpDir.'/foo' => self::$tmpDir.'/foo',
],
array_map(
static fn ($item) => $item->getRelativePathname(),

Comment on lines +1350 to +1353
self::$tmpDir . '/foo' => self::$tmpDir . '/foo'
],
array_map(
fn (SplFileInfo $item) => $item->getRelativePathname(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self::$tmpDir . '/foo' => self::$tmpDir . '/foo'
],
array_map(
fn (SplFileInfo $item) => $item->getRelativePathname(),
self::$tmpDir.'/foo' => self::$tmpDir.'/foo'
],
array_map(
static fn ($item) => $item->getRelativePathname(),

@nicolas-grekas
Copy link
Member

I spent quite some time on this PR trying to help, here is the patch I currently have (on top of yours). Let me know what you think of the result, please borrow it of course.

About the test case for relative paths, we should look for another one if we want to cover what @keradus raised: the test should pass on 6.4, and should fail with an improper fix. At the moment, the test fails, but it doesn't work on 6.4, which means we cannot consider it being a regression test.

My patch
diff --git a/src/Symfony/Component/Finder/Finder.php b/src/Symfony/Component/Finder/Finder.php
index 58a520851e2..f1558eeb5df 100644
--- a/src/Symfony/Component/Finder/Finder.php
+++ b/src/Symfony/Component/Finder/Finder.php
@@ -59,7 +59,6 @@ class Finder implements \IteratorAggregate, \Countable
     private int $ignore = 0;
     private array $dirs = [];
     private array $dates = [];
-    /** @var list<\Iterator<string, SplFileInfo>> */
     private array $iterators = [];
     private array $contains = [];
     private array $notContains = [];
@@ -667,27 +666,39 @@ class Finder implements \IteratorAggregate, \Countable
      */
     public function getIterator(): \Iterator
     {
-        if (0 === \count($this->dirs) && 0 === \count($this->iterators)) {
+        if (!$this->dirs && !$this->iterators) {
             throw new \LogicException('You must call one of in() or append() methods before iterating over a Finder.');
         }
 
-        if (1 === \count($this->dirs) && 0 === \count($this->iterators)) {
+        if (1 === \count($this->dirs) && !$this->iterators) {
             $iterator = $this->searchInDirectory($this->dirs[0]);
-
-            if ($this->sort || $this->reverseSorting) {
-                $iterator = (new Iterator\SortableIterator($iterator, $this->sort, $this->reverseSorting))->getIterator();
+        } else {
+            $iterator = new \AppendIterator();
+            foreach ($this->dirs as $dir) {
+                $iterator->append(new \IteratorIterator(new LazyIterator(fn () => $this->searchInDirectory($dir))));
             }
 
-            return $iterator;
-        }
-
-        $iterator = new \AppendIterator();
-        foreach ($this->dirs as $dir) {
-            $iterator->append(new \IteratorIterator(new LazyIterator(fn () => $this->searchInDirectory($dir))));
-        }
-
-        foreach ($this->iterators as $it) {
-            $iterator->append($it);
+            $yielded = [];
+
+            foreach ($this->iterators as $it) {
+                $iterator->append((static function () use ($it, &$yielded) {
+                    foreach ($it as $file) {
+                        if (!$file instanceof \SplFileInfo) {
+                            $file = new \SplFileInfo($file);
+                        }
+                        if (isset($yielded[$key = $file->getPathname()])) {
+                            continue;
+                        }
+                        if (!$file instanceof SplFileInfo) {
+                            $file = new SplFileInfo($key, $file->getPath(), $key);
+                        } else {
+                            $yielded[$key] = true;
+                        }
+
+                        yield $key => $file;
+                    }
+                })());
+            }
         }
 
         if ($this->sort || $this->reverseSorting) {
@@ -708,31 +719,7 @@ class Finder implements \IteratorAggregate, \Countable
      */
     public function append(iterable $iterator): static
     {
-        if ($iterator instanceof self) {
-            $this->iterators[] = $iterator->getIterator();
-        } else {
-            /** @var \ArrayIterator<string, SplFileInfo> $it */
-            $it = new \ArrayIterator();
-            foreach ($iterator as $file) {
-                if ($file instanceof SplFileInfo) {
-                    $it[$file->getPathname()] = $file;
-                } elseif ($file instanceof \SplFileInfo) {
-                    $it[$file->getPathname()] = new SplFileInfo(
-                        $file->getPathname(),
-                        $file->getPath(),
-                        $file->getPathname(),
-                    );
-                } else {
-                    $file = new \SplFileInfo($file);
-                    $it[$file->getPathname()] = new SplFileInfo(
-                        $file->getPathname(),
-                        $file->getPath(),
-                        $file->getPathname(),
-                    );
-                }
-            }
-            $this->iterators[] = $it;
-        }
+        $this->iterators[] = $iterator;
 
         return $this;
     }
diff --git a/src/Symfony/Component/Finder/Tests/Iterator/RealIteratorTestCase.php b/src/Symfony/Component/Finder/Tests/Iterator/RealIteratorTestCase.php
index 943c9ad0b38..dc9414a750d 100644
--- a/src/Symfony/Component/Finder/Tests/Iterator/RealIteratorTestCase.php
+++ b/src/Symfony/Component/Finder/Tests/Iterator/RealIteratorTestCase.php
@@ -72,8 +72,9 @@ abstract class RealIteratorTestCase extends IteratorTestCase
         file_put_contents(self::toAbsolute('test.php'), str_repeat(' ', 800));
         file_put_contents(self::toAbsolute('test.py'), str_repeat(' ', 2000));
 
-        touch(self::toAbsolute('foo/bar.tmp'), strtotime('2005-10-15'));
-        touch(self::toAbsolute('test.php'), strtotime('2005-10-15'));
+        $oneYearAgo = strtotime('-1 year');
+        touch(self::toAbsolute('foo/bar.tmp'), $oneYearAgo);
+        touch(self::toAbsolute('test.php'), $oneYearAgo);
 
         if (FinderTest::class === static::class) {
             $fs = new Filesystem();

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants