sirbrillig / phpcs-variable-analysis

Find undefined and unused variables with the PHP Codesniffer static analysis tool.
Other
136 stars 14 forks source link

Incorrect warning for arguments to static methods that return "new static()" #272

Closed rickdenhaan closed 2 years ago

rickdenhaan commented 2 years ago

After running composer up we received an updated version of phpcs-variable-analysis from v2.11.4 to v2.11.5 (no other PHP_CodeSniffer-related updates). Since then we're receiving warnings in custom services in our Drupal application:

FILE: modules/custom/some_module/src/Controller/ResultResponseController.php
--------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 22 WARNINGS AFFECTING 22 LINES
--------------------------------------------------------------------------------------------------------
 343 | WARNING | Unused static variable $container.
 345 | WARNING | Redeclaration of function parameter $container as static variable.
 346 | WARNING | Redeclaration of static variable $container as static variable.
 347 | WARNING | Redeclaration of static variable $container as static variable.
...etc

These warnings are about this code inside a PHP class:

public static function create(ContainerInterface $container): ResultResponseController {
  return new static(
    $container->get('entity_type.manager'),
    $container->get('form_builder'),
    $container->get('messenger'),
    $container->get('request_stack'),
    // ...etc
  );
}

We can work around the errors by explicitly naming the class we want to return (i.e. return new ResultResponseController() but this will be inaccurate for some locations where this static ::create method exists in an abstract base class.

Could this be caused by the recent change to static declaration checks (https://github.com/sirbrillig/phpcs-variable-analysis/pull/267)?

mkalkbrenner commented 2 years ago

Same here. The proposed workaround helps. But I agree that it is not a good solution to move and copy all create() functions from abstract base classes to all children.

KoenvanMeijeren commented 2 years ago

Same here. But I get also false positives for code pieces like this:

$definitions = $plugin_manager->getDefinitions();

return array_map(static function ($definition) {
   return $definition['label'];
}, $definitions);
sirbrillig commented 2 years ago

Sorry for the regression! Thanks for the fix, @arkener! I'll put out a patch release shortly as soon as I check out https://github.com/sirbrillig/phpcs-variable-analysis/issues/271