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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

minor: ExplicitStringVariableFixer - Fix to PHP8.2 compat code #6424

Conversation

SpacePossum
Copy link
Contributor

PHP8.2

see: #3488 (comment)

ping @dmvdbrugge as author of the rule, ping @iluuu1994 as author of the PHP language change, please have a look and let me know what you guys think, thanks! 馃憤

Stricktly this change could be considered a BC break, but fixing towards a PHP8.2 deprecated code style is more problematic and outweighs the minor BC thing IMHO.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.873% when pulling 0ee2b9d on SpacePossum:master_php82_ExplicitStringVariableFixer into 7936fb2 on FriendsOfPHP:master.

@dmvdbrugge
Copy link
Contributor

If I look at the linked comment, @Slamdunk is the author of ExplicitStringVariableFixer, I'm merely the author of SimpleToComplexVariableFixer (which should now be included in any "migrate to php 8.2+" sets).

It is correct the priority is no longer needed as the former will no longer produce the result the latter is fixing.

Anyway, the change looks good. Is this targeting a minor or a patch release? I wouldn't do patch because of the BC thing, but if you specify this as "8.2 compatibility feature" then minor sounds good to me 馃槃

@SpacePossum
Copy link
Contributor Author

SpacePossum commented Jun 13, 2022

Thanks for your thoughts @dmvdbrugge , much appreciated :) we'll have to see when and how this gets shipped, but good points 馃憤

@iluuu1994
Copy link
Contributor

LGTM. This doesn't handle the deprecated var var syntax "${expr}" described in the RFC but to be fair I couldn't find a single case of those in the wild with my script.

@SpacePossum
Copy link
Contributor Author

Thanks @iluuu1994 :)
We have an additional rule to rewrite those cases here:
simple_to_complex_string_variable
(additional tests cases).
Thanks for making the change in PHP and for checking things out here, much appreciated 馃憤

@iluuu1994
Copy link
Contributor

Note that "${var}" is equivalent to "{$var}" (which is handled by simple_to_complex_string_variable), but "${expr}" is equivalent to "{${expr}}". The distinction is very confusing (hence the deprecation). But this is probably negligible as I doubt anybody actually used that syntax non-accidentally.

@keradus keradus merged commit aacc804 into PHP-CS-Fixer:master Jul 10, 2022
@SpacePossum SpacePossum deleted the master_php82_ExplicitStringVariableFixer branch July 11, 2022 05:05
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.

None yet

5 participants