Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unsafe usage of new static() errors #136

Closed
1 of 4 tasks
davereid opened this issue Feb 17, 2020 · 6 comments · Fixed by #187
Closed
1 of 4 tasks

Unsafe usage of new static() errors #136

davereid opened this issue Feb 17, 2020 · 6 comments · Fixed by #187
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@davereid
Copy link

davereid commented Feb 17, 2020

How is drupal-check installed?

  • drupal-check is installed using the phar
  • drupal-check is installed globally via Composer
  • drupal-check is installed globally using consolidation/cgr
  • drupal-check is installed as a dependency to my project

Environment:

  • OS: Linux
  • PHP Version: 7.3
  • Drupal core: 8.8.x

Describe the bug
Getting errors with standard Drupal plugin code which implements the \Drupal\Core\DependencyInjection\ContainerInjectionInterface::create() method:

Unsafe usage of new static().
         💡 Consider making the class or the constructor final.

The code is usually implemented in the following way:

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container) {
    return new static(
      $container->get('entity_type.manager')
    );
  }

Should this error be excluded from drupal-check runs? Changing all of our code to use new self instead of new static means we have a lot of replacement to do, and fixing a lot of core code...

Console output
vendor/bin/drupal-check -a web/modules/custom

@davereid
Copy link
Author

For reference: phpstan/phpstan#2773

@Sergey-Orlov
Copy link
Contributor

Any clues how to avoid these errors?

@fgm
Copy link

fgm commented Mar 25, 2020

No idea how phpstan works, but if it was hookable, the extension could check whether this is either a ContainerInjectionInterface or ContainerFactoryPluginInterface, and pass the check in that case; which should cover most such cases.

@Dropa
Copy link

Dropa commented May 6, 2020

Following this, since as far as I know, new static is intentional solution. Changing it to new self would mean that every child class would have to override create() function, which in some cases, such as plugins would be huge pain in the arse.

While I agree with @fgm I don't think there is no other function than create() where new static would need to be allowed

@mglaman
Copy link
Owner

mglaman commented May 11, 2020

We need to alter the ignoreErrors value. This causes too many problems in Drupal were we always use new static instead of self.

@mglaman mglaman added enhancement New feature or request good first issue Good for newcomers labels May 11, 2020
MPParsley added a commit to MPParsley/drupal-check that referenced this issue Oct 13, 2020
In mglaman#136 we accidentally ignored all errors instead of just the one error:
"Unsafe usage of new static()."

This results in drupal-check failing with the following message:
 -- -------------------------------------------------------------------------- 
     Error                                                                     
 -- -------------------------------------------------------------------------- 
     Ignored error #Unsafe usage of new static()# has an unescaped '()' which  
     leads to ignoring all errors. Use '\(\)' instead.                         
 -- --------------------------------------------------------------------------
@MPParsley
Copy link
Contributor

Note that the failing tests hid this error:

 -- -------------------------------------------------------------------------- 
     Error                                                                     
 -- -------------------------------------------------------------------------- 
     Ignored error #Unsafe usage of new static()# has an unescaped '()' which  
     leads to ignoring all errors. Use '\(\)' instead.                         
 -- -------------------------------------------------------------------------- 

I opened a PR to fix this in #191.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants