Skip to content

[EarlyReturn] Skip else has inline html on RemoveAlwaysElseRector#2870

Merged
TomasVotruba merged 5 commits intomainfrom
close-2867
Aug 31, 2022
Merged

[EarlyReturn] Skip else has inline html on RemoveAlwaysElseRector#2870
TomasVotruba merged 5 commits intomainfrom
close-2867

Conversation

@samsonasik
Copy link
Copy Markdown
Member

@samsonasik samsonasik closed this Aug 31, 2022
@samsonasik samsonasik reopened this Aug 31, 2022
@samsonasik
Copy link
Copy Markdown
Member Author

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

@TomasVotruba TomasVotruba merged commit 3addb8f into main Aug 31, 2022
@TomasVotruba TomasVotruba deleted the close-2867 branch August 31, 2022 10:08
@TomasVotruba
Copy link
Copy Markdown
Member

🥳

@kkmuffme
Copy link
Copy Markdown
Contributor

kkmuffme commented Sep 5, 2022

Afaik this is wrong, bc now it does nothing, even though this could easily be optimized.
From:
https://getrector.org/demo/8ddb0bc5-8835-4c7a-bd2c-fe60b908282e
to:
https://getrector.org/demo/9b1b3305-acf9-4a08-a295-c4880cfd3f9a

@samsonasik
Copy link
Copy Markdown
Member Author

Feel free to provide patch if you have different solution :)

@kkmuffme
Copy link
Copy Markdown
Contributor

kkmuffme commented Sep 5, 2022

Just brainstorming: if there are any PHP closing/opening tags we just need to keep them instead of removing them?

Is there a way on the "demo" to load an older version of rector, to play around with the rule so I can check various cases?

@samsonasik
Copy link
Copy Markdown
Member Author

Rector is using nikic/php-parser, which replacing html+php may overap the index.

Still, feel free to provide patch if you found a solution, otherwise, you can use $rectorConfig->skip() option to skip the rule.

Feel free to contribute by create PR to https://github.com/rectorphp/getrector.org if you have want to provide PR to improve demo page.

@kkmuffme
Copy link
Copy Markdown
Contributor

kkmuffme commented Sep 5, 2022

How about we just report it instead of auto-fixing it?

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.

Incorrect behavior of RemoveAlwaysElseRector

3 participants