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

Incorrect behavior of WrapVariableVariableNameInCurlyBracesRector #8861

Open
corentin-larose opened this issue Oct 14, 2024 · 4 comments
Open
Labels

Comments

@corentin-larose
Copy link

Bug Report

Subject Details
Rector version last dev-main
Installed as composer dependency

Minimal PHP Code Causing Issue

See https://getrector.com/demo/7ebfdb2e-88ec-4c6e-9dbe-2faa74a30977

<?php
$$foo['bar']['baz'];

Responsible rules

  • WrapVariableVariableNameInCurlyBracesRector

Expected Behavior

According to https://www.php.net/manual/en/migration70.incompatible.php:

Expression PHP 5 interpretation
$$foo['bar']['baz'] ${$foo['bar']['baz']}
@samsonasik
Copy link
Member

The result seems valid, see https://3v4l.org/D3TDJ , the rule is on php 7.0 migration, not php 5.x and it show valid and equal result, could you show me which one that it cause invalid result at https://3v4l.org ? Thank you.

@corentin-larose
Copy link
Author

corentin-larose commented Oct 16, 2024

Your test hides the versions which actually interest us: PHP5.6 and PHP7.0.

Take a close look at those tests, you will see that the code won't do the same think in PHP5.6 and PHP7.0 despite the Rector modification, proof is you have an error in PHP5.6, I will take some time tonight in order to create better examples to explain my point:

https://3v4l.org/oCUdQ#v5.6.40
https://3v4l.org/gh24o#v7.0.33

@samsonasik
Copy link
Member

The rule is for php7.0 upgrade, not php 5.6, so the invalid php 5.6 code is fixed on php 7.0 rule.

We don't change to 2 kind of versions, php 5.6 vs php 7.0, instead: use syntax that works for both :), see https://3v4l.org/gh24o#v5.6.40

@corentin-larose
Copy link
Author

Thanks @samsonasik, let's put it another way, I understand I have not been clear in my previous message.

Can you please check each proposition and check it if you agree:

  • Purpose of the SetList::PHP70 rule is to modify PHP 5.6 code in order to make it work with the PHP 7.0 interpreter, obviously NOT changing the result of the code.
  • Rector\Php70\Rector\Variable\WrapVariableVariableNameInCurlyBracesRector belongs to SetList::PHP70.
  • In PHP 7.0, you can no longer write $$foo->bar, you MUST write ${$foo->bar} instead.
  • WrapVariableVariableNameInCurlyBracesRector rule has been written in order to perform this very part of the PHP 5.6 to PHP 7.0 migration process.
  • When you apply WrapVariableVariableNameInCurlyBracesRector rule on the PHP 5.0 code $$foo['bar'] it modifies it as ${$foo}['bar'].
  • Now please, run the following code:
// Run this on PHP 5.6 (https://3v4l.org/XB19f#v5.6.40)
<?php
$baz = 'bat';
$foo = array('bar' => 'baz');

echo $$foo['bar'];
echo PHP_EOL;
echo ${$foo['bar']};
echo PHP_EOL;

// PHP 5.6 output:
bat
bat
// Run this on PHP 7.0 (https://3v4l.org/jBLbv#v7.0.0)
<?php
$baz = 'bat';
$foo = array('bar' => 'baz');

echo ${$foo}['bar']; // This is the way `WrapVariableVariableNameInCurlyBracesRector` currently fixes the code.
echo PHP_EOL;
echo ${$foo['bar']}; // This is the way `WrapVariableVariableNameInCurlyBracesRector` SHOULD fix the code. 
echo PHP_EOL;

// PHP7.0 output
Notice: Array to string conversion in /in/jBLbv on line 5

Notice: Undefined variable: Array in /in/jBLbv on line 5

bat
  • After applying rule Rector\Php70\Rector\Variable\WrapVariableVariableNameInCurlyBracesRector the behavior of the code changes, WHICH IS A BUG.
  • As I wrote before, this is absolutely NO SUPRISE, since this is explained in the "Changes to the handling of indirect variables, properties, and methods" of the PHP 5.6 to PHP 7.0 migration documentation.

I DO make mistakes, but I take a lot of time before filing bugs, so please take your time to read me carefully :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants