-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[Finder] Fix Finder::append() breaking generic typing contract
#62762
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
base: 6.4
Are you sure you want to change the base?
Conversation
2ace1fc to
3381080
Compare
| $file->getPathname(), | ||
| $file->getPath(), | ||
| $file->getPathname(), |
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.
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"
}
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.
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:
->in()does not have to be called (only->append()used)->in()can be passed multiple directories, so then we would have to arbitrarily choose which one to use for the relative path
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.
->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
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.
->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.
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.
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.
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.
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);|
Please add a meaningful PR title, thanks |
Finder::append() breaking generic typing contract
|
The |
nicolas-grekas
left a comment
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.
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']))); |
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.
| $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']))); |
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.
| $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']))); |
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.
@nicolas-grekas , maybe let's formalise this?
#62783
| 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(), |
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.
| 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(), |
| self::$tmpDir . '/foo' => self::$tmpDir . '/foo' | ||
| ], | ||
| array_map( | ||
| fn (SplFileInfo $item) => $item->getRelativePathname(), |
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.
| 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(), |
|
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 patchdiff --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(); |
Updated Finder::append() to ensure the iterable items are SplFileInfo (symfony's version).