RemoveAlwaysElseRector bug

[escopecz]

Subject	Details
Rector version	v0.6.13)
PHP version	PHP 7.2
Full Command	e.g. vendor/bin/rector process /src --set dead-code --debug
Demo link	https://getrector.org/demo/dacac137-cae9-4d24-a153-06e5016615c1#result
rector.yaml	in the demo

Current Behaviour

Rector moves the elseif part out of the foreach loop and breaks the code. It should stay in the loop.

Minimal PHP Code Causing Issue

Applied rules:

• Rector\SOLID\Rector\If_\RemoveAlwaysElseRector

<?php declare(strict_types=1);

final class DemoFile
{
    public function run(array $list, bool $removeEmpty)
    {
        foreach ($list as $val => $label) {
			if ($removeEmpty && empty($val) && empty($label)) {
				continue;
			} elseif (empty($label)) {
				$label = $val;
			}
		}
    }
}

Expected Behaviour

The elseif part should stay in the foreach loop scope.

[TomasVotruba]

I don't see the problem. What is specific for this bug?

[escopecz]

You are right. It's not moved out of the foreach scope here. Tried to create a minimal example. But I cannot replicate it even if I use the full method:

https://getrector.org/demo/fd4e1e8f-9571-457c-99a7-14bf9b414c4a

But you can see it here: https://github.com/mautic/mautic/pull/8414/files#diff-9987c38b8aa80d46767eb05c8f591b94

I can replicate it locally with on Mautic when I add the early-return set to the config and running bin/rector process --config rector.yaml app/bundles/CoreBundle/Helper/AbstractFormFieldHelper.php. How come the output is different (correct) in the demo?

$ bin/rector process  --config rector.yaml app/bundles/CoreBundle/Helper/AbstractFormFieldHelper.php
Rector v0.6.13
Config file: rector.yaml

 3/3 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

1 file with changes
===================

1) app/bundles/CoreBundle/Helper/AbstractFormFieldHelper.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -129,6 +129,9 @@
             // Numberical array so set labels as values
             $list = array_combine($list, $list);
         }
+        if ($removeEmpty && empty($val) && empty($label)) {
+            continue;
+        }

         $valueFormatting = function ($list) use ($removeEmpty, $flipArray) {
             $choices = [];
@@ -137,9 +140,7 @@
                     $val   = $label['value'];
                     $label = $label['label'];
                 }
-                if ($removeEmpty && empty($val) && empty($label)) {
-                    continue;
-                } elseif (empty($label)) {
+                if (empty($label)) {
                     $label = $val;
                 }
                 if (!is_array($label)) {
    ----------- end diff -----------
Applied rules:
 * Rector\SOLID\Rector\If_\RemoveAlwaysElseRector                                                               
 [OK] Rector is done! 1 file have been changed.  

[escopecz]

Well, looking at the demo for a little longer, it's missing the if part completely:

$val   = $label['value'];
                     $label = $label['label'];
                 }
-                if ($removeEmpty && empty($val) && empty($label)) {
-                    continue;
-                } elseif (empty($label)) {
-                    $label = $val;
+                if (empty($label)) {
+                    $label = $val;
                 }
                 if (!is_array($label)) {
                     $value = trim(html_entity_decode($val, ENT_QUOTES));

So it's not showing where it moved the if part.

[TomasVotruba]

Demo is needed though, it's impossible to deduce anything from diff of few lines.

[escopecz]

The demo is here: https://getrector.org/demo/fd4e1e8f-9571-457c-99a7-14bf9b414c4a

Compare how the output looks like when I run it locally (#2842 (comment)) and how the output looks like in the demo (#2842 (comment)).

The demo output does not show the whole thing it does. Or it deletes the IF part completely which is also a bug, right? Maybe I don't understand what you mean.

[TomasVotruba]

I see. The demo might need an update too.

@JanMikes Hi, could you bump Rector on demo to 0.7? And check if it works?

[TomasVotruba]

@escopecz Could you send failing test case to
RemoveAlwaysElseRectorTest?

[escopecz]

@TomasVotruba here it is: #2862

[JanMikes]

Yeah i can confirm that running on same code with same docker container produces different output on online demo and running locally in docker.