Skip to content

[Php74][Php80] Handle TypedPropertyRector + StringableForToStringRector#1473

Merged
TomasVotruba merged 16 commits intomainfrom
fix-6862
Dec 13, 2021
Merged

[Php74][Php80] Handle TypedPropertyRector + StringableForToStringRector#1473
TomasVotruba merged 16 commits intomainfrom
fix-6862

Conversation

@samsonasik
Copy link
Copy Markdown
Member

@samsonasik samsonasik commented Dec 13, 2021

Given the following code:

declare(strict_types=1);

namespace Rector\Core\Tests\Issues\StringableTypedProperty\Fixture;

class Fixture
{
    private $nickname;

    public function __toString()
    {
        return $this->nickname;
    }

    public function getNickname(): ?string
    {
        return $this->nickname;
    }
}

It produce error:

1) Rector\Core\Tests\Issues\StringableTypedProperty\StringableTypedPropertyTest::test with data set #0 (Symplify\SmartFileSystem\SmartFileInfo Object (...))
PHPStan\ShouldNotHappenException: Cannot create PHPStan\Type\UnionType with: string, string|null

This PR fix it. Fixes rectorphp/rector#6862

@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented Dec 13, 2021

/cc @mkopinsky

@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented Dec 13, 2021

Fixed 🎉 The solution is only apply typed property to make nullable:

-    private $nickname;
+    private ?string $nickname = null;

But not apply implements Stringable as the type returned to __toString() is not string.

@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented Dec 13, 2021

@mkopinsky it seems in PHP 8.0, when class has method __toString(), it must return string, without return string, it will got error:

Fatal error: Uncaught TypeError: Fixture::__toString(): Return value must be of type string, null returned in /in/dhmYZ:7
Stack trace:
#0 /in/dhmYZ(11): Fixture->__toString()

ref https://3v4l.org/dhmYZ

@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented Dec 13, 2021

It seems the solution is find return statement in the classmethod with __toString() method, if not found, return empty string '', if found and not string, cast to (string), eg:

-return $this->nickname;
+return (string) $this->nickname;

@samsonasik samsonasik marked this pull request as draft December 13, 2021 07:40
@mkopinsky
Copy link
Copy Markdown

mkopinsky commented Dec 13, 2021

That makes perfect sense.

In the application I took this from, $this->nickname in practice is never null, which is why I haven't hit that fatal error. I guess the other alternative (which is beyond the scope of rector, but is an option for me) is to actually change the column (via the annotation) to be not null.

@samsonasik samsonasik marked this pull request as ready for review December 13, 2021 08:12
@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented Dec 13, 2021

@mkopinsky implemented 🎉

@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented Dec 13, 2021

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

@TomasVotruba
Copy link
Copy Markdown
Member

TomasVotruba commented Dec 13, 2021

Thank you, looks good 👍

@TomasVotruba TomasVotruba merged commit 08208e3 into main Dec 13, 2021
@TomasVotruba TomasVotruba deleted the fix-6862 branch December 13, 2021 11:57
samsonasik
samsonasik commented Dec 13, 2021
View reviewed changes
rules/Php80/Rector/Class_/StringableForToStringRector.php
$hasReturn = $this->betterNodeFinder->hasInstancesOfInFunctionLikeScoped($toStringClassMethod, Return_::class);

if (! $hasReturn) {
$lastKey = array_key_last((array) $toStringClassMethod->stmts);
Copy link
Copy Markdown
Member Author

@samsonasik samsonasik Dec 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

it seems cause downgrade error:

https://github.com/rectorphp/rector-src/runs/4505677100?check_suite_focus=true#step:14:80

-            $lastKey = array_key_last((array) $toStringClassMethod->stmts);
+            end((array) $toStringClassMethod->stmts);
+            $lastKey = key((array) $toStringClassMethod->stmts);

DowngradeArrayKeyFirstLastRector seems needs update for casting condition.

Copy link
Copy Markdown
Member Author

@samsonasik samsonasik Dec 13, 2021

Choose a reason for hiding this comment

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

I created PR #1475 for it. It next need a tweak in DowngradeArrayKeyFirstLastRector to adapt this condition.

Copy link
Copy Markdown
Member

@TomasVotruba TomasVotruba Dec 13, 2021

Choose a reason for hiding this comment

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

Thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TomasVotruba TomasVotruba TomasVotruba left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

StringableForToStringRector + TypedPropertyRector + string|null = ERROR

4 participants

@samsonasik @mkopinsky @TomasVotruba @actions-user