Fix "Unsafe usage of new static()" errors reported by phpstan

[ADmad]

phpstan currently reports error for statements like new static(..) in various class. For e.g. in Session class:

cakephp/src/Http/Session.php

Line 127 in e3c728d

return new static($sessionConfig);

The reason is if someone extends the class and changes the constructor signature the new static() calls could start generating errors.

There are two ways to fix this error, which one should we chose?

1. Add final keyword to the class constructor
2. Change new static() to new self().

[ADmad]

IMO if someone is extending the class they would want it to produce instance of that class instead of the parent, so using new self() wouldn't be prudent.

So next question is if we are to add final keyword to the constructors then would it be okay to do so in 4.0 or we should do it in 4.1?

[othercorey]

I agree that new self() is a poor choice.

I think it depends on whether we provide an initialization method (or something).

[garas]

Adding final keyword is backwards incompatible change as it would be Fatal error: Cannot override final method Session::__construct() if somebody has extended the class.

[ADmad]

@garas Yes, adding final would be a backwards incompatible change.

It's unfortunate the there's no way in PHP to only allow overriding the constructor without changing it's signature.

[ADmad]

Other projects are grappling with the same issue https://www.drupal.org/project/drupal/issues/3118975.

[othercorey]

I don't know what drupal's code looks like, but I don't see how they never want to extend a class that they instantiate with new static().

The problem we'll have is when we have cases where we need to override the constructor internally. We can't just remove the final keyword because cake makes everything public use. Users will simply extend the base class and cause an issue.

We need classes that have proper initialization routines so we don't have a problem with making base class constructors final. I don't know if we have this situation anywhere internally.

[dereuromark]

if someone extends the class and changes the constructor signature the new static() calls could start generating errors.

I dont think we need to do anything here.
In general, when you extend classes you own the responsibility of doing so in a manner that still reflects useful usage of it.
So, overly trying to project devs by just closing those classes using final or alike sound like the opposite of useful.

new self could be done maybe in the next minor or major.
But that itself seems also already a bit far stretched in terms of trying to protect developers from themselves.

Personally, where this is relevant, I always use the method factory pattern

// internally then calls new static::__construct()
ItemDto::create($args)

as those can be properly interfaced.
And the constructor here is either not used, or even made private/protected.

[dereuromark]

So, are we OK using an interface contractable factory method as I suggested?

return static::create($sessionConfig); 

That should solve it without further hazzle and is BC enough as the added interface method is also implemented right away in the base class.

[ADmad]

I am inclined to not change anything. Just accept that what we have currently technically can cause issues but in practice anyone changing the constructor signature is expected to change related new static() calls too.

[othercorey]

I think this is a 5.x design question around initialization interfaces. Unless everything is wrapped in a factory, new self isn't a great choice right now.

[dereuromark]

Adding to 5.0 topics in wiki and close?