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] Add implements interface support on single file on AddParamBasedOnParentClassMethodRector #2660

Merged
merged 8 commits into from Jul 15, 2022

Conversation

samsonasik
Copy link
Member

Based on @MartinMystikJonas comment on rectorphp/rector#7296 (comment), which AddParamBasedOnParentClassMethodRector doesn't add param on implements interface yet.

@samsonasik
Copy link
Member Author

implemented 🎉 , it currently detect interface on same file, for interface in different file, that's probably require different PR.

@samsonasik
Copy link
Member Author

129 PHPStan errors seems unrelated:

 ------------------------------------------------------------------------
  packages/BetterPhpDocParser/Comment/CommentsMerger.php:40
 ------------------------------------------------------------------------
  - '#Parameters should use "PhpParser\\Node\\Stmt\\ClassMethod\|PhpParser\\Node\\Expr\\Array_" types as the only types passed to this method#'
  
[ERROR] Found 129 errors      

ref https://github.com/rectorphp/rector-src/runs/7345146986?check_suite_focus=true

Registered to ignoreErrors config 637c07b

@MartinMystikJonas
Copy link
Contributor

How it would work with multiple interfaces and/or parent class and interface? For params it should add parameter type that accepts union of types from all interfaces and parent class.

For return type rule I would need to create intersection of types from all interfaces and parent class.

@samsonasik
Copy link
Member Author

that's should be working if interface resolver solved, as currently will work on multiple classes https://getrector.org/demo/511766ad-5f7f-429a-898f-5d84e2b06417

flipped position still not working on multiple classes in same file as well https://getrector.org/demo/260dfe2e-eeeb-44ce-930c-da1b5f7f5690

@samsonasik samsonasik marked this pull request as draft July 14, 2022 18:23
@samsonasik
Copy link
Member Author

@MartinMystikJonas feel free to create different PR if you found a solution for it

@samsonasik samsonasik force-pushed the add-implement-interface-support branch from 932e3af to 3353f28 Compare July 14, 2022 22:41
@samsonasik
Copy link
Member Author

Fixed 🎉 with loop found ClassLike instances to get the same ClassLike for multiple ClassLike in single file. 0819d0b

@samsonasik samsonasik marked this pull request as ready for review July 14, 2022 22:52
@samsonasik samsonasik changed the title [Php80] Add implements interface support on AddParamBasedOnParentClassMethodRector [Php80] Add implements interface support on single file on AddParamBasedOnParentClassMethodRector Jul 14, 2022
@samsonasik
Copy link
Member Author

it seems cause error in ReturnTypeDeclarationRector:

There were 2 failures:

1) Rector\Tests\TypeDeclaration\Rector\FunctionLike\ReturnTypeDeclarationRector\ReturnTypeDeclarationRectorTest::test with data set #25 (Symplify\SmartFileSystem\SmartFileInfo Object (...))
rules-tests/TypeDeclaration/Rector/FunctionLike/ReturnTypeDeclarationRector/Fixture/skip_respect_children_return_type_mixed.php.inc
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 
 class SkipRespectChildrenReturnTypeParentMixed
 {
-    public function run()
+    public function run(): void

I will check more.

@samsonasik
Copy link
Member Author

Fixed 🎉

@samsonasik
Copy link
Member Author

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

@samsonasik
Copy link
Member Author

Finally 🎉 , implements from interface with different file is also working 🎉 🎉 🎉, added fixture for it f824fab

@samsonasik
Copy link
Member Author

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

@MartinMystikJonas
Copy link
Contributor

Nice. Now when we have all method definitions from parent and interfaces I think I can merge it into union type that would satisfy all. After this is merged I can try to improve it with this merging. I will also use similar mechanism for detection of intersect return type in my new rule that adds return types based on parents. I already have basic draft but it does not work yet

@TomasVotruba TomasVotruba merged commit db7012e into main Jul 15, 2022
@TomasVotruba TomasVotruba deleted the add-implement-interface-support branch July 15, 2022 08:15
@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
3 participants