Incorrect behavior of RemoveDeadStmtRector (and others?) with short echo tags

[bananastalktome]

Bug Report

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

Minimal PHP Code Causing Issue

See https://getrector.com/demo/a7122f39-c255-4c56-908c-6f412f696ec4

<?php
$args;
?>
<img src="<?= esc_url($image_src); ?>" alt="">

Additionally, from https://getrector.com/demo/f3d8ac7d-212b-4cff-86bd-5939a2ee338f ,

<?php
function test_callback() {
  // var_dump($output);
  ?>
  <div class="wrap">
    <img src="<?= escape('hi there'); ?>">
  </div>
  <?php
}

Responsible rules

• RemoveDeadStmtRector
• No rules listed for second demo link

Expected Behavior

For the first demo link, removing $args; and for the second either removing or ignoring the commented var_dump, but for both not also adding php open/close tags around otherwise functional short echo tag.

[samsonasik]

That's possibly php-parser bug itself, as just by printing itself got additional <?php, see:

https://getrector.com/demo/558b238e-b4d6-497c-918b-5b821c6b4e49

This also possibly related with issue:

• Incorrect PHP code is generated with commented-out <?php tags #7703

which commented custom syntax on if elseif got duplicated comment.

I sent failing test case PR to nikic/php-parser for duplicated comment:

• Failing test case duplicated commented elseif on alternative syntax nikic/PHP-Parser#914

for this use case, you may create another failing test case PR as well to https://github.com/nikic/PHP-Parser :)

[bananastalktome]

@samsonasik thank you for the reply! I'm not familiar with the details of PHP-Parser, but I cloned the repo and added a test locally:

<?php

function test()
{
    // var_dump('test');
    ?>
    <img src="<?= escape('Test'); ?>">
    <?php
}
-----
function test()
{
    // var_dump('test');
    ?>
    <img src="<?= escape('Test'); ?>">
    <?php
}

Though the test failed, it only changed the following:

-    <img src="<?= escape('Test'); ?>">
+    <img src="<?php
+    echo escape('Test');
+    ?>">

With rector, the same function (https://getrector.com/demo/9574e31f-3017-4d28-903b-490bcf416628) has a different replacement. PHP-Parser didn't appear to try and add <?php around the existing <?=, but replaced the short echo with the longer form, whereas rector did seem to want to mess with the php tags.

Edit:
Oddly, I just noticed https://getrector.com/demo/041045d4-e04b-400f-af7d-31e0e5f97f94 does not result in any changes:

<?php

function hi() {
  // var_dump('hi');
  ?>
  <img src="<?= escape('Test'); ?>">
<?php 
}

though https://getrector.com/demo/d04994f8-2e5b-44bc-8720-868074fd4caf does:

<?php

function hi() {
  // var_dump('hi');
  ?>
  <img src="<?= escape('Test'); ?>">
  <?php 
}

[samsonasik]

Demo page and local may have different addition ?> , thats possibly due to php read php code on mix html+php on website.

You can try latest "rector/rector": "dev-main" locally

[bananastalktome]

Demo page and local may have different addition ?> , thats possibly due to php read php code on mix html+php on website.

You can try latest "rector/rector": "dev-main" locally

@samsonasik When run locally, though I no longer get the extra closing tag from the second example above, I still get the weird tag doubling:

-  <img src="<?= escape('Test'); ?>">
+  <img src="<?php <?= escape('Test'); ?>">

Are you thinking this is from PHP-Parser, though running tests through it as included above doesn't seem to result in doubling-up of php opening tags (rector turning <?= into <?php <?=)?

Thanks!

[samsonasik]

Yes, if no rule applied and open tag added, it possibly php-parser bug or cache somewhere, you may also try enable short open tag as @realtebo tried at #7784 (comment)

and ensure run with --clear-cache

There is ongoing todo on nikic/php-parser due to printing inlineHTML, see:

• Format-preserving AST transformations TODO nikic/PHP-Parser#344

[bananastalktome]

@samsonasik Thanks for the continued advice! I ran it again locally, with short_open_tag enabled, but there was no difference (<?= isn't affected by short_open_tag, I don't think).

I am running with the rules

$rectorConfig->rule(InlineConstructorDefaultToPropertyRector::class);
$rectorConfig->sets([
    LevelSetList::UP_TO_PHP_74
]);

nikic/php-parser does seem to handle parsing the code fine, and only changes <?= to <?php echo rather than rector changing it to <?php <?=. Where would this doubling up come from? Please let me know if there is any other debugging or testing I can do to help figure this out.

[samsonasik]

If unit test result is different with run on command, you may need to add e2e/plain-views test https://github.com/rectorphp/rector-src/tree/main/e2e with the following steps:

1. open e2e/plain-views
2. run composer install
3. Add fixture files under https://github.com/rectorphp/rector-src/tree/main/e2e/plain-views/src
4. run php ../e2eTestRunner.php

See the diff, and submit the pull request with expected output at https://github.com/rectorphp/rector-src/blob/main/e2e/plain-views/expected-output.diff

then, create PR as failing test case PR, would be awesome if you can include the fix :)

[samsonasik]

I am looking into it at PR:

• [e2e] [printer] Fix short open tag mixed php+html cause surplus <?php  rector-src#3395