Skip to content

Conversation

zonuexe
Copy link
Contributor

@zonuexe zonuexe commented Dec 15, 2023

Generally when we pass a class name to a container we expect only objects of that class to be returned, not mixed.

In particular, PHPStan normalizes the T|mixed type to mixed, so the old type would not work.

Conditional return types notation is supported by both Psalm and PHPStan and provides a more appropriate type for both.

I've been using this patch for over a year now and it's working fine with PHPStan in our application.

@mnapoli
Copy link
Member

mnapoli commented Dec 15, 2023

That looks very interesting, thanks!

2 questions:

  • can you confirm that if I retrieve a non-class (e.g. a string parameter like $container->get('db.host')) then the inferred return type will be mixed?
  • should this be annotated on PSR-11's ContainerInterface instead?

@zonuexe
Copy link
Contributor Author

zonuexe commented Dec 17, 2023

can you confirm that if I retrieve a non-class (e.g. a string parameter like $container->get('db.host')) then the inferred return type will be mixed?

You can check that these types are accepted with code like the following:

should this be annotated on PSR-11's ContainerInterface instead?

Logically it is possible, and in our application we add the same type to PSR-11 and PSR-7 ServerRequest with composer-patches.

However, the PSR documentation does not define the semantics of these $ids, so it appears that adding them to the interface will require modifying PSR. In my opinion, it is enough to extend types at the responsibility of libraries and applications rather than recommendations.

@mnapoli
Copy link
Member

mnapoli commented Dec 17, 2023

🤔 right. Unfortunately, I think the same logic applies to this class and PSR-11. I'd rather be consistent with PSR-11.

@MarcHagen
Copy link

MarcHagen commented Dec 18, 2023

Found this PR, and we are having the same "problem" (first world problems right 😅).

Don't know if the playground could be better, but it reflects the error given ignore others
https://phpstan.org/r/5ed37a8b-e8d6-423e-a78d-69ccdc5ce1af

Property TestClass::$app (App) does not accept mixed.

I my opinion the package has changed the PSR-11 PHPDoc to add the generics in PHP-DI.
If you think if should be consistent with the PSR-11 then remove the generics.

But the conditional return will fix this issue.
All wouldn't I like the psalm-specific syntax. @return will do fine.

     * @return ($id is class-string<T> ? T : mixed)

@mnapoli
Copy link
Member

mnapoli commented Dec 19, 2023

Closing per comment above.

@mnapoli mnapoli closed this Dec 19, 2023
@staabm
Copy link

staabm commented Dec 26, 2023

@zonuexe @MarcHagen I guess this PHPStan extension implements what you try to achieve

https://github.com/bnf/phpstan-psr-container

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.

4 participants