Skip to content
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.

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

Fix parsing of ambiguous classes to avoid ignoring real classes when test ones get scanned first #13

Merged
merged 1 commit into from
Oct 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions phpstan-baseline.neon
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,8 @@ parameters:
message: "#^Call to static method PHPUnit\\\\Framework\\\\Assert\\:\\:assertArrayHasKey\\(\\) with 'A' and array\\<class\\-string, array\\<non\\-empty\\-string\\>\\> will always evaluate to false\\.$#"
count: 1
path: tests/ClassMapGeneratorTest.php

-
message: "#^Offset 'A' does not exist on non\\-empty\\-array\\<class\\-string, array\\<non\\-empty\\-string\\>\\>\\.$#"
count: 4
path: tests/ClassMapGeneratorTest.php
31 changes: 29 additions & 2 deletions src/ClassMap.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@

namespace Composer\ClassMapGenerator;

use Composer\Pcre\Preg;

/**
* @author Jordi Boggiano <[email protected]>
*/
Expand Down Expand Up @@ -71,11 +73,36 @@ public function getPsrViolations(): array
*
* To get the path the class is being mapped to, call getClassPath
*
* By default, paths that contain test(s), fixture(s), example(s) or stub(s) are ignored
* as those are typically not problematic when they're dummy classes in the tests folder.
* If you want to get these back as well you can pass false to $duplicatesFilter. Or
* you can pass your own pattern to exclude if you need to change the default.
*
* @param non-empty-string|false $duplicatesFilter
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not using null for the absence of filter ? That would be a cleaner API

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I started as filter true/false, then figured I could let people customize the filter, and the false stuck. A bit late now that it's been tagged, but it's not really a broadly used API so not such a big deal.

*
* @return array<class-string, array<non-empty-string>>
*/
public function getAmbiguousClasses(): array
public function getAmbiguousClasses($duplicatesFilter = '{/(test|fixture|example|stub)s?/}i'): array
{
return $this->ambiguousClasses;
if (false === $duplicatesFilter) {
return $this->ambiguousClasses;
}

if (true === $duplicatesFilter) {
throw new \InvalidArgumentException('$duplicatesFilter should be false or a string with a valid regex, got true.');
}

$ambiguousClasses = [];
foreach ($this->ambiguousClasses as $class => $paths) {
$paths = array_filter($paths, function ($path) use ($duplicatesFilter) {
return !Preg::isMatch($duplicatesFilter, strtr($path, '\\', '/'));
});
if (\count($paths) > 0) {
$ambiguousClasses[$class] = array_values($paths);
}
}

return $ambiguousClasses;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/ClassMapGenerator.php
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ public function scanPaths($path, ?string $excluded = null, string $autoloadType
foreach ($classes as $class) {
if (!$this->classMap->hasClass($class)) {
$this->classMap->addClass($class, $filePath);
} elseif ($filePath !== $this->classMap->getClassPath($class) && !Preg::isMatch('{/(test|fixture|example|stub)s?/}i', strtr($this->classMap->getClassPath($class).' '.$filePath, '\\', '/'))) {
} elseif ($filePath !== $this->classMap->getClassPath($class)) {
$this->classMap->addAmbiguousClass($class, $filePath);
}
}
Expand Down
37 changes: 29 additions & 8 deletions tests/ClassMapGeneratorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -186,9 +186,11 @@ public function testUnambiguousReference(): void
{
$tempDir = self::getUniqueTmpDirectory();

file_put_contents($tempDir . '/A.php', "<?php\nclass A {}");
mkdir($tempDir.'/src');
mkdir($tempDir.'/ambiguous');
file_put_contents($tempDir . '/src/A.php', "<?php\nclass A {}");
file_put_contents(
$tempDir . '/B.php',
$tempDir . '/src/B.php',
"<?php
if (true) {
interface B {}
Expand All @@ -199,17 +201,36 @@ interface B extends Iterator {}
);

foreach (array('test', 'fixture', 'example') as $keyword) {
if (!is_dir($tempDir . '/' . $keyword)) {
mkdir($tempDir . '/' . $keyword, 0777, true);
if (!is_dir($tempDir . '/ambiguous/' . $keyword)) {
mkdir($tempDir . '/ambiguous/' . $keyword, 0777, true);
}
file_put_contents($tempDir . '/' . $keyword . '/A.php', "<?php\nclass A {}");
file_put_contents($tempDir . '/ambiguous/' . $keyword . '/A.php', "<?php\nclass A {}");
}

$this->generator->scanPaths($tempDir);
// if we scan src first, then test ambiguous refs will be ignored correctly
$this->generator->scanPaths($tempDir.'/src');
$this->generator->scanPaths($tempDir.'/ambiguous');
$classMap = $this->generator->getClassMap();

self::assertCount(0, $classMap->getAmbiguousClasses());

// but when retrieving without filtering, the ambiguous classes are there
self::assertCount(1, $classMap->getAmbiguousClasses(false));
self::assertCount(3, $classMap->getAmbiguousClasses(false)['A']);

// if we scan tests first however, then we always get ambiguous refs as the test one is overriding src
$this->generator = new ClassMapGenerator(['php', 'inc', 'hh']);
$this->generator->scanPaths($tempDir.'/ambiguous');
$this->generator->scanPaths($tempDir.'/src');
$classMap = $this->generator->getClassMap();

// when retrieving with filtering, only the one from src is seen as ambiguous
self::assertCount(1, $classMap->getAmbiguousClasses());
self::assertCount(1, $classMap->getAmbiguousClasses()['A']);
self::assertSame($tempDir.'/src'.DIRECTORY_SEPARATOR.'A.php', $classMap->getAmbiguousClasses()['A'][0]);
// when retrieving without filtering, all the ambiguous classes are there
self::assertCount(1, $classMap->getAmbiguousClasses(false));
self::assertCount(3, $classMap->getAmbiguousClasses(false)['A']);

$fs = new Filesystem();
$fs->remove($tempDir);
}
Expand Down Expand Up @@ -291,7 +312,7 @@ public static function getUniqueTmpDirectory(): string
$root = sys_get_temp_dir();

do {
$unique = $root . DIRECTORY_SEPARATOR . uniqid('composer-test-' . random_int(1000, 9000));
$unique = $root . DIRECTORY_SEPARATOR . uniqid('composer-classmap-' . random_int(1000, 9000));

if (!file_exists($unique) && @mkdir($unique, 0777)) {
return (string) realpath($unique);
Expand Down