-
-
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.
|
Please add a meaningful PR title, thanks |
Finder::append() breaking generic typing contract
|
The |
Updated Finder::append() to ensure the iterable items are SplFileInfo (symfony's version).