Skip to content

[FileProcessor] Add warning instead of skip InlineHTML on PhpFileProcessor#4058

Merged
TomasVotruba merged 5 commits intomainfrom
add-warning
Jun 4, 2023
Merged

[FileProcessor] Add warning instead of skip InlineHTML on PhpFileProcessor#4058
TomasVotruba merged 5 commits intomainfrom
add-warning

Conversation

@samsonasik
Copy link
Copy Markdown
Member

Ref #4055 (review)

Instead of skip, better to show warning so user can just use it but need to verify manually the rectored file.

@samsonasik samsonasik requested a review from TomasVotruba as a code owner June 3, 2023 23:40
@samsonasik
Copy link
Copy Markdown
Member Author

I removed with inline html fixture as SymfonyStyle warning seems not shown in unit test.

@samsonasik
Copy link
Copy Markdown
Member Author

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

samsonasik referenced this pull request in rectorphp/rector Jun 3, 2023
@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented Jun 4, 2023

@TomasVotruba here the output with warning instead of skip which fine on indirect change near InlineHTML:

➜  rector-src git:(add-warning) ✗ bin/rector process vendor/tracy/tracy/src/Tracy/BlueScreen/assets/section-stack-callStack.phtml --config build/config/config-downgrade.php --dry-run
 0/1 [░░░░░░░░░░░░░░░░░░░░░░░░░░░░]   0%
 [WARNING] File vendor/tracy/tracy/src/Tracy/BlueScreen/assets/section-stack-callStack.phtml has InlineHTML node, this  
           may cause unexpected output, you may need to manually verify the changed file                                

 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
1 file with changes
===================

1) vendor/tracy/tracy/src/Tracy/BlueScreen/assets/section-stack-callStack.phtml:71

    ---------- begin diff ----------
@@ @@
                        try {
                                $r = isset($row['class']) ? new \ReflectionMethod($row['class'], $row['function']) : new \ReflectionFunction($row['function']);
                                $params = $r->getParameters();
-                       } catch (\Exception) {
+                       } catch (\Exception $exception) {
                                $params = [];
                        }
                        foreach ($row['args'] as $k => $v) {
    ----------- end diff -----------

Applied rules:
 * DowngradeNonCapturingCatchesRector (https://wiki.php.net/rfc/non-capturing_catches)

so this should good to have it processed 👍

Copy link
Copy Markdown

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea to leave mixed HTML+PHP alone, but some more verification (test) would be a good idea 😁

@kenjis
Copy link
Copy Markdown
Contributor

kenjis commented Jun 4, 2023

It seems better to support mixed HTML+PHP even if there are limitations.

@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented Jun 4, 2023

@Ocramius the tests were on MixedPhpHtmlDecorator, since that decorator removed at PR:

so the tests not needed in here

@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented Jun 4, 2023

I enabled php-linter for tracy PHP+HTML file to be checked after downgrade for verifying on scoped build that the file still downgraded correctly 👍 e88a3c6 + d24dc9f

@TomasVotruba
Copy link
Copy Markdown
Member

Looks good, lets give it a try 👍

If the feedback is rather breaking code again, we'll disable it to avoid causing errors.

@TomasVotruba TomasVotruba merged commit 165609b into main Jun 4, 2023
@TomasVotruba TomasVotruba deleted the add-warning branch June 4, 2023 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants