Rector: [symfony42] Rector\Symfony\Rector\New_\StringToArrayArgumentProcessRector applies on classes it shouldn't (like FormTypes and tests)

Created on 28 Sep 2019 · 11Comments · Source: rectorphp/rector

Running 0.5.13 on our entire codebase applies the following unexpected changes :

124) src/Form/SomeFormType.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -18,7 +18,7 @@
     public function buildForm(FormBuilderInterface $builder, array $options): void
     {
         $builder
-            ->add('q', TextType::class, [
+            ->add(['q'], TextType::class, [
    ----------- end diff -----------

127) tests/Functional/Page/PageControllerTest.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -93,7 +93,7 @@
-            static::assertGreaterThan(1, $crawler->filter('tbody tr')->count());
+            static::assertGreaterThan(1, $crawler->filter(['tbody', 'tr'])->count());
    ----------- end diff -----------

Applied rectors:

 * Rector\Symfony\Rector\New_\StringToArrayArgumentProcessRector

bug

Source

gnutix

All 11 comments

This seems like another rule making the mess.

Could you run the only rule StringToArrayArgumentProcessRector on src/Form/SomeFormType.php file only?

I have no idea why this happens here, since it should only process the Process type

TomasVotruba on 28 Sep 2019

I've added test case that passes: https://github.com/rectorphp/rector/compare/fix-array-string?expand=1#diff-09b2b6b5ee621ab0ac7b08bc7125e799

TomasVotruba on 28 Sep 2019

Ok, I'll try it out next Tuesday. Won't have time before that.

gnutix on 29 Sep 2019

👍1

Running only this rule on a single file still produces the change :

vendor/bin/rector --rule='Rector\Symfony\Rector\New_\StringToArrayArgumentProcessRector' process src/Form/Logs/Performance/PerformanceLogSearchType.php
Rector v0.5.13
Config file: rector.yaml

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

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

1) src/Form/Logs/Performance/PerformanceLogSearchType.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -15,8 +15,8 @@
     public function buildForm(FormBuilderInterface $builder, array $options): void
     {
         parent::buildForm($builder, $options);
-        $builder->add('orderBy', HiddenType::class);
-        $builder->add('orderDirection', HiddenType::class);
+        $builder->add(['orderBy'], HiddenType::class);
+        $builder->add(['orderDirection'], HiddenType::class);
     }

     public function configureOptions(OptionsResolver $resolver): void
    ----------- end diff -----------

Applied rectors:
 * Rector\Symfony\Rector\New_\StringToArrayArgumentProcessRector

 [OK] Rector is done! 1 changed files

<?php

declare(strict_types=1);

namespace App\Form\Logs\Performance;

use App\BusinessLogic\Logs\Performance\PerformanceLogSearch;
use App\Form\Logs\FileLogSearchType;
use Symfony\Component\Form\Extension\Core\Type\HiddenType;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\OptionsResolver\OptionsResolver;

final class PerformanceLogSearchType extends FileLogSearchType
{
    public function buildForm(FormBuilderInterface $builder, array $options): void
    {
        parent::buildForm($builder, $options);
        $builder->add('orderBy', HiddenType::class);
        $builder->add('orderDirection', HiddenType::class);
    }

    public function configureOptions(OptionsResolver $resolver): void
    {
        $resolver->setDefaults([
            'data_class' => PerformanceLogSearch::class,
            'csrf_protection' => false,
            'method' => 'GET',
        ]);
    }
}

gnutix on 1 Oct 2019

Failing test case is needed

TomasVotruba on 1 Oct 2019

@TomasVotruba I've tried adding my code as fixtures for StringToArrayArgumentProcessRectorTest, but the test passed... So I've added some debug in my project's rector's code instead :

diff --git a/packages/Symfony/src/Rector/New_/StringToArrayArgumentProcessRector.php b/packages/Symfony/src/Rector/New_/StringToArrayArgumentProcessRector.php
index 779219d89..16a0ba4fd 100644
--- a/packages/Symfony/src/Rector/New_/StringToArrayArgumentProcessRector.php
+++ b/packages/Symfony/src/Rector/New_/StringToArrayArgumentProcessRector.php
@@ -67,10 +67,14 @@ PHP
     public function refactor(Node $node): ?Node
     {
         if ($this->isObjectType($node, Process::class)) {
+            dump(Process::class);
+            dump($this->getObjectType($node));exit;
             return $this->processArgumentPosition($node, 0);
         }

         if ($this->isObjectType($node, ProcessHelper::class)) {
+            dump(ProcessHelper::class);
+            dump($this->getObjectType($node));exit;
             return $this->processArgumentPosition($node, 1);
         }

And the output is the following :

vendor/bin/rector process src/Form/Logs/Performance/PerformanceLogSearchType.php
Rector dev-master@ccc88b1
Config file: rector.yaml

 2/3 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓░░░░░░░░░░]  66%

"Symfony\Component\Process\Process"

PHPStan\Type\UnionType^ {#5440
  -types: array:5 [
    0 => PHPStan\Type\ObjectType^ {#4576
      -className: "Symfony\Component\Form\FormBuilderInterface"
      -subtractedType: null
    }
    1 => PHPStan\Type\ObjectType^ {#5434
      -className: "Traversable"
      -subtractedType: null
    }
    2 => PHPStan\Type\ObjectType^ {#5435
      -className: "Countable"
      -subtractedType: null
    }
    3 => PHPStan\Type\ObjectType^ {#5431
      -className: "Symfony\Component\Form\FormConfigBuilderInterface"
      -subtractedType: null
    }
    4 => PHPStan\Type\ObjectType^ {#5438
      -className: "Symfony\Component\Form\FormConfigInterface"
      -subtractedType: null
    }
  ]
}

So calling \Rector\NodeTypeResolver\NodeTypeResolver::isObjectType with a Symfony\Component\Form\FormBuilderInterface $node against Symfony\Component\Process\Process type will return true because of the Traversable interface, which is implemented by both.

I've tried doing the following change and the error is not happening anymore :

-class Process implements \IteratorAggregate
+class Process

And it's the same for the \Symfony\Component\DomCrawler\Crawler, which also extends \IteratorAggregate. So the culprit is return $resolvedType->isSuperTypeOf($requiredType)->yes(); at the end of isObjectType. Not sure how to fix that though, as I don't know the internals well enough.

gnutix on 2 Oct 2019

Good catch!

Please send the test case as a PR, I'll try to fix the isObjectType() issue and make it fail.

TomasVotruba on 2 Oct 2019

As I said, I didn't manage to have a failing test case. And I'm not sure how to add a failing test on isObjectType (which Test file - as there's like 8 of them -, how to call the method, mock the arguments, etc). That's why I explained you the issue the best I could instead.

gnutix on 2 Oct 2019