8000 [Finder] Handle filtering of recursive iterators and use it to skip looping over excluded directories by nicolas-grekas · Pull Request #15802 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Finder] Handle filtering of recursive iterators and use it to skip looping over excluded directories #15802

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[Finder] Handle filtering of recursive iterators and use it to skip looping over excluded directories #15802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions src/Symfony/Component/Finder/Adapter/PhpAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,13 @@ public function searchInDirectory($dir)
$flags |= \RecursiveDirectoryIterator::FOLLOW_SYMLINKS;
}

$iterator = new \RecursiveIteratorIterator(
new Iterator\RecursiveDirectoryIterator($dir, $flags, $this->ignoreUnreadableDirs),
\RecursiveIteratorIterator::SELF_FIRST
);
$iterator = new Iterator\RecursiveDirectoryIterator($dir, $flags, $this->ignoreUnreadableDirs);

if ($this->exclude) {
$iterator = new Iterator\ExcludeDirectoryFilterIterator($iterator, $this->exclude);
}

$iterator = new \RecursiveIteratorIterator($iterator, \RecursiveIteratorIterator::SELF_FIRST);

if ($this->minDepth > 0 || $this->maxDepth < PHP_INT_MAX) {
$iterator = new Iterator\DepthRangeFilterIterator($iterator, $this->minDepth, $this->maxDepth);
Expand All @@ -44,10 +47,6 @@ public function searchInDirectory($dir)
$iterator = new Iterator\FileTypeFilterIterator($iterator, $this->mode);
}

if ($this->exclude) {
$iterator = new Iterator\ExcludeDirectoryFilterIterator($iterator, $this->exclude);
}

if ($this->names || $this->notNames) {
$iterator = new Iterator\FilenameFilterIterator($iterator, $this->names, $this->notNames);
}
Expand Down
12 changes: 11 additions & 1 deletion src/Symfony/Component/Finder/Iterator/FilterIterator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,18 @@
*
* @author Alex Bogomazov
*/
abstract class FilterIterator extends \FilterIterator
abstract class FilterIterator extends \FilterIterator implements \RecursiveIterator
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure implementing RecursiveIterator on all out filters is a good idea. Most of our filters are required to be applied on a flattened iterator to work properly.

What about using a child class RecursiveFilterIterator which would become the parent of the filters applied in a recursive way ?

Copy link
Member

Choose a reason for hiding this comment

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

this would also avoid the need to memoize the return value in all other filters

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it's necessary: implementing inner recursive iterator is just a new feature of the class, but it's opt-in. If you don't use it as a recursive iterator (which nobody does since it didn't implement the interface) then its the same as before. Now, if one uses it as a recursive iterator, then it works. I see no need for more.

Copy link
Member Author

Choose a reason for hiding this comment

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

The return value is not memoized in all other filters, but in filters where having a recursive inner iterator could make sense. The filter on the contents of a file for example doesn't memoize.

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 the issue is that this makes it harder to distinguish filters which can be applied in a recursive way and filters which simply cannot be applied this way because it would change their behavior.

For instance, the PathFilterIterator should never be applied as a recursive iterator: it would mean that rejecting a folder would also reject all files in it (because of the behavior of RecursiveIteratorIterator which does not look for children of excluded elements).
Implementing RecursiveIterator on these filter iterators would be misleading for people looking at them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Memoization removed, this should fix your concern

Copy link
Member

Choose a reason for hiding this comment

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

memoization was not the culprit (it was simply useless). The issue is marking filter iterators as being RecursiveIterator while using them in a recursive way provides a broken behavior (it will exclude too much things)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not broken at all, it's just a new behavior that didn't exist before and can be changed easily by wrapping the injected recursive iterator in a RecursiveIteratorIterator. I see no issue here. To get the new behavior is really opt-in: inject a recursive iterator, and use this one as a recursive iterator also. I wouldn't say anyone will do this by mistake.

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 but having an iterator configured to exclude /Tests$/ and having it exclude .../Tests/foo.php would be confusing.
The filtering iterators are meant to implement the filters provided in the finder component, and implementing RecursiveIterator should be done only on filters which are actually providing their expected behavior in a recursive filtering IMO. Otherwise it would be too easy for someone to break the behavior in the future by moving filters around and getting one before the RecursiveIteratorIterator by mistake

Copy link
Member Author

Choose a reason for hiding this comment

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

But what if that's exactly what I want, excluding /Tests$/ and having it exclude .../Tests/foo.php?
This gives more power to the user, and we wouldn't force what can or can't be done with the iterators.
More power means more way to get things wrong of course... and I see no issue with that.

{
public function hasChildren()
{
return $this->getInnerIterator() instanceof \RecursiveIterator && $this->getInnerIterator()->hasChildren();
}

public function getChildren()
{
return $this->getInnerIterator()->getChildren();
}

/**
* This is a workaround for the problem with \FilterIterator leaving inner \FilesystemIterator in wrong state after
* rewind in some cases.
Expand Down
0