[Finder] Exclude relative to content root if prefixed `/` #54752

joshtrichards · 2024-04-27T13:46:38Z

Related: #26396
Related: #9158
Related: #28410

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	Fix #28158 #47431
License	MIT

This implements a fix for this situation in a non-BC breaking way by doing content root-only relative matches when the exclude() path is prefixed by /. If an exclude() path does not start with /, the behavior stays the same as today and will exclude any matching sub-dir anywhere in the tree.

Approach first suggested here by @ogizanagi and most recently reiterated by @wouterj here.

carsonbot · 2024-04-27T13:46:44Z

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 7.1 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

Fixes symfony#28158 Fixes symfony#47431 Related: symfony#26396 Related: symfony#9158 Related: symfony#28410

joshtrichards · 2024-04-27T14:15:27Z

Both test failures appear unrelated.

src/Symfony/Component/Finder/Iterator/ExcludeDirectoryFilterIterator.php

phil-davis · 2024-04-27T17:07:29Z

I had some unit test changes ready to test this solution, so I have made PR #54754 with the test changes/additions and the code cherry-picked from here. I made it to the 7.1 branch, because, as I read the rules, this is a "new feature" and so new features go to the current minor version branch.

Is this a "new feature" or an "enhancement that helps adjust behavior"?
Will we be able to get it added to the v6 series (and maybe to the v5 series, like this PR attempts)?

smnandre · 2024-04-27T17:56:08Z

Does the notInPath method work for this ?

This will be a big BC break, as ->exclude('/var/') will have a major behaviour change, no ?

joshtrichards · 2024-04-27T21:28:44Z

Thanks for your comments. Some initial responses below.

Does the notInPath method work for this ?

Only in the same sense that notPath can always be used as an alternative to exclude if performance isn't of concern. So no. :-)

This will be a big BC break, as ->exclude('/var/') will have a major behaviour change, no ?

Paths provided to exclude are only evaluated relative to the in() specified folder(s). Absolute (system) paths were never supported by exclude.

So if someone puts a slash in front they (presumably) mean the root of their content (in()) directory. That's fine.

If that's not what they intended, well that's never been supported (or worked - and still won't).

i.e.in($contentRoot)->exclude("/var") will match $contentRoot/var just fine.

So if someone is specifying /var/ they're already likely not getting what they're expecting. Either they meant:

the system /var/ which never worked and still won't work

Or:

a folder called var located in their $contentRoot (i.e. within the root of in($contentRoot)) and only there

That second situation is what this would fix/enable.

You can play with the regex a bit (keeping in mind the new one is only used against exclude values that start with / - which is how this is isn't breaking BC):

Old and only: #(?:^|/)(?:data|/blah|/var/)(?:/|$)#

New and supplemental: #^(data|/blah|/var/)$#

Aside: The actual bug is in the original regex and/or in which scenarios even end up using it I suppose[1], but fixing this would not be BC. This PR does so in a reasonable way (I think!) without breaking existing code. It makes exclude useful outside of anything but the simplest folder structures (which is important since its main benefit over notPath is performance and that is likely to be of particular interest to those with big/complicated trees, but we can't use it as-is right now without risking losing deeper down paths that match undesirably).

[1]

symfony/src/Symfony/Component/Finder/Iterator/ExcludeDirectoryFilterIterator.php

Lines 56 to 84 in c834064

    
                       $directory = rtrim($directory, '/'); 
        
                       if (!$this->isRecursive || str_contains($directory, '/')) { 
        
                           $patterns[] = preg_quote($directory, '#'); 
        
                       } else { 
        
                           $this->excludedDirs[$directory] = true; 
        
                       } 
        
                   } 
        
                   if ($patterns) { 
        
                       $this->excludedPattern = '#(?:^|/)(?:'.implode('|', $patterns).')(?:/|$)#'; 
        
                   } 
        
                   parent::__construct($iterator); 
        
               } 
        
               /** 
        
                * Filters the iterator values. 
        
                */ 
        
               public function accept(): bool 
        
               { 
        
                   if ($this->isRecursive && isset($this->excludedDirs[$this->getFilename()]) && $this->isDir()) { 
        
                       return false; 
        
                   } 
        
                   if ($this->excludedPattern) { 
        
                       $path = $this->isDir() ? $this->current()->getRelativePathname() : $this->current()->getRelativePath(); 
        
                       $path = str_replace('\\', '/', $path); 
        
                       return !preg_match($this->excludedPattern, $path); 
        
                   }

smnandre · 2024-04-27T23:31:42Z

Thank you for the detailed answer. I guess adding some functional tests (testing the behaviour with the recursiveiterator) is not a bad idea :)

phil-davis · 2024-04-28T01:39:17Z

Thank you for the detailed answer. I guess adding some functional tests (testing the behaviour with the recursiveiterator) is not a bad idea :)

See the PR #54754 that has this code to the 7.1 branch, where I have expanded the test coverage. I'm happy to hear ideas for other combinations of things that should have test cases.

(Note: "someone" will need to decide if this is a fix or a new feature. That will determine if this will be allowed as a "patch" fix in v5 and v6 or whether it is only released to v7)

Co-authored-by: Oskar Stark <oskarstark@googlemail.com>

Seldaek · 2024-05-02T15:52:14Z

IMO this would be very nice to have as a bug fix, because the advertised behavior was broken.

As an aside, we are trying to add support for this in Composer's autoloader which uses Finder to iterate files for classmaps, as this would allow skipping whole directories instead of iterating all files anyway like we do now, gaining perf, but I have no way of converting an exclude-from-classmap: ['Tests/'] into a finder exclusion right now, as excluding 'Tests' would exclude src/Tests as well, potentially wreaking havoc. Ref composer/class-map-generator#7

phil-davis · 2024-05-02T15:55:45Z

IMO this would be very nice to have as a bug fix, because the advertised behavior was broken.

Agree. I am happy to help sort out "back-porting" the test changes to here for 5.4, and making a "mid-port" PR for major version 6. "Someone" just decide if this will be accepted as a bug fix.

Signed-off-by: Josh Richards <josh.t.richards@gmail.com>

joshtrichards · 2024-05-08T21:24:15Z

Added tests:

The new iterator unit tests all pass { /foo, /toto/foo }
The finder integration test for /foo passes, but there's an issue with sub-folders /toto/foo (they're not being excluded when the iterator is called thru Finder)

Technically sub-folders don't have to work I suppose for now, but I'd like to see if I can get them to work too since it'll feel like a more correct implementation. I'll try to take a look at those in the coming days. If anyone sees the culprit, don't hesitate to speak up (I have a few guesses but don't have more time to look at it tonight).

…solution at the moment Signed-off-by: Josh Richards <josh.t.richards@gmail.com>

javiereguiluz · 2024-08-09T15:11:14Z

This looks like an important thing to fix (and we have another fix proposal in #54754). Let's ping @symfony/mergers to see if we can work on finishing this one. Thanks.

phil-davis · 2024-08-09T15:17:10Z

This looks like an important thing to fix (and we have another fix proposal in #54754). Let's ping @symfony/mergers to see if we can work on finishing this one. Thanks.

My contribution there is just to expand the test coverage to try and cover more cases. And I made the PR to a later branch, in case "those that have the power" decide that it has to go in the later branch.

How to we get the attention of the Symfony maintainers?

raveren · 2025-05-14T08:28:46Z

Carson the bot is threatening to close the OP issue and there seems to be failing checks in this PR?

I think this is still relevant, @joshtrichards do you feel that fixing the failing tests and code style issues is possible?

carsonbot added Status: Needs Review Bug Finder labels Apr 27, 2024

carsonbot added this to the 5.4 milestone Apr 27, 2024

fix(Finder): ExcludeDirectoryFilterIterator relative to "root" if /

d685583

Fixes symfony#28158 Fixes symfony#47431 Related: symfony#26396 Related: symfony#9158 Related: symfony#28410

joshtrichards force-pushed the fix-28158-exclude-root branch from 3aeedb6 to d685583 Compare April 27, 2024 13:54

joshtrichards changed the title ~~[Finder] Exclude relative to content root if suffixed /~~ [Finder] Exclude relative to content root if prefixed / Apr 27, 2024

This was referenced Apr 27, 2024

Finder excluded does NOT exclude relative directories #47431

Open

There is no way to exclude path only from root directory #28158

Open

phil-davis mentioned this pull request Apr 27, 2024

[Finder] Exclude relative to content root if prefixed / #54754

Open

OskarStark reviewed Apr 27, 2024

View reviewed changes

src/Symfony/Component/Finder/Iterator/ExcludeDirectoryFilterIterator.php Outdated Show resolved Hide resolved

Spacing

ca2ca8b

Co-authored-by: Oskar Stark <oskarstark@googlemail.com>

Seldaek mentioned this pull request May 2, 2024

feat: ClassMapGenerator::scanPaths can exclude specific dirs composer/class-map-generator#7

Merged

Seldaek mentioned this pull request May 3, 2024

Optimize exclude-from-classmap composer/composer#11962

Draft

joshtrichards added 5 commits May 8, 2024 16:38

Add tests for Finder exclude root absolute

70e0ac4

Signed-off-by: Josh Richards <josh.t.richards@gmail.com>

coding standards update

9f70375

Signed-off-by: Josh Richards <josh.t.richards@gmail.com>

add test for absolute underneath a subfolder

6912c9a

Signed-off-by: Josh Richards <josh.t.richards@gmail.com>

additional test adjustments

5b49aa6

Signed-off-by: Josh Richards <josh.t.richards@gmail.com>

add test for sub-root subdir

5b47349

Signed-off-by: Josh Richards <josh.t.richards@gmail.com>

fixup: exclude non-root absolute paths since not supported with this …

992c48f

…solution at the moment Signed-off-by: Josh Richards <josh.t.richards@gmail.com>

fabpot modified the milestones: 5.4, 6.4 Dec 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Finder] Exclude relative to content root if prefixed `/` #54752

[Finder] Exclude relative to content root if prefixed `/` #54752

joshtrichards commented Apr 27, 2024

Uh oh!

carsonbot commented Apr 27, 2024

Uh oh!

joshtrichards commented Apr 27, 2024

Uh oh!

Uh oh!

phil-davis commented Apr 27, 2024

Uh oh!

smnandre commented Apr 27, 2024

Uh oh!

joshtrichards commented Apr 27, 2024

Uh oh!

smnandre commented Apr 27, 2024

Uh oh!

phil-davis commented Apr 28, 2024

Uh oh!

Seldaek commented May 2, 2024

Uh oh!

phil-davis commented May 2, 2024

Uh oh!

joshtrichards commented May 8, 2024

Uh oh!

javiereguiluz commented Aug 9, 2024

Uh oh!

phil-davis commented Aug 9, 2024

Uh oh!

raveren commented May 14, 2025

Uh oh!

Uh oh!

Uh oh!

[Finder] Exclude relative to content root if prefixed / #54752

Are you sure you want to change the base?

[Finder] Exclude relative to content root if prefixed / #54752

Conversation

joshtrichards commented Apr 27, 2024

Uh oh!

carsonbot commented Apr 27, 2024

Uh oh!

joshtrichards commented Apr 27, 2024

Uh oh!

Uh oh!

phil-davis commented Apr 27, 2024

Uh oh!

smnandre commented Apr 27, 2024

Uh oh!

joshtrichards commented Apr 27, 2024

Uh oh!

smnandre commented Apr 27, 2024

Uh oh!

phil-davis commented Apr 28, 2024

Uh oh!

Seldaek commented May 2, 2024

Uh oh!

phil-davis commented May 2, 2024

Uh oh!

joshtrichards commented May 8, 2024

Uh oh!

javiereguiluz commented Aug 9, 2024

Uh oh!

phil-davis commented Aug 9, 2024

Uh oh!

raveren commented May 14, 2025

Uh oh!

Uh oh!

[Finder] Exclude relative to content root if prefixed `/` #54752

[Finder] Exclude relative to content root if prefixed `/` #54752