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 #20076

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add generics #20076

wants to merge 1 commit into from

Conversation

markhuot
Copy link
Contributor

Adds generics so $container->get(Foo::class) returns a Foo instance. This should help IDE's like PHPStorm reason about what's coming out of ->get(). It will also help static analysis tools like PHPStan.

Q A
Is bugfix?
New feature? ✔️
Breaks BC?
Fixed issues n/a

Adds generics so `$container->get(Foo::class)` returns a `Foo` instance
Copy link

what-the-diff bot commented Nov 30, 2023

PR Summary

  • Enhancing Documentation in Container Class
    The descriptive block, known as a PHPDoc comment, associated with the Container class was improved. This helps anyone reading the code understand its purpose and usage.

  • Introduction of Generic Typing
    A special type of notation called @template was added. This allows defining a common structure that can hold any type of data. This addition improves the code's adaptability.

  • Enhancements in Parameter Annotations
    The parameter annotations in the class, which serve to describe the type and significance of parameters, were updated to accommodate for generic typing, increasing the flexibility and reusability of the code.

  • Amendments in Return Type Annotations
    The return type annotations, which define what type of data a function gives back when called, have been adjusted to support generic typing. This change assures that the function will return consistent and predictable data.

Copy link

codecov bot commented Nov 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d43341a) 48.02% compared to head (fca3d05) 40.65%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #20076      +/-   ##
==========================================
- Coverage   48.02%   40.65%   -7.38%     
==========================================
  Files         445      445              
  Lines       43889    43889              
==========================================
- Hits        21077    17841    -3236     
- Misses      22812    26048    +3236     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -145,7 +145,8 @@ class Container extends Component
* In this case, the constructor parameters and object configurations will be used
* only if the class is instantiated the first time.
*
* @param string|Instance $class the class Instance, name, or an alias name (e.g. `foo`) that was previously
* @template T
* @param class-string<T>|Instance $class the class Instance, name, or an alias name (e.g. `foo`) that was previously
Copy link
Member

Choose a reason for hiding this comment

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

Is that Psalm syntax?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this should work in Psalm,

https://psalm.dev/docs/annotating_code/templated_annotations/#param-class-stringt

I've tested and used this frequently in PHPStorm and PHPStan. No first-hand experience in Psalm, but the docs imply the syntax is the same. That said I'm getting an error in their playground,

https://psalm.dev/r/da35f91c85

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Error seems to be because Pslam is more strict and won't let you new a generic class-string because we don't know what the constructor requires. In this case Yii's DI will handle all that for us so suppressing the Psalm error should be fine and would be handled by other tests/types elsewhere.

Related issue, vimeo/psalm#8628

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.

2 participants