Skip to content

[Php74] Skip ObjectProphecy type on TypedPropertyRector#1358

Merged
TomasVotruba merged 4 commits into
mainfrom
close-6843
Dec 1, 2021
Merged

[Php74] Skip ObjectProphecy type on TypedPropertyRector#1358
TomasVotruba merged 4 commits into
mainfrom
close-6843

Conversation

@samsonasik

Copy link
Copy Markdown
Member

@samsonasik

Copy link
Copy Markdown
Member Author

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

Comment on lines +157 to 159
if ($this->objectTypeAnalyzer->isSpecial($varType)) {
return null;
}

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.

Here should be check for PHP version and IntersectionType or UnionType instead.

E.g. the fixture above should work on PHP 8.1 as it's intersection type.

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.

it is not about usage of UnionType, it is about usage of ObjectProphecy in types found, can be UnionType when has null default value, if no default value, it will be object type of ObjectProphecy

@samsonasik samsonasik Dec 1, 2021

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.

On php 8.x feature enabled, without this check, it will produce this for UnionType nullable:

-    private $obj = null;
+    private ?\Prophecy\Prophecy\ObjectProphecy $obj = null;

or object type:

-    private $obj;
+    private \Prophecy\Prophecy\ObjectProphecy $obj;

which incorrect, as it need to be special:

/** @var ReturnString|ObjectProphecy */

as original issue described at rectorphp/rector#6843

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.

The type should be ReturnString|ObjectProphecy. Somewhere we loose the ReturnString type.

This is correct result:

private ReturnString|ObjectProphecy|null $obj = null;

As a bonus, type might be contextually resolved as never-null, since the setUp() method is always run in PHPUnit test.

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.

That maybe because of it uses the $this->prophesize() call return type so it goes to ObjectProphecy type. I will check if it can fallback to the type based on the @var if it has ObjectProphecy type.

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.

I see, that might be difficult to resolve with incorrect 3rd party doc types. Let's go with current solution then 👍

@TomasVotruba TomasVotruba merged commit bf80d32 into main Dec 1, 2021
@TomasVotruba TomasVotruba deleted the close-6843 branch December 1, 2021 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

[TypedPropertyRector] Invalid refactoring for Prophecy's ObjectProphecy

2 participants