Skip to content

Commit

Permalink
bug #30313 Avoid mutating the Finder when building the iterator (stof)
Browse files Browse the repository at this point in the history
This PR was merged into the 3.4 branch.

Discussion
----------

Avoid mutating the Finder when building the iterator

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

When excluding dot files or vcs files (which is done by default), the Finder object was mutated each time `searchInDirectory` was called to register the extra exclusions. This leads to registering them multiple times when the method is called multiple times (which happens either because you have multiple directories in `->in()` or because you call `getIterator` multiple times, for instance because of using `hasResults` or `count`).

This mutation create bugs if the Finder object is reconfigured between the 2 calls to `getIterator` to disable some of these ignore rules, as they would already be registered in the other config properties. New tests have been added to reproduce these bugs and prevent regressions.

This mutation is now avoided by using a local array for the final configuration, preserving the user configuration.

Commits
-------

94989fe Avoid mutating the Finder when building the iterator
  • Loading branch information
fabpot committed Feb 20, 2019
2 parents df0fc5e + 94989fe commit 47d26f6
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 6 deletions.
15 changes: 9 additions & 6 deletions src/Symfony/Component/Finder/Finder.php
Original file line number Diff line number Diff line change
Expand Up @@ -645,12 +645,15 @@ public function count()
*/
private function searchInDirectory($dir)
{
$exclude = $this->exclude;
$notPaths = $this->notPaths;

if (static::IGNORE_VCS_FILES === (static::IGNORE_VCS_FILES & $this->ignore)) {
$this->exclude = array_merge($this->exclude, self::$vcsPatterns);
$exclude = array_merge($exclude, self::$vcsPatterns);
}

if (static::IGNORE_DOT_FILES === (static::IGNORE_DOT_FILES & $this->ignore)) {
$this->notPaths[] = '#(^|/)\..+(/|$)#';
$notPaths[] = '#(^|/)\..+(/|$)#';
}

$minDepth = 0;
Expand Down Expand Up @@ -683,8 +686,8 @@ private function searchInDirectory($dir)

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

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

$iterator = new \RecursiveIteratorIterator($iterator, \RecursiveIteratorIterator::SELF_FIRST);
Expand Down Expand Up @@ -717,8 +720,8 @@ private function searchInDirectory($dir)
$iterator = new Iterator\CustomFilterIterator($iterator, $this->filters);
}

if ($this->paths || $this->notPaths) {
$iterator = new Iterator\PathFilterIterator($iterator, $this->paths, $this->notPaths);
if ($this->paths || $notPaths) {
$iterator = new Iterator\PathFilterIterator($iterator, $this->paths, $notPaths);
}

if ($this->sort) {
Expand Down
23 changes: 23 additions & 0 deletions src/Symfony/Component/Finder/Tests/FinderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,18 @@ public function testIgnoreVCS()
$this->assertIterator($this->toAbsolute(['foo', 'foo/bar.tmp', 'test.php', 'test.py', 'toto', '.bar', '.foo', '.foo/.bar', '.foo/bar', 'foo bar']), $finder->in(self::$tmpDir)->getIterator());
}

public function testIgnoreVCSCanBeDisabledAfterFirstIteration()
{
$finder = $this->buildFinder();
$finder->in(self::$tmpDir);
$finder->ignoreDotFiles(false);

$this->assertIterator($this->toAbsolute(['foo', 'foo/bar.tmp', 'test.php', 'test.py', 'toto', '.bar', '.foo', '.foo/.bar', '.foo/bar', 'foo bar']), $finder->getIterator());

$finder->ignoreVCS(false);
$this->assertIterator($this->toAbsolute(['.git', 'foo', 'foo/bar.tmp', 'test.php', 'test.py', 'toto', 'toto/.git', '.bar', '.foo', '.foo/.bar', '.foo/bar', 'foo bar']), $finder->getIterator());
}

public function testIgnoreDotFiles()
{
$finder = $this->buildFinder();
Expand All @@ -214,6 +226,17 @@ public function testIgnoreDotFiles()
$this->assertIterator($this->toAbsolute(['foo', 'foo/bar.tmp', 'test.php', 'test.py', 'toto', 'foo bar']), $finder->in(self::$tmpDir)->getIterator());
}

public function testIgnoreDotFilesCanBeDisabledAfterFirstIteration()
{
$finder = $this->buildFinder();
$finder->in(self::$tmpDir);

$this->assertIterator($this->toAbsolute(['foo', 'foo/bar.tmp', 'test.php', 'test.py', 'toto', 'foo bar']), $finder->getIterator());

$finder->ignoreDotFiles(false);
$this->assertIterator($this->toAbsolute(['foo', 'foo/bar.tmp', 'test.php', 'test.py', 'toto', '.bar', '.foo', '.foo/.bar', '.foo/bar', 'foo bar']), $finder->getIterator());
}

public function testSortByName()
{
$finder = $this->buildFinder();
Expand Down

0 comments on commit 47d26f6

Please sign in to comment.