Skip to content

[TypeDeclaration] Clean up get ClassReflection on loop on TypedPropertyFromCreateMockAssignRector#6181

Merged
TomasVotruba merged 6 commits intomainfrom
clean-up-get-classreflection-on-loop
Jul 24, 2024
Merged

[TypeDeclaration] Clean up get ClassReflection on loop on TypedPropertyFromCreateMockAssignRector#6181
TomasVotruba merged 6 commits intomainfrom
clean-up-get-classreflection-on-loop

Conversation

@samsonasik
Copy link
Copy Markdown
Member

per review on #6179 (review)

@samsonasik
Copy link
Copy Markdown
Member Author

All checks have passed 🎉 @TomasVotruba it is ready for review.

@samsonasik samsonasik changed the title [TypeDeclaration] Clean up get ClassReflection on loop on rules [TypeDeclaration] Clean up get ClassReflection on loop on TypedPropertyFromCreateMockAssignRector Jul 24, 2024
@samsonasik
Copy link
Copy Markdown
Member Author

I cleaned up ClassReflection usage only on TypedPropertyFromCreateMockAssignRector as unnecessary on it, with direct use of AssignToPropertyTypeInferer

@samsonasik
Copy link
Copy Markdown
Member Author

All checks have passed 🎉 @TomasVotruba I think it is ready.


$type = $this->allAssignNodePropertyTypeInferer->inferProperty(
$propertyName = (string) $this->getName($property);
$type = $this->assignToPropertyTypeInferer->inferPropertyInClassLike(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks 👍

The assignToPropertyTypeInferer should be removed as well, to avoid checking all the possible types and assigns.
This rule should be as simple as possible.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

if no check on the assignments on other methods, then, reset on other method will cause error, as it can't detect nullability, this ensure nullability or reset by other type applied,eg:

protected function setUp()
{
   $this->a = $this->createMock(Foo::class);
}

protected function reset()
{
    $this->a = null;
}

if we check only on setUp(), that will cause it only detect as MockObject, which wrong

Copy link
Copy Markdown
Member Author

@samsonasik samsonasik Jul 24, 2024

Choose a reason for hiding this comment

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

the only apply MockObject or ?MockObject verify already checked on later line 121 on this PR or 133 on main branch

if (! $this->isObjectType($propertyType, new ObjectType(self::MOCK_OBJECT_CLASS))) {
continue;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I added test for skip with non-mock object type, eg: skip string 298f24c

Copy link
Copy Markdown
Member

@TomasVotruba TomasVotruba Jul 24, 2024

Choose a reason for hiding this comment

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

I see, let's go with this version for now then. Thanks 👍

@TomasVotruba TomasVotruba merged commit 1aef755 into main Jul 24, 2024
@TomasVotruba TomasVotruba deleted the clean-up-get-classreflection-on-loop branch July 24, 2024 07:48
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.

2 participants