unsafe usage of new static()

@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...

@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."

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 the 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!

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.

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. 🙂

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