-
-
Notifications
You must be signed in to change notification settings - Fork 374
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
Remove HTML + PHP support, as must be handled in php-parser first #4051
Conversation
a4404d1
to
1737d3d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e2e/timeout-file-not-cached
is still needed for test big file with limited time defined.
a691abd
to
6a21174
Compare
@TomasVotruba I just realise rector build downgrade need php+html
So it possibly break downgrade |
See this PR for downgrade tracy files this is happen for immediate html+php patch, which on tracy part, probably fine, but will certainly break if append/prepend/remove stmt with InlineHTML as next or target refactor. |
@leoloso FYI if you have mix php+html in your wordpress extension to downgrade |
By add warning on found InlineHTML node, current downgrade will no longer work. The php+html hack was used on append/prepend/remove changed Stmt/InlineHTML (with file has changed check, that on purpose), not changed inside Stmt itself |
@samsonasik Thanks for the heads up. Luckily I don't have any mix of PHP + HTML to downgrade |
@leoloso thank you for the check. @TomasVotruba for handling summary, the issue was not about mix html+php, as inline change should works as expected, but when on target node near InlineHTML or after a Nop Stmt prepended/appended/removed, that InlineHTML need to be refreshed, or Nop need to be marked as comment. That's why the handling decorator uses start token check as new stmt doesn't have start token attribute yet. |
@leoloso Hey, thanks for checking. Btw, how far down do you downgrade? |
@TomasVotruba I'm downgrading to PHP 7.1, and already very happy with this level. Some important WordPress plugins now require PHP 7.2, so I think nobody uses 7.0 anymore, and I certainly wouldn't spend much energy in adding the unsupported 7.0 features to the downgrade. Btw, I believe that PHP 7.1 is fully covered with the downgrade, with all its features supported, so I'm not sure what you call "thin". (Actually, I think that pretty much everything is supported between 7.1 and 8.0... only enums in 8.1 are the major feature not yet supported by the downgrade) |
5a2cfeb
to
7867176
Compare
@samsonasik I'm working on this and rebase back and forth so I've lost your commit about timeout test. Could you add it via separate PR to dev-main so we have it? :) |
49f8d78
to
71c012a
Compare
8e1127c
to
47c2cac
Compare
@TomasVotruba yes #4059 |
This is one of design I want to solve before Rector 1.0 rectorphp/rector#7854
The HTML and PHP support is now well grounded in php-parser - https://github.com/nikic/PHP-Parser/
We have bunch of workarounds for files, where Rector should not run at all. It only patches lack of types and does not provide much value. PHPStan does not know about the types and usage in HTML + PHP is limited, if any.
That's why we remove such hacks and rather skip any code with HTML + PHP nodes with warning. This will keep HTML files skipped without any touches and won't break them untill PHP is separated from HTML.
Todo