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

[rodrigoprimo]

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 ?

[rodrigoprimo]

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]

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

[jrfnl]

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

[rodrigoprimo]

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

[jrfnl]

Thanks for making those updates @rodrigoprimo!

Just one small suggestion to prevent head scratching in the future. Otherwise this looks good to go.

[rodrigoprimo]

Thanks, @jrfnl. Makes sense to me to add this comment. I just approved your suggestion.

[jrfnl]

Thanks @rodrigoprimo ! I'll squash that last commit into the first and will merge this once the build has passed.