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

fix: Support shebang in fixers operating on PHP opening tag #7687

Merged
merged 15 commits into from Jan 18, 2024

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Jan 6, 2024

fix #7634 (and #7681 duplicate)

@mvorisek mvorisek marked this pull request as ready for review January 6, 2024 23:48
@mvorisek mvorisek force-pushed the fix_declare_7634 branch 4 times, most recently from 36f0af3 to d453856 Compare January 8, 2024 07:39
@mvorisek mvorisek requested a review from keradus January 8, 2024 08:02
Copy link
Member

@Wirone Wirone left a comment

Choose a reason for hiding this comment

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

I did not review changes in the actual fixer, I'll continue after concerns related to tests are resolved.

PS. I prefer explicit with / without instead of /w / /wo in test cases' names, but it's not something I will fight for 😉.

tests/Fixer/PhpTag/NoClosingTagFixerTest.php Outdated Show resolved Hide resolved
tests/Tokenizer/TokensTest.php Outdated Show resolved Hide resolved
@mvorisek
Copy link
Contributor Author

mvorisek commented Jan 8, 2024

I relanded the original behaviour. I do not however see any benefit by considering T_OPEN_TAG_WITH_ECHO as monotonic (one script file) as most of the fixers does not support it. Maybe they miss the support. IDK.

see last commit - it makes things ugly and inconsistent (to match original design), but you decide, I would revert the last commit

@mvorisek mvorisek requested a review from Wirone January 10, 2024 01:10
@mvorisek mvorisek marked this pull request as draft January 10, 2024 01:20
Copy link
Member

@Wirone Wirone left a comment

Choose a reason for hiding this comment

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

Not complete review, but feedback for "ugly" changes 😉.

src/Fixer/Comment/HeaderCommentFixer.php Outdated Show resolved Hide resolved
src/Fixer/PhpTag/BlankLineAfterOpeningTagFixer.php Outdated Show resolved Hide resolved
src/Fixer/PhpTag/LinebreakAfterOpeningTagFixer.php Outdated Show resolved Hide resolved
src/Fixer/Strict/DeclareStrictTypesFixer.php Outdated Show resolved Hide resolved
@mvorisek mvorisek marked this pull request as ready for review January 10, 2024 02:04
@mvorisek mvorisek requested a review from Wirone January 10, 2024 02:16
@mvorisek
Copy link
Contributor Author

@Wirone is there any feedback left?

@Wirone
Copy link
Member

Wirone commented Jan 11, 2024

@mvorisek I don't know, I did not have time to look at it yet 🤷‍♂️.

@mvorisek
Copy link
Contributor Author

Kindly ping @Wirone, the current master/release is broken.

Copy link
Member

@julienfalque julienfalque left a comment

Choose a reason for hiding this comment

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

Looks pretty good overall 👍

Maybe the title of the PR should be updated as the scope of changes is not limited to declare_strict_types

src/Tokenizer/Tokens.php Outdated Show resolved Hide resolved
src/Fixer/PhpTag/BlankLineAfterOpeningTagFixer.php Outdated Show resolved Hide resolved
@mvorisek mvorisek changed the title fix: declare_strict_types must work with shebang fix: fixers must work with shebang Jan 18, 2024
@Wirone Wirone changed the title fix: fixers must work with shebang fix: Support shebang in fixers operating on PHP opening tag Jan 18, 2024
@Wirone Wirone enabled auto-merge (squash) January 18, 2024 18:47
@Wirone Wirone merged commit cff91c0 into PHP-CS-Fixer:master Jan 18, 2024
25 checks passed
@Wirone
Copy link
Member

Wirone commented Jan 18, 2024

Thanks @mvorisek 🍻

@mvorisek mvorisek deleted the fix_declare_7634 branch January 18, 2024 23:37
@krzysztof-sikorski
Copy link

Thank you @mvorisek 🎉

danog pushed a commit to zoonru/PHP-CS-Fixer that referenced this pull request Feb 2, 2024
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.

Rule declare_strict_types does not work on files that start with a "hashbang" line
5 participants