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

[DowngradePHP74] Fix parent removal regression #1081

Merged
merged 3 commits into from
Oct 27, 2021

Conversation

TomasVotruba
Copy link
Member

@TomasVotruba TomasVotruba commented Oct 27, 2021

Fixed regression in parent type resolution in #1071

Closes rectorphp/rector#6772 bug

@samsonasik
Copy link
Member

@TomasVotruba this is same with failing PR #1080 . The use case is different, as in the rector build, the : parent actually removed entirely, but it is probably related and need to be skipped.

@TomasVotruba
Copy link
Member Author

the : parent actually removed entirely

I think this happens in cascade by the next rule taking over alraedy broken code.
I'll see how can I fix this one first.

@samsonasik
Copy link
Member

The \Symfony\Component\String\AbstractString::class, may need to be removed from build/config/config-downgrade.php like in PR https://github.com/rectorphp/rector-src/pull/1080/files#diff-c8125c36a958b829ffd79f1774dc9649d2ef21b044e4d849225960bfed613a80

Comment on lines 86 to 93
return new ParentStaticType($classReflection);
$parentClassReflection = $classReflection->getParentClass();
if (! $parentClassReflection instanceof ClassReflection) {
throw new ShouldNotHappenException();
}

return new ParentStaticType($parentClassReflection);
Copy link
Member Author

Choose a reason for hiding this comment

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

@samsonasik Here was the bug probably. The parent did not refere to parent class, but self class.

-return new ParentStaticType($classReflection);
+return new ParentStaticType($parentClassReflection);

Copy link
Member

Choose a reason for hiding this comment

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

@TomasVotruba thank you 👍

@TomasVotruba TomasVotruba changed the title [DowngradePHP74] Add test case for parent removal bug [DowngradePHP74] Fix parent removal regression Oct 27, 2021
@TomasVotruba TomasVotruba merged commit 697c6e7 into main Oct 27, 2021
@TomasVotruba TomasVotruba deleted the tv-keep-parent-reference branch October 27, 2021 10:51
@samsonasik
Copy link
Member

@TomasVotruba I tried locally, it seems still not working, with different error:

➜  rector-src git:(tv-keep-parent-reference) ✗ cd rector-prefixed-downgraded 
➜  rector-prefixed-downgraded git:(tv-keep-parent-reference) ✗ cp ../build/target-repository/bootstrap.php .
➜  rector-prefixed-downgraded git:(tv-keep-parent-reference) ✗ cp ../preload.php .
➜  rector-prefixed-downgraded git:(tv-keep-parent-reference) ✗ bin/rector list --ansi
Rector 2215b0e85570ca53d450bf83c20cf3629e3d7680

Usage:
  command [options] [arguments]

PHP Fatal error:  Declaration of RectorPrefix20211027\Symfony\Component\String\UnicodeString::join($strings, $lastGlue = null) must be compatible with RectorPrefix20211027\Symfony\Component\String\AbstractUnicodeString::join($strings, $lastGlue = null): RectorPrefix20211027\Symfony\Component\String\AbstractString in /Users/samsonasik/www/rector-src/rector-prefixed-downgraded/vendor/symfony/string/UnicodeString.php on line 169
Fatal error: Declaration of RectorPrefix20211027\Symfony\Component\String\UnicodeString::join($strings, $lastGlue = null) must be compatible with RectorPrefix20211027\Symfony\Component\String\AbstractUnicodeString::join($strings, $lastGlue = null): RectorPrefix20211027\Symfony\Component\String\AbstractString in /Users/samsonasik/www/rector-src/rector-prefixed-downgraded/vendor/symfony/string/UnicodeString.php on line 169

@samsonasik
Copy link
Member

@samsonasik
Copy link
Member

@TomasVotruba the AbstractString::join() return self seems need to be kept, and the child UnicodeString::join() return AbstractString seems need to be kept as well.

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.

rector build error
2 participants