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

Uses @template-covariant in collections #1682

Conversation

Grldk
Copy link

@Grldk Grldk commented Jul 3, 2023

  • Added or updated tests
  • Documented user facing changes

Changes

Changes Collection TValue from @template to @template-covariant

see, laravel/framework#46872

I am not sure if this change needs added tests. All existing tests pass.

@Grldk
Copy link
Author

Grldk commented Jul 6, 2023

Adding @template-covariant in EloquentBuilder.stub would solve a number of problems for me as well. Should I create a separate PR for that, or can I add that to this one?

@szepeviktor
Copy link
Collaborator

It would be nice to fix unit tests here.

@Grldk
Copy link
Author

Grldk commented Jul 6, 2023

Hmm, all tests passed for me locally, so to be honest I didn't really pay any attention to the test results here..

I'll try to fix the unit tests soon, hopefully tomorrow or else early next week.

@Grldk
Copy link
Author

Grldk commented Jul 7, 2023

Well, I guess those tests will not pass: phpstan/phpstan#9590

Not sure what to do with this now. We'll have to wait for call-site variance in phpstan I guess..

@mad-briller
Copy link
Contributor

phpstan/phpstan-src#2485 (review) i believe it's already in the works thanks to this pr

@canvural
Copy link
Collaborator

Hi,

This is just wrong 😄 And the decision that Laravel defined the templates as covariant is also wrong. Now I'm happy that Larastan overrides that info!

With template-covariant basically there'd be no guarantee that Collection<int, User> would only contain Users, in the type system level.

This is how Doctrine collections also have their annotations. Also any mutable collection implementations.

So, as long as the Laravel collections are mutable this will not happen. Hope it's clear.

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.

None yet

4 participants