Problem/Motivation

phpstan reports,

Unsafe usage of new static().
💡 See: https://phpstan.org/blog/solving-phpstan-error-unsafe-usage-of-new-static

on AdminToolbarSearchController, ToolbarController, ExtraLinks, and MenuLinkEntity.

Steps to reproduce

phpstan analyse admin_toolbar

Proposed resolution

In AdminToolbarSearchController, ToolbarController, and MenuLinkEntity, use the $instance = parent::create() pattern instead of implementing __construct(). In ExtraLinks, mark __construct() as final.

Remaining tasks

make an MR

Issue fork admin_toolbar-3514075

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

benstallings created an issue. See original summary.

benstallings opened merge request !126

benstallings’s picture

benstallings commented
Assigned: benstallings » Unassigned
Status: Active » Needs review
dydave’s picture

dydave commented
Version: 3.5.3 » 3.x-dev
Status: Needs review » Needs work

@benstallings:

Any chance we could get this refactored as well in the MR?
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...

Thus, removing any occurrence of new static in the module.

Or maybe it is because the constructor is set final that we need to keep this pattern for this class?
Your opinion on that would be greatly appreciated.

Otherwise, the MR looks great!

Thanks in advance!

dydave’s picture

dydave commented

@benstallings: Re #4

OK, I've taken a closer look at the changes and understand now why the constructor had to be made final:

The class ExtraLinks extends abstract class DeriverBase which does not have a create method.
Therefore in order to implement dependency container injection, we need to create a new container using the new static syntax and make its constructor final, to fix the PHPSTAN unsafe warning/error.

Question:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/126/di...
It looks like this "could" break backward compatibility 🛑

If users of the module extended the ExtraLinks class and implemented a __constructor method, then the module "could" potentially break their sites.

We can't really know for sure but there "could" be other contrib modules extending admin_toolbar, using this class and constructor, for example.

I don't know what the exact policy and suggested approach is for handling such a breaking change, but I would tend to think this would have to be part of new minor or major release, I "suppose"...

I've seen there are other ways to fix this error:
https://www.drupal.org/docs/develop/development-tools/phpstan/handling-u...

Making the constructor Final instead of the entire class.
Making the constructor abstract.
Enforcing the constructor signature through an interface.
Using the @phpstan-consistent-constructor tag.

Maybe the 4th option could work? "Using the @phpstan-consistent-constructor tag." in the class doc comment block.

Could we maybe use this instead?

Otherwise if none of them would make sense for the module/class, then I would still prefer ignoring this PHPSTAN error until this breaking change could be introduced in a major release, for example.

I would greatly appreciate to have your opinion and feedback on that.
Thanks in advance!
 

Useful documentation links:
https://phpstan.org/blog/solving-phpstan-error-unsafe-usage-of-new-static

benstallings’s picture

benstallings commented

Hi, @DYdave. I am no expert on this issue, unfortunately. My (limited) understanding is that once a class implements create(), it is preferable for any extending class to also use create() and not re-implement __construct(). Unless, that is, it uses AutowireTrait, in which case you want to implement __construct() and not create()! https://www.drupal.org/node/3396179 It's a dilemma. I defer to the judgment of someone who better understands the goals of this particular module.

  • dydave committed 4dc4d910 on 3.x authored by benstallings 
    Issue #3514075 by benstallings, dydave: Fixed PHPSTAN validation error '...
dydave’s picture

dydave commented
Status: Needs work » Fixed
Related issues: +#3405618: GitLab CI - PHPStan - "Unsafe usage of new static()"

Thanks Ben (@benstallings) for taking the time to reply, it's greatly appreciated.

OK, I think we're going to move forward with this change: making the constructor final.

I feel a bit more confident about the BC issue after looking again at #3405618-9: GitLab CI - PHPStan - "Unsafe usage of new static()", related documentation Extending Smart Trim, and the fact I couldn't find any other contrib module that would seem to extend module's Extralinks class, see the following search on the Gitlab Drupal Code base:
https://git.drupalcode.org/search?group_id=2&scope=blobs&search=Plugin%5...

If users need to extend the class, they should be able to do so with the various methods described in the documentation and tickets mentioned in this comment.

Therefore, I went ahead and merged the changes above at #7.

This issue will be added to the notes of the next release and "should" provide a base for the documentation of this change.

Marking issue as Fixed, for now.

Thanks again Ben (@benstallings) for reporting the issue, contributing the solution and working through the review of the changes. 🙂

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.