Skip to content

Commit eb8c379

Browse files
rodrigoprimorodrigoprimo
committed
Files/FileList: adding the same file twice should not increment FileList::$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.
1 parent ae1e31e commit eb8c379

File tree

3 files changed

+81
-9
lines changed

3 files changed

+81
-9
lines changed

‎src/Files/FileList.php‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,14 @@ public function __construct(Config $config, Ruleset $ruleset)
9191
$iterator = new RecursiveIteratorIterator($filter);
9292

9393
foreach ($iterator as $file) {
94-
$this->files[$file->getPathname()] = null;
94+
$pathname = $file->getPathname();
95+
96+
if (array_key_exists($pathname, $this->files) === true) {
97+
// The path has already been added.
98+
continue;
99+
}
100+
101+
$this->files[$pathname] = null;
95102
$this->numFiles++;
96103
}
97104
} else {
@@ -132,6 +139,11 @@ public function addFile($path, $file=null)
132139
$iterator = new RecursiveIteratorIterator($filter);
133140

134141
foreach ($iterator as $path) {
142+
if (array_key_exists($path, $this->files) === true) {
143+
// The path has already been added.
144+
continue;
145+
}
146+
135147
$this->files[$path] = $file;
136148
$this->numFiles++;
137149
}

‎tests/Core/Files/FileList/AddFileTest.php‎

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,9 @@ protected function initializeFileList()
4141
$config = new ConfigDouble();
4242
$config->filter = __DIR__.'/FilterDouble.php';
4343
$config->files = [];
44-
$ruleset = $this->createStub('PHP_CodeSniffer\Ruleset');
44+
$ruleset = $this->getMockBuilder('PHP_CodeSniffer\Ruleset')
45+
->disableOriginalConstructor()
46+
->getMock();
4547
$this->fileList = new FileList($config, $ruleset);
4648

4749
}//end initializeFileList()
@@ -100,4 +102,36 @@ public static function dataAddFile()
100102
}//end dataAddFile()
101103

102104

105+
/**
106+
* Test that it is not possible to add the same file twice.
107+
*
108+
* @return void
109+
*/
110+
public function testAddFileShouldNotAddTheSameFileTwice()
111+
{
112+
$file1 = 'test1.php';
113+
$file2 = 'test2.php';
114+
$expectedFiles = [
115+
$file1,
116+
$file2,
117+
];
118+
119+
// Add $file1 once.
120+
$this->fileList->addFile($file1);
121+
$this->assertCount(1, $this->fileList);
122+
$this->assertSame([$file1], array_keys(iterator_to_array($this->fileList)));
123+
124+
// Try to add $file1 again. Should be ignored.
125+
$this->fileList->addFile($file1);
126+
$this->assertCount(1, $this->fileList);
127+
$this->assertSame([$file1], array_keys(iterator_to_array($this->fileList)));
128+
129+
// Add $file2. Should be added.
130+
$this->fileList->addFile($file2);
131+
$this->assertCount(2, $this->fileList);
132+
$this->assertSame($expectedFiles, array_keys(iterator_to_array($this->fileList)));
133+
134+
}//end testAddFileShouldNotAddTheSameFileTwice()
135+
136+
103137
}//end class

‎tests/Core/Files/FileList/ConstructTest.php‎

Lines changed: 33 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -46,13 +46,15 @@ protected function initializeConfigAndRuleset()
4646
{
4747
$this->config = new ConfigDouble();
4848
$this->config->filter = __DIR__.'/FilterDouble.php';
49-
$this->ruleset = $this->createStub('PHP_CodeSniffer\Ruleset');
49+
$this->ruleset = $this->getMockBuilder('PHP_CodeSniffer\Ruleset')
50+
->disableOriginalConstructor()
51+
->getMock();
5052

5153
}//end initializeConfigAndRuleset()
5254

5355

5456
/**
55-
* Test basic construction with file paths.
57+
* Test the __construct() method.
5658
*
5759
* @param array<string> $files List of file paths in the Config class.
5860
* @param array<string> $expectedFiles List of expected file paths in the FileList.
@@ -74,7 +76,12 @@ public function testConstruct($files, $expectedFiles)
7476

7577
$i = 0;
7678

77-
foreach ($fileList as $filePath => $fileObject) {
79+
// Sort the value to make the tests stable as different OSes will read directories
80+
// in a different order and the order is not relevant for these tests. Just the values.
81+
$fileListArray = iterator_to_array($fileList);
82+
ksort($fileListArray);
83+
84+
foreach ($fileListArray as $filePath => $fileObject) {
7885
$this->assertSame($expectedFiles[$i], $filePath, 'File path mismatch at index '.$i);
7986
$this->assertInstanceOf('PHP_CodeSniffer\Files\File', $fileObject, 'File object mismatch at index '.$i);
8087
$i++;
@@ -86,16 +93,16 @@ public function testConstruct($files, $expectedFiles)
8693
/**
8794
* Data provider for testConstruct.
8895
*
89-
* @return array<string, array<string>>
96+
* @return array<string, array<string|null>>
9097
*/
9198
public static function dataConstruct()
9299
{
93100
return [
94-
'No files' => [
101+
'No files' => [
95102
'files' => [],
96103
'expectedFiles' => [],
97104
],
98-
'Two files' => [
105+
'Two files' => [
99106
'files' => [
100107
'file1.php',
101108
'file2.php',
@@ -105,11 +112,30 @@ public static function dataConstruct()
105112
'file2.php',
106113
],
107114
],
108-
'A directory' => [
115+
'A directory' => [
109116
'files' => [__DIR__.'/Fixtures'],
110117
'expectedFiles' => [
118+
__DIR__.'/Fixtures/file1.php',
111119
__DIR__.'/Fixtures/file2.php',
120+
],
121+
],
122+
'Same file twice' => [
123+
'files' => [
124+
'file1.php',
125+
'file1.php',
126+
],
127+
'expectedFiles' => [
128+
'file1.php',
129+
],
130+
],
131+
'File and then directory containing that file' => [
132+
'files' => [
112133
__DIR__.'/Fixtures/file1.php',
134+
__DIR__.'/Fixtures',
135+
],
136+
'expectedFiles' => [
137+
__DIR__.'/Fixtures/file1.php',
138+
__DIR__.'/Fixtures/file2.php',
113139
],
114140
],
115141
];

0 commit comments

Comments
 (0)