rectorphp / type-perfect

Next level type declaration check PHPStan rules
https://getrector.com/blog/introducing-type-perfect-for-extra-safety
MIT License
74 stars 4 forks source link

no_mixed false positive #30

Closed istvan-ujjmeszaros closed 3 months ago

istvan-ujjmeszaros commented 3 months ago

This code (it is from PHPMailer) gives a mixed variable warning about $configurator->configure(), but the type should be already known there.

public static function mailer(string $dsn, bool $exceptions = null): PHPMailer
{
    static $configurator = null;

    if (null === $configurator) {
        $configurator = new DSNConfigurator();
    }

    return $configurator->configure(new PHPMailer($exceptions), $dsn);
}
TomasVotruba commented 3 months ago

Report is correct. The $configurator can be anything, e.g. "notAnOjbect" string :)

This should be fixed:

    if (! $configurator instanceof Configurator) {
        $configurator = new DSNConfigurator();
    }

   // here we always get "Configurator" instance
samsonasik commented 3 months ago

$configurator already initialized in static variable, and values is always DSNConfigurator on usage, I think this should be skipped, see https://3v4l.org/OZ9YI

samsonasik commented 3 months ago

it seems phpstan bug see

https://phpstan.org/r/e29dd2fe-4416-44c7-a08d-dbddf3731d22

vs

https://phpstan.org/r/10bf3731-bf06-44ea-9627-2801fe56f546

so it needs to be solved in phsptan first.

istvan-ujjmeszaros commented 3 months ago

Thanks guys, I didn't like PHPMailer's source code in general, but couldn't tell why. This was a good example for bad type checking, instanceof definitely is the proper method to use here. I need some time to get back on track with PHP after skipping it for almost 15 years. And I will replace PHPMailer with Symfony Mailer.