Skip to content

[Arrows] MC-21844: SVC false-positive: short names & FQCN in DockBlock #51

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

Merged
merged 3 commits into from
Sep 21, 2020

Conversation

zakdma
Copy link
Contributor

@zakdma zakdma commented Sep 17, 2020

Scope

Bug - P1

  • MC-21844 SVC false-positive: short names & FQCN in DockBlock

Bamboo CI builds

Related Pull Requests

Created by : @zakdma

@zakdma
Copy link
Contributor Author

zakdma commented Sep 17, 2020

@roribio could you please take a look at our PR

@zakdma zakdma changed the title MC-21844: SVC false-positive: short names & FQCN in DockBlock [Arrows] MC-21844: SVC false-positive: short names & FQCN in DockBlock Sep 17, 2020
@roribio roribio requested a review from vkublytskyi September 17, 2020 14:42
{
$return = parent::enterNode($node);

if ($node instanceof ClassMethod) {

Choose a reason for hiding this comment

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

Property, ClassConst also may have annotation that describes a type. For Magento this is a less important cases and may be addressed in separate PRs.

if ($docNode) {
$result = [];
/** @var PhpDocTagNode[] $paramTags */
$paramTags = $docNode->getTagsByName('@param');

Choose a reason for hiding this comment

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

Note: getParamTagValues returns node with more suitable API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

}

/** @var PhpDocTagNode[] $returnTags */
$returnTags = $docNode->getTagsByName('@return');

Choose a reason for hiding this comment

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

Note: getReturnTagValues returns node with more suitable API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

$resolvedType = $this->resolveType($normalizedType);
$result[] = $resolvedType;
}
} else {

Choose a reason for hiding this comment

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

Note: Please ensure that PHPStan\PhpDocParser\Ast\Type\NullableTypeNode should not be handled as special case (equals to union null|type)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

?type is parsed as NullableType
type|null is parsed as UnionType
So I handle only UnionType case as list of FullyQualified types because there can be also annotation like that
Product|int|null

Copy link

@vkublytskyi vkublytskyi left a comment

Choose a reason for hiding this comment

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

Please fix unit test

@zakdma
Copy link
Contributor Author

zakdma commented Sep 18, 2020

@vkublytskyi all fixed except constants and variable types parsing
It can be done in separate fix for that cases

Copy link
Contributor

@roribio roribio left a comment

Choose a reason for hiding this comment

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

Approved 💯

@roribio roribio requested review from vkublytskyi and removed request for vkublytskyi September 21, 2020 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants