rectorphp / rector

Instant Upgrades and Automated Refactoring of any PHP 5.3+ code
https://getrector.com
MIT License
8.71k stars 687 forks source link

Bug: laravel57 ruleset ArgumentRemoverRector rule seems incorrectly configured #1598

Closed Levivb closed 5 years ago

Levivb commented 5 years ago
Subject Details
PHP version PHP 7.2
Full Command vendor/bin/rector --config rector.yml process SomeClass.php -n
# rector.yml
parameters:
  php_version_features: '7.2'

imports:
  - { resource: '../../vendor/rector/rector/config/level/laravel/laravel57.yaml' }

The internal rule causing incorrect behaviour:

    Rector\Rector\Argument\ArgumentRemoverRector:
        Illuminate\Foundation\Application:
            1:
                name: 'options'

See: https://github.com/rectorphp/rector/blob/master/config/set/laravel/laravel57.yaml#L23

Current Behaviour

Rector v0.5.5
Config file: /my-project/rector.yml

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

 [ERROR] Could not process "Support/Localization/LocaleAccessor.php" file by "Rector\Rector\AbstractRector", due to:
         "Argument 2 passed to Rector\Rector\AbstractRector::isName() must be of the type string, integer given, called
         in /my-project/vendor/rector/rector/src/Rector/Argument/ArgumentRemoverRector.php on line 78".

Minimal PHP Code Causing Issue

<?php

use Illuminate\Foundation\Application;

class SomeClass
{
    /** @var Application */
    protected $application;

    public function __construct()
    {
        $this->application->asd();
    }
}

Expected Behaviour

I'm not sure what the configured rule is trying to accomplish:

    Rector\Rector\Argument\ArgumentRemoverRector:
        Illuminate\Foundation\Application:
            1:
                name: 'options'

But looking at the refactor method of ArgumentRemoverRector:

    public function refactor(Node $node): ?Node
    {
        foreach ($this->positionsByMethodNameByClassType as $type => $positionByMethodName) {
            if (! $this->isType($node, $type)) {
                continue;
            }

            foreach ($positionByMethodName as $methodName => $positions) {
                if (! $this->isName($node, $methodName)) {
                    continue;
                }

                foreach ($positions as $position => $match) {
                    $this->processPosition($node, $position, $match);
                }
            }
        }

        return $node;
    }

we can determine the following:

$this->positionsByMethodNameByClassType = [
  'Illuminate\Foundation\Application' => [1 => ['name' => 'options]]
]

// which means in foreach $this->positionsByMethodNameByClassType
$type = 'Illuminate\Foundation\Application'
$positionByMethodName = [1 => ['name' => 'options]]

// and within foreach $positionByMethodName
$methodName = 1
$positions = ['name' => 'options]

// which crashes
$this->isName($node, $methodName);
// since second parameter of isName should be string, (int)(1) given

That is - if I'm correct. Can you check what is supposed to happen?

TomasVotruba commented 5 years ago

There is method name missing in a config :man_facepalming:

See: https://laravel.com/docs/5.7/upgrade

image

Thanks for reporting :+1:

Levivb commented 5 years ago

Thanks for fixing this and the other reports this quickly!