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

Remove HTML + PHP support, as must be handled in php-parser first #4051

Merged
merged 2 commits into from
Jun 3, 2023

Conversation

TomasVotruba
Copy link
Member

@TomasVotruba TomasVotruba commented Jun 2, 2023

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

  • add warning on HTML node found

@TomasVotruba TomasVotruba force-pushed the tv-merge-tests-2 branch 2 times, most recently from a4404d1 to 1737d3d Compare June 2, 2023 13:27
@samsonasik
Copy link
Member

/cc @staabm @kkmuffme

Copy link
Member

@samsonasik samsonasik left a 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.

@TomasVotruba TomasVotruba force-pushed the tv-merge-tests-2 branch 2 times, most recently from a691abd to 6a21174 Compare June 2, 2023 14:21
@samsonasik
Copy link
Member

@TomasVotruba I just realise rector build downgrade need php+html

$rectorConfig->fileExtensions(['php', 'phtml']);

So it possibly break downgrade

@samsonasik
Copy link
Member

samsonasik commented Jun 2, 2023

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.

@samsonasik
Copy link
Member

@leoloso FYI if you have mix php+html in your wordpress extension to downgrade

@samsonasik
Copy link
Member

samsonasik commented Jun 2, 2023

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

@leoloso
Copy link
Contributor

leoloso commented Jun 3, 2023

@samsonasik Thanks for the heads up. Luckily I don't have any mix of PHP + HTML to downgrade

@samsonasik
Copy link
Member

samsonasik commented Jun 3, 2023

@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.

@TomasVotruba
Copy link
Member Author

@leoloso Hey, thanks for checking. Btw, how far down do you downgrade?
I'm reviewing usefulness of target PHP versions, as some sets like PHP 7.0 or 7.1 are quite thin.

@TomasVotruba TomasVotruba changed the title Remove HTML + PHP support, as must be handled in php-parser first [WIP]Remove HTML + PHP support, as must be handled in php-parser first Jun 3, 2023
@TomasVotruba TomasVotruba changed the title [WIP]Remove HTML + PHP support, as must be handled in php-parser first [WIP] Remove HTML + PHP support, as must be handled in php-parser first Jun 3, 2023
@leoloso
Copy link
Contributor

leoloso commented Jun 3, 2023

@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)

@TomasVotruba TomasVotruba changed the title [WIP] Remove HTML + PHP support, as must be handled in php-parser first Remove HTML + PHP support, as must be handled in php-parser first Jun 3, 2023
@TomasVotruba
Copy link
Member Author

TomasVotruba commented Jun 3, 2023

@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? :)

@TomasVotruba TomasVotruba force-pushed the tv-merge-tests-2 branch 2 times, most recently from 8e1127c to 47c2cac Compare June 3, 2023 16:07
@TomasVotruba TomasVotruba merged commit ef3d97d into main Jun 3, 2023
@TomasVotruba TomasVotruba deleted the tv-merge-tests-2 branch June 3, 2023 16:12
@samsonasik
Copy link
Member

@TomasVotruba yes #4059

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.

3 participants