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

PHPStan: Fixed handling of union types in ConfigReturnTypeExtension #11312

Merged
merged 3 commits into from Feb 10, 2023

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Feb 9, 2023

see https://phpstan.org/blog/why-is-instanceof-type-wrong-and-getting-deprecated

after this fix the PHPStan extension supports cases like

if (..) {
  $property = 'license';
} else {
  $property = 'description';
}

$value = Config::get($property);

which it did not before

Comment on lines 73 to 74
// for compat with old phpstan versions, we use a deprecated phpstan method.
$strings = TypeUtils::getConstantStrings($keyType);
Copy link
Contributor Author

@staabm staabm Feb 9, 2023

Choose a reason for hiding this comment

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

refactored to use the deprecated TypeUtils::getConstantStrings method, since composer itself does not specify a min-phpstan version requirement.

using the suggested Type->getConstantStrings() variant is available as of PHPStan 1.9.4

if ($keyType instanceof ConstantStringType) {
if (isset($this->properties[$keyType->getValue()])) {
return $this->properties[$keyType->getValue()];
// for compat with old phpstan versions, we use a deprecated phpstan method.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this should do a version check or a method_exists check to use the new API when available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, fixed

if ($strings !== []) {
$types = [];
foreach($strings as $string) {
if (isset($this->properties[$string->getValue()])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any string not being known here should force skipping the more precise type, falling back to the basic method signature

Copy link
Contributor Author

Choose a reason for hiding this comment

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

adressed

@@ -64,18 +65,32 @@ public function getTypeFromMethodCall(MethodReflection $methodReflection, Method
{
$args = $methodCall->getArgs();

$defaultReturn = ParametersAcceptorSelector::selectSingle($methodReflection->getVariants())->getReturnType();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in case we would have a min phpstan version of 1.5.0 or higher, the method could just return null and the phpstan-core would take care of determining the default-return type.

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to assume latest PHPStan and bump the require if you like :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this won't work. it depends on the phpstan version used by the project which depends on composer.

the extension is meant for re-use in plugin projects. this means I would either need to declare a non-dev dependency on phpstan or add a conflict. both things I don't like.

Copy link
Member

Choose a reason for hiding this comment

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

Oh right I forgot that aspect, let's leave as is then 👍🏻

@staabm staabm marked this pull request as ready for review February 9, 2023 16:35
@Seldaek Seldaek merged commit d8221bd into composer:main Feb 10, 2023
@Seldaek
Copy link
Member

Seldaek commented Feb 10, 2023

Thanks

@Seldaek Seldaek added this to the 2.6 milestone Feb 10, 2023
@staabm staabm deleted the phpstan-fix branch February 10, 2023 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants