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

[Php80] Handle parent with typed param on AddParamBasedOnParentClassMethodRector #1455

Merged
merged 9 commits into from
Dec 11, 2021

Conversation

samsonasik
Copy link
Member

@samsonasik samsonasik commented Dec 11, 2021

When parent class has typed param:

class A
{
    public function foo(int $foo)
    {
        
    }
}

class B extends A
{
    public function foo()
    {
        
    }
}

It currently produce internal php-parser error:

1) Rector\Tests\Php80\Rector\ClassMethod\AddParamBasedOnParentClassMethodRector\AddParamBasedOnParentClassMethodRectorTest::test with data set #10 (Symplify\SmartFileSystem\SmartFileInfo Object (...))
assert($startPos >= 0 && $endPos >= 0) in /Users/samsonasik/www/rector-src/vendor/nikic/php-parser/lib/PhpParser/PrettyPrinterAbstract.php:535

this PR try to solve it.

@samsonasik
Copy link
Member Author

I think the solution is when parent has type, child should not has type so it safe to apply, ref https://3v4l.org/4Mgrt

@samsonasik
Copy link
Member Author

When user wants it to work on php 7.1 too, there is DowngradeParameterTypeWideningRector rule for it to remove type hint in parent.

@samsonasik
Copy link
Member Author

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

@samsonasik samsonasik force-pushed the handle-parent-with-typed-param branch from 63c817d to d7d69a7 Compare December 11, 2021 12:06
@samsonasik
Copy link
Member Author

rebased.

@TomasVotruba
Copy link
Member

When user wants it to work on php 7.1 too, there is DowngradeParameterTypeWideningRector rule for it to remove type hint in parent.

That would not work now, as the interface must be explicitly mentioned as unsafe.

@TomasVotruba
Copy link
Member

assert($startPos >= 0 && $endPos >= 0) in /Users/samsonasik/www/rector-src/vendor/nikic/php-parser/lib/PhpParser/PrettyPrinterAbstract.php:535

These errors usually means that node reprint of original tokens failed. PhpParser tried to keep original formatting and guesses the original position of tokens. Sometimes the node changes, but tokens don't refresh.

The $node->set(AttributeKey::ORIGINAL_NODE, null) on related node usually helps.

@samsonasik
Copy link
Member Author

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

Note:

@TomasVotruba
Copy link
Member

I've added commit that include the original node example that I meant. See if it's ok

@samsonasik
Copy link
Member Author

@TomasVotruba thank you, that solve it, I moved to separate method to fix phpstan a200007

@samsonasik samsonasik closed this Dec 11, 2021
@samsonasik samsonasik reopened this Dec 11, 2021
@samsonasik
Copy link
Member Author

Close-reopen to restart CI build

@samsonasik
Copy link
Member Author

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

@TomasVotruba TomasVotruba merged commit 7fbf604 into main Dec 11, 2021
@TomasVotruba TomasVotruba deleted the handle-parent-with-typed-param branch December 11, 2021 13:47
@TomasVotruba
Copy link
Member

Thank you 👍

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.

3 participants