-
Notifications
You must be signed in to change notification settings - Fork 72
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
Use roave/better-reflection to guess packages #346
base: 4.1.x
Are you sure you want to change the base?
Changes from 2 commits
8b97812
e8cfe80
876cc15
a092bf6
eee2904
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace ComposerRequireChecker\DependencyGuesser; | ||
|
||
use Roave\BetterReflection\BetterReflection; | ||
use Roave\BetterReflection\Identifier\Identifier; | ||
use Roave\BetterReflection\Identifier\IdentifierType; | ||
use Roave\BetterReflection\Reflection\ReflectionClass; | ||
use Roave\BetterReflection\Reflector\DefaultReflector; | ||
use Roave\BetterReflection\SourceLocator\Type\Composer\Factory\MakeLocatorForInstalledJson; | ||
use Generator; | ||
use Roave\BetterReflection\SourceLocator\Type\MemoizingSourceLocator; | ||
use Roave\BetterReflection\SourceLocator\Type\SourceLocator; | ||
use function preg_match; | ||
use function preg_quote; | ||
use function sprintf; | ||
use function str_replace; | ||
use const DIRECTORY_SEPARATOR; | ||
|
||
final class GuessFromInstalledComposerPackages implements Guesser | ||
{ | ||
private SourceLocator $sourceLocator; | ||
private string $pathRegex; | ||
|
||
public function __construct(string $installationPath) | ||
{ | ||
$this->sourceLocator = new MemoizingSourceLocator((new MakeLocatorForInstalledJson())( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This locator does not include PHP symbols There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just realized that this is potentially also broken in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not quite sure but dont we have for the php internal stuff the other guesser? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Possibly, but BetterReflection can also reflect all of that, heh :D There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would say, follow up PR, if wanted. |
||
$installationPath, | ||
(new BetterReflection())->astLocator() | ||
)); | ||
|
||
$cleanPath = preg_quote(sprintf('%s/vendor', str_replace(DIRECTORY_SEPARATOR, '/', $installationPath)), '@'); | ||
$this->pathRegex = sprintf('@^%s/(?:composer/\.\./)?([^/]+/[^/]+)/@', $cleanPath); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure for this regex tbh. Is there a way to get the name from the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Getting the name from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there already a way to get the name from the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. each There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So i reused now the regex but to get the composer.json and load it via the JsonLoader already present in this lib. Im not sure if this is now really better since we are still using a regex but also have now more I/O which impacts performance. But since this is a CI tool i think the performance aspect could be overlooked. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mentioned by @DanielBadura in #346 (comment) - hacky as heck, so we may need to check this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about this now quite some time and I didnt came up with another solution :/ |
||
} | ||
|
||
/** | ||
* @return Generator<string> | ||
*/ | ||
public function __invoke(string $symbolName): Generator | ||
{ | ||
$reflection = $this->sourceLocator->locateIdentifier( | ||
new DefaultReflector($this->sourceLocator), | ||
new Identifier($symbolName, new IdentifierType(IdentifierType::IDENTIFIER_CLASS)) | ||
DanielBadura marked this conversation as resolved.
Show resolved
Hide resolved
|
||
); | ||
|
||
if (!($reflection instanceof ReflectionClass)) { | ||
return; | ||
} | ||
|
||
$path = $reflection->getFileName(); | ||
|
||
if (preg_match($this->pathRegex, $path, $captures)) { | ||
yield $captures[1]; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace ComposerRequireCheckerTest\DependencyGuesser; | ||
|
||
use ComposerRequireChecker\DependencyGuesser\GuessFromInstalledComposerPackages; | ||
use PHPUnit\Framework\TestCase; | ||
|
||
final class GuessFromInstalledComposerPackagesTest extends TestCase | ||
{ | ||
private GuessFromInstalledComposerPackages $guesser; | ||
|
||
protected function setUp(): void | ||
{ | ||
$this->guesser = new GuessFromInstalledComposerPackages(dirname(__DIR__, 3)); | ||
} | ||
|
||
public function testGuessVendorClass(): void | ||
{ | ||
$result = ($this->guesser)(TestCase::class); | ||
|
||
self::assertNotEmpty($result); | ||
self::assertContains('phpunit/phpunit', $result); | ||
} | ||
|
||
public function testDoNotGuessVendorFunction(): void | ||
{ | ||
$result = iterator_to_array(($this->guesser)('DeepCopy\deep_copy')); | ||
|
||
self::assertEmpty($result); | ||
} | ||
|
||
|
||
public function testDoNotGuessClassFromProject(): void | ||
{ | ||
$result = iterator_to_array(($this->guesser)(GuessFromInstalledComposerPackages::class)); | ||
|
||
self::assertEmpty($result); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's avoid introducing mutability here: instead, let's use a new constructor argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional or not: the BC break from using
roave/better-reflection
by default will probably be notable anyway, so we can potentially break BC here, and release in5.0.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain why adding the dependency would lead to a BC break?
Yeah, would go for the argument too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially because scanning behavior changes => new symbols detected (or missed) => CI failure for consumers that upgrade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I misunderstand: But we are not detecting here more / new symbols, we just give here a hint which dependency should be installed to get rid of the warning, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the mutability but still i dont get why we already break BC with using better-reflection. Maybe you could explain it more?