-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat: introduce string_implicit_backslashes
as escape_implicit_backslashes
replacement
#7669
Conversation
d631add
to
31692f9
Compare
escape_implicit_backslashes
must also unescape backslashedescape_implicit_backslashes
must also unescape backslashes
f026d84
to
1fc2173
Compare
51dbe67
to
16b0e15
Compare
PR is done. Tested with
configuration on the whole project, ie. the inverse configuration. Once removed and fixer is run again, all code returned to exactly the same version as before. |
a85f176
to
f25d7a2
Compare
Hi @Wirone, can you please review this PR? |
I hope to do some reviews in the late evening 🤞. |
@Wirone is now a good late evening? |
Shouldn't this new behavior be behind an option? |
Hah, with 3 kids you can plan and end with totally different reality 😅. Today I am going on business trip for 2 days, many hours in the train, so I again plan to do some reviews 😉. |
What about It is easy to add, but I am a little worried to whom it will be helpful in reality. |
To be honest, I don't know what to do with this PR 🤷♂️. In general, fixers are named after the state desired after fix is applied, in this case: all implicit backslashes should be escaped. It's clear that we can't change the current behaviour just like that, it must be introduced behind configuration option. But even if we do this, the idea behind fixer under this particular name would be broken, so in order to do it properly we should introduce new fixer, e.g. But it's only my perspective, I believe we need @keradus' opinion. |
I like the 3-state concept and new fixer name - is there any "proxy migration fixer PR" I can as a reference? Other options is to keep this PR as is, I do not see any value in not unescaping implicit backslashes when they are not fixed to the oposite. |
I also like @Wirone's proposal and agree the current name of the fixer might not be very accurate with this new behavior. I do think unescaping should be the default behavior but that might be an unexpected change for users, hence the option. We might reconsider this when we'll prepare a new major version. |
98894d1
to
014837a
Compare
escape_implicit_backslashes
must also unescape backslashesstring_implicit_backslashes
as escape_implicit_backslashes
replacement
9253d17
to
3fa7707
Compare
string_implicit_backslashes
as escape_implicit_backslashes
replacementstring_implicit_backslashes
as escape_implicit_backslashes
replacement
9181cf5
to
5361800
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.
I would like to see small improvement in tests, other than that - looks good.
tests/Fixer/StringNotation/StringImplicitBackslashesFixerTest.php
Outdated
Show resolved
Hide resolved
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.
Co-authored-by: Greg Korba <wirone@gmail.com>
976c4ac
to
302ef71
Compare
Thank you @mvorisek 🍻! |
…kslashes` replacement (PHP-CS-Fixer#7669) Co-authored-by: Greg Korba <wirone@gmail.com>
fix #7621
if backslashes are disabled to be explicitly escaped, they must be unescaped