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

[e2e][Printer] Handle crash indentation on AddParamBasedOnParentClassMethodRector #6112

Merged
merged 6 commits into from
Jul 12, 2024

Conversation

samsonasik
Copy link
Member

@samsonasik samsonasik commented Jul 3, 2024

Fixes rectorphp/rector#8712

Step to reproduce only via e2e test:

cd e2e/print-new-node 
composer install
php ../e2eTestRunner.php 

without this patch, it will cause error:

➜  print-new-node git:(indentation-crash-error) php ../e2eTestRunner.php
 2/2 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%    ---------- begin diff ----------
@@ @@
-[ERROR] Could not process "./print-new-node/src/ExtendingTestClass.php" file, due
-         to:
-         "System error: "Rector\PhpParser\Printer\BetterStandardPrinter::p(): Return value must be of type string, null
-         returned"
-         Run Rector with "--debug" option and post the report here: https://github.com/rectorphp/rector/issues/new". On line: 154

For note: I already tried set origNode = null on the target rule, but no luck, it only resolvable via printer.

@samsonasik
Copy link
Member Author

Fixed 🎉 /cc @RuesimOfCode

@samsonasik
Copy link
Member Author

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

@TomasVotruba
Copy link
Member

This looks like bug in php-parser itself. If that's true, we should report it first there with a reproducer.

@TomasVotruba TomasVotruba force-pushed the indentation-crash-error branch 2 times, most recently from 0c0980b to c71c27f Compare July 12, 2024 08:43
@TomasVotruba
Copy link
Member

TomasVotruba commented Jul 12, 2024

Ready to go 👍

@TomasVotruba TomasVotruba requested a review from a team July 12, 2024 09:19
@samsonasik
Copy link
Member Author

I can't approve my own PR, i think this is ready, just probably concern about renaming method bc break :)

@TomasVotruba TomasVotruba enabled auto-merge (squash) July 12, 2024 09:28
@TomasVotruba TomasVotruba merged commit eae3e2c into main Jul 12, 2024
34 checks passed
@TomasVotruba TomasVotruba deleted the indentation-crash-error branch July 12, 2024 09:29
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.

AddParamBasedOnParentClassMethodRector causes "System error"
2 participants