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

Documentation: Clearly and visibly document that rector does not work reliably when PHP closing tags are used anywhere in the file #8479

Closed
kkmuffme opened this issue Feb 9, 2024 · 3 comments
Labels

Comments

@kkmuffme
Copy link

kkmuffme commented Feb 9, 2024

Bug Report

Subject Details
Rector version v1.0.0

Please clearly document that currently rector does not reliably support files where PHP closing tags are used, except at the end of the file.
Lots of rules create fatal errors in files when you have a PHP closing tag somewhere in a file - which is incredibly common in legacy projects, which psalm is made for.
When reporting these errors it's always only met with #8465 (comment) but there's no mention of this anywhere in the docs - this should be something that's at the forefront, unless you want rector's users to spend hours checking/wasting their time why rector creates fatal errors in various files.

@kkmuffme kkmuffme added the bug label Feb 9, 2024
@TomasVotruba
Copy link
Member

This is documented here: https://github.com/rectorphp/rector#known-drawbacks

The php-parser printer limitations are applied on any tool using it, not just Rector. The linked comment is right, as that's inherited trait. I recommend to promote the issue there, as fixing it would help all tools running on legacy projects.

I personally do not recommend to run any php-parser printer tool on mix of PHP/HTML.

@kkmuffme
Copy link
Author

kkmuffme commented Feb 10, 2024

Yes, but the documentation is wrong and a understatement at best.
The problem isn't a mix of PHP/HTML - it can happen with anything after PHP closing tags and has nothing to do with HTML.

Also clearly stating that the "fixed" code could be invalid PHP and may trigger fatal errors would make it clear that this is an issue, bc right now it's clearly very understated

you may need to manually verify the changed file

especially given that you claim "Automated Refactoring" in the title? Not really automated when I have to manually verify each and every file, since rector may add fatal errors, is it?

EDIT: in no way I want to attack you or rector, I just want to help other users have clear expectations when starting to use rector and being aware that this can be a problem that can happen and to not abandon rector in case it does happen - since the fix is to just disable those rector rules for files where non-PHP is used.

@kkmuffme
Copy link
Author

I just took a look at #8465 again which was closed bc of lack of php-parser support.

However the AST shows \PhpParser\Node\Stmt\InlineHTML correctly, doesn't it?
Since you link to php-parser not supporting php+html, could you perhaps link to a github issue where this issue is reported? I only found 1 which was marked as completed nikic/PHP-Parser#344 (comment) and it's seems for most cases that php-parser handles php+non-php fine now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants