rectorphp / rector

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

Incorrect behavior of FuncCallToStaticCallRector #6527

Closed cavo789 closed 3 years ago

cavo789 commented 3 years ago

Bug Report

If the class doesn't extends an ancestor, it's working; if it extends a class, I get a fatal error, am I missing something?

Subject Details
Rector version rector/rector:php7.4, just downloaded right now
Installed as docker

I'm running Rector on my machine like below and nothing happens while should be (I'm refactoring an old Laravel 5.5 app; first migrating to Laravel 5.8)

docker run --rm -v "$(pwd)":/project rector/rector:php7.4 process app/Http/Controllers/Ajax/Stats/GlobalController.php --dry-run --config .config/rector-laravel58.php

Since nothing happens, I've tried the demo site and there I've a fatal error. By simplifying the code as much as possible, I can make it working but ...

Minimal PHP Code Causing Issue

This code is working: https://getrector.org/demo/1ebd5a44-4ed0-6dea-8723-d75ce3070cc1

As you can see below, the GlobalController class don't extends a parent.

<?php
use App\Http\Controllers\Controller;
class GlobalController {
    protected function demoFunction()  {
        $filename = str_slug('ipso lorem') . '.xls';
    }
}

If I use the code below, it doesn't work anymore due to the presence of extends Controller and the demo site is returning a Fatal error: Syntax error:

<?php
use App\Http\Controllers\Controller;
class GlobalController extends Controller  {
    protected function demoFunction()  {
        $filename = str_slug('ipso lorem') . '.xls';
    }
}

By adding use App\Http\Controllers\Controller; at the top, I still get the fatal error.

Am I missing something?

Many thanks!

Responsible rules

Expected Behavior

Adding the extends Controller parent should make the test to work and suggest to replace str_slug to \Illuminate\Support\Str::slug

samsonasik commented 3 years ago

I can't reproduce it, it seems working ok with Controller class simulation, ref https://getrector.org/demo/1ebd5a8e-2528-6156-b526-dfac5d4e3ccb .

Could you create faiilng test case under https://github.com/rectorphp/rector-laravel/tree/main/tests ? You possibly can see rector-doctrine "Set" tests for inspiration https://github.com/rectorphp/rector-doctrine/tree/main/tests/Set .

cavo789 commented 3 years ago

Many thanks Abdul.

I see you've changed the code.. Ok, that's fine and it's useful to me to understand that rector need to know who is the parent.

The code below (your's but smaller) is working

<?php

class BaseController {}

class GlobalController extends BaseController  {
    protected function demoFunction()  {
        $filename = str_slug('ipso lorem') . '.xls';
    }
}

If I remove the class BaseController {} line (https://getrector.org/demo/1ebd5b0e-fcbe-63d8-bb70-9123fa2a2a50), it doesn't work.

<?php
class GlobalController extends BaseController  {
    protected function demoFunction()  {
        $filename = str_slug('ipso lorem') . '.xls';
    }
}

I'll make a few changes in my legacy codebase to see if rector will suggest to replace str_slug in a dry-run mode. If so, this would seem to indicate a problem with the auto-loader.

Many thanks, I've now a direction to search for.

cavo789 commented 3 years ago

Solved thanks your help @samsonasik πŸ‘

The problem was indeed with the autoloader. Rector has to know who is the ancestor. I was not aware of this. By running rector with the --verbose parameter, it was clear.

My real use:

  1. I'm hosting my repository on my WSL2 partition (Windows)
  2. I'm using Docker to run my code (and some folders are only inside the container like node_modules and vendor)
  3. I'm running rector from my host machine (and since /vendor is only present in the container, rector can't access to the vendor/autoload.php file)

To solve my problem:

  1. Share /vendor between the container and the host (done in my docker-compose.override.yml file)
  2. Add --autoload-file vendor/autoload.php when running rector

This command gives now the expected result i.e. show me deprecated code and his replacement:

 docker run --rm -v "$(pwd)":/project rector/rector:php7.4 process app/Http/Controllers/Ajax/Stats/GlobalController.php --dry-run --config .config/rector-laravel58.php --autoload-file vendor/autoload.php

Note

@TomasVotruba

Perhaps not easy to solve this but, running the command below

 docker run --rm -v "$(pwd)":/project rector/rector:php7.4 process app/Http/Controllers/Ajax/Stats/GlobalController.php --dry-run --config .config/rector-laravel58.php

gives this output:

 0/4 [β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘]   0%
 2/4 [β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘β–‘]  50%
 4/4 [β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“] 100%

 [OK] Rector is done!

Cool, everything is ok ... But no, that's not true. By adding --verbose

 docker run --rm -v "$(pwd)":/project rector/rector:php7.4 process app/Http/Controllers/Ajax/Stats/GlobalController.php --dry-run --config .config/rector-laravel58.php --verbose

here is the output:

[parsing] app/Http/Controllers/Ajax/Stats/GlobalController.php

In IdentifierNotFound.php line 24:

  [PHPStan\BetterReflection\Reflector\Exception\IdentifierNotFound]
  PHPStan\BetterReflection\Reflection\ReflectionClass "App\Http\Controllers\C
  ontroller" could not be found in the located source

In fact PHPStan can't detect the parent, this lead Rector to think "it's ok, nothing to do" but, yes, that's not true.

samsonasik commented 3 years ago

I am closing it then, feel free to provide PR for improvement.