Template container

[ro0NL]

fixes #34

[Jean85]

What's the value in this? Without using the template somehow, it doesn't add any value IMO...

[ro0NL]

See https://github.com/php-fig/container/issues/34#issuecomment-821883308

[ChristophWurst]

Let me chime in here as well. I am also worried that this doesn't add any value. If you type the whole container for a certain type, then its use is very limited as it can always just retrieve one type.

If you have to annotate the template parameter of the container for each time that you use the container for a certain type, then you can also just /** @var XYZ */ the retrieved value. That is possible without any changes on the PSR interface. IDEs and static analysis can work with this information.

[ondrejmirtes]

I agree, this isn't very useful, this isn't a case for generics. Services from containers will always have different types.

[ciaranmcnulty]

This is the wrong contract for the FIG container - the ID does not have to be a class-string nor are there any guarantees around the types you get back

[mnapoli]

the ID does not have to be a class-string

To be fair the PR has nothing to do with class-string.

[ro0NL]

Here's an example use case i have: https://symfony.com/doc/current/service_container/service_subscribers_locators.html#defining-a-service-subscriber

For interop reasons, let's assume the argument type Psr\Container\ContainerInterface will not change. As we aim for compat with any potential container implementation.

The current situation is untyped. Meh.

With this PR we can do cool stuff like: https://psalm.dev/r/f62a62376f

Typesafety. Yay.

[mnapoli]

Oh ok that's pretty cool!

My position would be that if there are no downsides to anyone, why not merge this?

[ondrejmirtes]

There are downsides. The interface is gonna become generic, so PHPStan is gonna rightfully complain that the template types must be specified every time the interface is implemented or typehinted. So adding <string, mixed> would be annoying.

[ro0NL]

im not sure, i figured the current status quo remains if people dont actually annotate.

If the consumer annotates a container, but the caller (eg. a framework) passes an untyped one ... that's a violation :shrug: It's not the scope of this PR per se.

PHPStan is gonna rightfully complain that the template types must be specified every time the interface is implemented or typehinted

I see. Looks like this is less of an issue for psalm? https://psalm.dev/r/a018fb2de8 (vs. https://phpstan.org/r/035d044e-e389-4b69-a78e-23a9817f6635)

[mnapoli]

PHPStan is gonna rightfully complain that the template types must be specified every time the interface is implemented or typehinted.

Right, that's a real downside I would say.

[ro0NL]

for phpstan users yes :)

the question to me is is psam too loose, or phpstan too strict? How can 2 tools in the same scope behave different in this regard.

[ondrejmirtes]

I don't want users to forget to specify the type variables of an interface they implement or typehint, it might hide bugs. TypeScript does the same thing, see: https://www.typescriptlang.org/play?#code/JYOwLgpgTgZghgYwgAgOIRNYCCSATDMYMATwDEQAeAFRIAcIA+ZAbwChlkAKOKAcwBcyWgwCUQkRADcbAL5s2MAK4gERAPYhkMdep78h6TFGz5CxciHHIAbuuB5WHZHKA

[ro0NL]

typescript allows the same thing?

Playground Link

[ondrejmirtes]

You used a generics "type parameter default" feature which has different semantics from <Type extends String>: https://www.typescriptlang.org/play?ssl=1&ssc=49&pln=1&pc=28#code/JYOwLgpgTgZghgYwgAgOIRNYCCSATDMYMATwDEQAeAFRIAcUIAPSEPAZ2QGUwpQBzAHzIA3gChkyABRwo-AFzJaDAJSLlEANxiAvmLEwAriAREA9iGQwzZmXMXpMfXAXDFyINcgBuZ4HlEJZF0gA and that isn't yet supported by neither PHPStan nor Psalm.

[ro0NL]

I see. I believe what psalm defines as T as string is actually Type extends String = String in typescript :exploding_head:

Also psalm understand T is string which is indeed a different semantic :+1:

Then, i'm not sure how to proceed. I think what psalm does is reasonable, hence this PR works IMHO. Remains the difference between 2 tools in PHP ...

[ondrejmirtes]

I still don't think this is that useful even if PHPStan chose not to report the errors. Typically you have dozens or even hundreds of services registered in the container so the example with ContainerInterface<'ok'|'also-ok', mixed> might look useful but in practice it would be cumbersome to update all the places in the code every time a new service is added or removed.

This is much better served by a custom framework-specific PHPStan/Psalm rule that checks the arguments passed to ContainerInterface::get(), like phpstan-symfony does and reports 'Service "%s" is not registered in the container.'. Same goes for inferring the actual type returned from ContainerInterface::get().

I understand you might be using this interface in a place where these generic type parameters might make sense, but that's a very narrow use-case that would better be served by a custom stub file registered in your project (or using a different interface that you can annotate as you please).

[ro0NL]

The ContainerInterface<'ok'|'also-ok', mixed> was just an example possibility. I understand it's not really practical to expect a container with only those 2 entries, per se.

This also isnt about services nor frameworks, but merely "container entries". Subtle ... :)

I think i'll give up then. Perhaps service locators are an anti-pattern after all :shrug: as we cannot do @param ContainerInterface<class-string, object> $c at the interop level.