laminas / laminas-di

Automated dependency injection for PSR-11 containers
https://docs.laminas.dev/laminas-di/
BSD 3-Clause "New" or "Revised" License
36 stars 20 forks source link

DEFAULT CONTAINER: Update docblock return type of has method #47

Closed sjokkateer closed 2 years ago

sjokkateer commented 2 years ago

Signed-off-by: Remy Bos 27890746+sjokkateer@users.noreply.github.com

Q A
Documentation yes
Bugfix no
BC Break no
New Feature no
RFC no
QA no

Description

This PR updates the return type in the docblock for the DefaultContainer::has() method. Currently it is said to return mixed, while both the implemented ContainerInterface::has() is said to return a bool, as the method itself.

sjokkateer commented 2 years ago

I was also wondering if it is alright to update the return type of DefaultContainer::get() to the more specific object since it only seems to return objects?

Or should changing the return type of correct docblocks be considered breaking as well since static analysers might start complaining?

Ocramius commented 2 years ago

since it only seems to return objects?

I remember the container being able to be populated with non-object types too?

Ocramius commented 2 years ago

Thanks @sjokkateer!

sjokkateer commented 2 years ago

since it only seems to return objects?

I remember the container being able to be populated with non-object types too?

Let me retry @Ocramius, in DefaultContainer::__construct() objects are registered as services. Registering a new service through DefaultContainer::setInstance() is documented to only accept type object as param $service. Further, in DefaultContainer::get() the forward call to the InjectorInterface::class implementation, according to the code of InjectorInterface::create() this method should return an object.

This makes it seem that all the branches return an object type, therefore the return type based on PHP 7.4's covariance

Covariance allows a child's method to return a more specific type than the return type of its parent's method.

could be made more specific to object instead of the current mixed, however, this might introduce issues with static analysis since users might have extended the DefaultContainer::class and rely on a mixed return type somehow?

Ocramius commented 2 years ago

The main problem with adding type declarations is that these classes are not final , and we would therefore break subclasses

sjokkateer commented 2 years ago

@Ocramius Thanks for your response but I am mainly wondering about return types in docblocks at the moment and would really appreciate if you could help take away my confusion.

Say in this case the docblock's return type would have been incorrect, would an update of the docblock's return type happen according to the same rules? By incorrect I would as an example assume that the author wanted to narrow down the return type in the docblock but forgot to do so.

Ocramius commented 2 years ago

Fine to add these to docblock! :+1:

sjokkateer commented 2 years ago

@Ocramius Thanks for the info!