rectorphp / rector

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

SimplifyStrposLowerRector isn't safe and should be removed #8618

Closed kkmuffme closed 4 months ago

kkmuffme commented 4 months ago

Bug Report

Subject Details
Rector version 1.0.4

SimplifyStrposLowerRector underlying assumption is wrong, and the rule should be deprecated https://getrector.com/demo/fbdf29d5-c428-498f-aead-30ad8e600f61

Minimal PHP Code Causing Issue

final class DemoFile
{
    public function run(string $param, string $param2)
    {   
        if (strpos(strtolower($param), $param2) !== false) {
            return 'hello';
        }

        return 'world';
    }

    public function runI(string $param, string $param2)
    {   
        if (stripos($param, $param2) !== false) {
            return 'hello';
        }

        return 'world';
    }
}

$demo_file = new DemoFile();
echo $demo_file->run('Abc', 'aBc') . PHP_EOL;
echo $demo_file->runI('Abc', 'aBc');

This will result in

world
hello

Expected Behaviour

Using strpos with strtolower is not the same as stripos. Therefore changes done by SimplifyStrposLowerRector are not correct in many cases (= all cases where the 2nd argument may contain any non-lowercase characters)

samsonasik commented 4 months ago

I think the rule still fine when 2nd argument validated as only string with all lowercased