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.

@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?

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.

5 participants