-
Notifications
You must be signed in to change notification settings - Fork 71
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
Add Composer\InstalledVersions
in symbol whitelist
#218
Conversation
…ime-api:^2 required + minor CheckCommandTest refactoring
86495d1
to
0ed547f
Compare
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.
LGTM - requires feedback from author, but seems to be really good/correct fix overall.
Technical (architectural) issues were still found, and need discussion.
@@ -93,6 +95,14 @@ protected function execute(InputInterface $input, OutputInterface $output): int | |||
) | |||
) | |||
); | |||
|
|||
if ( |
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.
Overall, the approach seems much more correct now, as it correctly appends Composer\InstalledVersions
to the declared "found" symbols.
The high level behavior is correct, but we're still operating in the wrong code unit.
I'm fine with merging this code as-is by adding a @TODO
, but if you look above, the sources do come from:
$getAdditionalSourceFiles($options->getScanFiles(), dirname($composerJson)),
$getPackageSourceFiles($composerData, dirname($composerJson)),
(new LocateComposerPackageDirectDependenciesSourceFiles())->__invoke($composerJson)
I think we need to define and call $getComposerPluginApi($composerJson)
in that section (by adding a parameter and a dedicated class that is only responsible for this dependency check).
What's your opinion?
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 made some refactoring and moved code that work with composerData to the dedicated locator class.
I can't use LocateASTFromFiles
and LocateDefinedSymbolsFromASTRoots
for get Composer\InstalledVersions
from InstalledVersions.php because it contains use Composer\Semver\VersionParser
and composer/semver
may not be required in composer.json. I originally thought the same as you, but have got a message about undefined symbol Composer\Semver\VersionParser
.
Also I see that you write about composer-plugin-api, but I work in this PR with composer-runtime-api. Should I change code and process the both packages?
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.
@Ocramius could you give me a feedback about the last comment?
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.
This works for me and is more than sufficient, thank you! 👍
de64aea
to
f40b41b
Compare
…about `Composer\InstalledVersions`
f40b41b
to
83092c3
Compare
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.
@a-menshchikov super happy with the approach taken so far, thank you so much! 👍
Composer\InstalledVersions
in symbol whitelist
Fixes #217.