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

Add generics to ContainerInterface (new proposal) #50

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

loic425
Copy link

@loic425 loic425 commented Jan 11, 2024

Inspired by #44

I've tried @nicolas-grekas' proposal but I do not have autocompletion without adding "@param" annotation.

@loic425 loic425 changed the title Add generics to ContainerInterface Add generics to ContainerInterface (new proposal) Jan 11, 2024
src/ContainerInterface.php Outdated Show resolved Hide resolved
*
* @throws NotFoundExceptionInterface No entry was found for **this** identifier.
* @throws ContainerExceptionInterface Error while retrieving the entry.
*
* @return mixed Entry.
* @return T|mixed Entry.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have use same return type and works amazing with @phpstan as stub

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done :)

@snapshotpl
Copy link
Contributor

Now works well with PHPStan https://phpstan.org/r/03c35ee8-10f5-490c-a263-37855651e35d
And Psalm https://psalm.dev/r/8b5c98aa39

@KorvinSzanto
Copy link

I'm a big fan of generics and I think a container that implements this functionality should feel free to apply this to their container implementation. I think applying them here however doesn't quite make sense and goes against the spirit of this PSR.

For one PSR-11 says:

An entry identifier is an opaque string, so callers SHOULD NOT assume that the structure of the string carries any semantic meaning.

Which to me heavily implies that a class string key should not automatically result in an instance of that class. By typehinting ($id is class-string<T> ? T : mixed) we are requiring the return type to be an instance of a class if the key looks like a class string and preventing valid usage like ->get('\Exception') // 'Exception is the base class for all user exceptions'. ($id is class-string<T> ? T|mixed : mixed) could work instead, but I still think that goes against the quoted text by implying there's some potential structure to keys.

What's more, PSR-11 recommends:

Users SHOULD NOT pass a container into an object so that the object can retrieve its own dependencies. This means the container is used as a Service Locator which is a pattern that is generally discouraged.

And I'd argue having generics on the base get method implies the opposite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants