Files/FileList: adding the same file twice should not increment `FileList::$numFiles` #1150

rodrigoprimo · 2025-06-25T22:22:37Z

Description

This PR fixes the FileList::__construct() and FileList::addFile() methods to ensure that, when there is an attempt to add the same file twice, the FileList::$numFiles variable is not incremented. This behavior was causing a fatal error when using the --parallel option and passing the same file twice (see #1113).

The code was already handling duplicates correctly in the sense that duplicated files were not added to FileList::$files. However, FileList::$numFiles was being incorrectly incremented.

This PR also includes some basic tests for FileList::__construct() and FileList::addFile() in separate commits.

Suggested changelog entry

Fixed: fatal error when using the --parallel option and passing the same file twice

Related issues/external references

Fixes #1113

Additional notes

There is some duplicated logic in FileList::__construct() and FileList::addFile(). I considered refactoring the duplicated code to a private method before adding the check that is added in this commit. I decided not to do it as there are some subtle differences between the logic in the two methods that make the refactor not straightforward.

FileList::__construct() always sets the value of an entry in the FileList::$files to null and the key to whatever is returned by SplFileInfo::getPathname(). While FileList::addFile() sets the value of an entry in the FileList::$files to null or an object passed to the method, and the key to the path passed to the method.

I decided to leave this refactor to remove the duplication to the future and focus this commit on fixing the issue with handling duplicated files. I believe it will be necessary to write additional tests for this class and also incorporate more defensive code.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
- This change is only breaking for integrators, not for external standards or end-users.
Documentation improvement

PR checklist

I have checked there is no other PR open for the same change.
I have read the Contribution Guidelines.
I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
I have added tests to cover my changes.
I have verified that the code complies with the projects coding standards.
[Required for new sniffs] I have added XML documentation for the sniff.

jrfnl

Thanks @rodrigoprimo, nice work!

I've left some small comments and questions in various places, nothing really blocking.

Only real question I have is with regards to the solution applied in the third commit versus the suggestion I left in #1113 (comment)

I've now started wondering if $numFiles shouldn't be set just once at the end of the __construct() method via a count($this->files) instead of doing the incrementing whenever a new file is being added... ? I'd expect that would solve the issue too.

While I can see that, as the addFile() method is public, this wouldn't solve anything for addFile() when not called from the constructor, I do imagine the above suggestion could simplify the code change for the constructor itself.
Was there a particular reason why you went with a different solution ?

jrfnl · 2025-06-27T00:09:12Z

tests/Core/Files/FileList/AddFileTest.php

+/**
+ * Tests for the \PHP_CodeSniffer\Files\FileList::addFile method.
+ *
+ * @copyright 2019-2025 PHPCSStandards Contributors


Where does the "2019-2025" come from ? (here and elsewhere)

I copied the line @copyright 2019-2024 PHPCSStandards Contributors from another file and, without thinking too much about it, updated it to 2025 since this is the current year.

It is not clear to me if the years apply to just this file or to the whole repository. If it is just this file, should it be?

@copyright 2025 PHPCSStandards Contributors

I can see that phpDocumentor leaves the format of the description of the @copyright tag to each individual project (https://docs.phpdoc.org/guide/references/phpdoc/tags/copyright.html). Maybe this is documented somewhere in this repo, and I missed it?

It is not clear to me if the years apply to just this file or to the whole repository. If it is just this file, should it be?

Well, it's a bit messy in this repository, so generally speaking, you can assume it applies to the specific file. I still intend to look into streamlining this at some point, but there may be legal implications of doing that, so something I want to be careful with.

@copyright 2025 PHPCSStandards Contributors is what I would have expected for this new file.

Done. I opted to make this change in two separate commits (one for ConstructTest.php and FilterDouble.php, and another for AddFileTest.php) to hopefully make it easier when squashing these changes into the original commits.

tests/Core/Files/FileList/FilterDouble.php

tests/Core/Files/FileList/ConstructTest.php

tests/Core/Files/FileList/AddFileTest.php

tests/Core/Files/FileList/ConstructTest.php

jrfnl · 2025-06-27T00:26:00Z

tests/Core/Files/FileList/ConstructTest.php

+
+        $this->assertCount(count($expectedFiles), $fileList, 'File count mismatch');
+
+        $i = 0;


Does the $i have value in the error messages ? I mean, the array is being sorted by key anyway and the $expectedFiles does not have explicit keys.

My idea is that it would help identify which file caused the assertion to fail by providing the index of the corresponding value in the $expectedFiles array. But I can see that this is not very straightforward. Perhaps it would be better to modify the error message to include the $filePath instead of $i??

I like that idea. Sounds more descriptive to me.

jrfnl · 2025-06-27T02:01:08Z

tests/Core/Files/FileList/ConstructTest.php

+    /**
+     * Setup objects needed for the tests.
+     *
+     * @before


@before or could/should this be @beforeClass ?

I have a vague memory of us discussing that creating the Ruleset class in the tests has a performance impact. That is why I went with the mock of this class. I don't think I'm able to create a mock of Ruleset if I use @beforeClass.

That being said, I'm not sure what is better: creating an instance of ConfigDouble and a mock of Ruleset for every single test, or creating an instance of ConfigDouble and an instance of Ruleset once for all the tests.

Is this more or less what you had in mind with this question?

Considering you need both for the data provider anyway.... I imagine there is some optimization which can now be applied....

I'm not entirely sure if I understand what you're hinting at, but I gave it a try and added two new commits (one for ConstructTest.php and another for AddFileTest.php), making both test classes use an initializeConfigAndRuleset() method that uses the @beforeClass tag.

For AddFileTest, the Ruleset class is still instantiated twice, as initializeConfigAndRuleset() needs to be called directly inside dataAddFile().

What I was hinting at was that both the a beforeClass method and the data provider need to be static methods - and need to write to static properties, so the beforeClass method could check if the properties had been initialized and if not, could create the objects and the data provider could call the beforeClass method to do the same and then in both the tests as well as the data provider, the statically stored objects can be re-used.

Thanks for the additional details, Juliette! I have now implemented this change.

tests/Core/Files/FileList/AddFileTest.php

jrfnl · 2025-06-27T02:05:59Z

tests/Core/Files/FileList/AddFileTest.php

+            'STDIN'                         => ['fileName' => 'STDIN'],
+            'Regular file with file object' => [
+                'fileName'   => 'test1.php',
+                'fileObject' => new stdClass(),


This should create a File object, not a stdClass object. As things are, this will break in PHPCS 4.0 when the type declarations are added (addFile(string $path, ?File $file=null)).

Good catch! I added a commit creating an instance of the File class. To do that, I had to instantiate both ConfigDouble and Ruleset. Let me know if you had a different approach in mind.

rodrigoprimo · 2025-06-27T15:04:03Z

Thanks for your review, @jrfnl!

Only real question I have is with regards to the solution applied in the third commit versus the suggestion I left in #1113 (comment)

I've now started wondering if $numFiles shouldn't be set just once at the end of the __construct() method via a count($this->files) instead of doing the incrementing whenever a new file is being added... ? I'd expect that would solve the issue too.

While I can see that, as the addFile() method is public, this wouldn't solve anything for addFile() when not called from the constructor, I do imagine the above suggestion could simplify the code change for the constructor itself.
Was there a particular reason why you went with a different solution ?

I did consider your suggestion, but noticed that it did not apply to addFile(). What I failed to realize, is that I could apply it only to the __construct() method. I have now added a new commit simplifying how FileList::$numFiles is calculated in the constructor.

Besides that, I addressed all the other comments. Could you please take another look when you get a chance?

Also, before merging this PR, let me know if you prefer to squash the commits yourself or if you would like me to do it.

rodrigoprimo · 2025-06-27T17:52:03Z

Rebased without changes to fix the failing remark test. I forgot to do it earlier for this PR.

jrfnl · 2025-06-30T16:48:38Z

@rodrigoprimo As I'm going to need to look at the complete thing again before merging this, feel free to reorganize the commits already when you make your final changes.

This is just an initial set of tests. It does not fully cover the FileList::__construct() method.

This is just an initial set of tests. It does not fully cover the FileList::addFile() method.

…List::$numFiles` This commit fixes the `FileList::__construct()` and `FileList::addFile()` methods to ensure that, when there is an attempt to add the same file twice, the `FileList::$numFiles` variable is not incremented. The code was already handling duplicates correctly in the sense that duplicated files were not added to `FileList::$files`. However, `FileList::$numFiles` was being incorrectly incremented. There is some duplicated logic in `FileList::__construct()` and `FileList::addFile()`. I considered refactoring the duplicated code to a private method before adding the check that is added in this commit. I decided not to do it as there are some subtle differences between the logic in the two methods. `FileList::__construct()` always sets the value of an entry in the `FileList::$files` to `null` and the key to whatever is returned by `SplFileInfo::getPathname()`. While `FileList::addFile()` sets the value of an entry in the `FileList::$files` to `null` or an object passed to the method and the key to the path passed to the method. I decided to leave this refactor to remove the duplication to the future and focus this commit on fixing the issue with handling duplicated files.

rodrigoprimo · 2025-06-30T18:41:18Z

@rodrigoprimo As I'm going to need to look at the complete thing again before merging this, feel free to reorganize the commits already when you make your final changes.

Done, this PR is now ready for a final review. Thanks.

github-actions bot added Type: bug Core Component: File Status: triage labels Jun 25, 2025

rodrigoprimo removed the Status: triage label Jun 25, 2025

rodrigoprimo force-pushed the fix-file-list-duplicate-files branch 2 times, most recently from 0b0430a to befb62b Compare June 25, 2025 23:20

jrfnl reviewed Jun 27, 2025

View reviewed changes

jrfnl added the Status: awaiting feedback label Jun 27, 2025

rodrigoprimo force-pushed the fix-file-list-duplicate-files branch from 4b3b6ec to 78ab8ac Compare June 27, 2025 15:02

rodrigoprimo removed the Status: awaiting feedback label Jun 27, 2025

rodrigoprimo force-pushed the fix-file-list-duplicate-files branch from 78ab8ac to d958137 Compare June 27, 2025 17:51

jrfnl added the Status: awaiting feedback label Jun 28, 2025

rodrigoprimo removed the Status: awaiting feedback label Jun 30, 2025

rodrigoprimo added 3 commits June 30, 2025 15:21

Files/FileList: add basic tests for the FileList::__construct() method

f08b64a

This is just an initial set of tests. It does not fully cover the FileList::__construct() method.

Files/FileList: add basic tests for the FileList::addFile() method

279788b

This is just an initial set of tests. It does not fully cover the FileList::addFile() method.

rodrigoprimo force-pushed the fix-file-list-duplicate-files branch from 185e384 to 5165510 Compare June 30, 2025 18:26


		$this->assertCount(count($expectedFiles), $fileList, 'File count mismatch');

		$i = 0;

Uh oh!

Files/FileList: adding the same file twice should not increment FileList::$numFiles #1150

Are you sure you want to change the base?

Files/FileList: adding the same file twice should not increment FileList::$numFiles #1150

Uh oh!

Conversation

rodrigoprimo commented Jun 25, 2025

Description

Suggested changelog entry

Related issues/external references

Additional notes

Types of changes

PR checklist

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodrigoprimo commented Jun 27, 2025

Uh oh!

rodrigoprimo commented Jun 27, 2025

Uh oh!

jrfnl commented Jun 30, 2025

Uh oh!

rodrigoprimo commented Jun 30, 2025

Uh oh!

Uh oh!

Files/FileList: adding the same file twice should not increment `FileList::$numFiles` #1150

Files/FileList: adding the same file twice should not increment `FileList::$numFiles` #1150