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

[11.x] Fix incorrect PHPDoc for KeyBy and GroupBy #52918

Merged

Conversation

kayw-geek
Copy link
Contributor

Description:
This PR fixes the PHPDoc annotations for the keyBy and groupBy methods. The current changes force users to ignore argument.templateType errors, which would otherwise result in the following error:

Unable to resolve the template type TNewKey in call to method Illuminate\Support\Collection<int, App\Models\Users>::keyBy()

This issue may be cause by #52787

cc @calebdw

@kayw-geek kayw-geek force-pushed the bugfix/fix-incorrect-phpdoc-for-keyby branch from 43db249 to b8b6bf7 Compare September 25, 2024 07:30
@calebdw
Copy link
Contributor

calebdw commented Sep 25, 2024

@kayw-geek,

This error is more of a warning and is safe to ignore if you choose to (locally or globally). If the method template type is not resolved it falls back to array-key anyway. I was trying not to complicate the PHPDocs too much with all the ternaries for the other cases.

The current changes force users to ignore argument.templateType errors

It doesn't force you to ignore, it's just letting you know that you can get more accurate types if you use closures. Of course you can ignore if you choose to do so.

This is the best we can do with PHPDocs, but I've been thinking about creating an extension in Larastan that would be able to determine the type even if an array/string is passed.

@kayw-geek
Copy link
Contributor Author

But the problem now is that if strings or arrays are passed, I must ignore this error. I think a relatively complex phpdoc is a better choice to prevent users from ignoring the phpstan error.

@calebdw
Copy link
Contributor

calebdw commented Sep 25, 2024

Yeah I could see how that would be annoying if you don't care about the better types or need an array.

You might be able to shorten it to one ternary:

* @return static<($groupBy is string ? array-key : ($groupBy is array ? array-key : TGroupKey)), static<($preserveKeys is true ? TKey : int), TValue>>
* @return static<($groupBy is (callable(TValue, TKey): TGroupKey) ? TGroupKey : array-key), static<($preserveKeys is true ? TKey : int), TValue>>

@kayw-geek
Copy link
Contributor Author

@calebdw I am worried that if the key of an array happens to be a function name, there will be problems.
https://phpstan.org/r/c2dedf00-662a-4947-ade5-009105cd1434

@calebdw
Copy link
Contributor

calebdw commented Sep 25, 2024

These types should probably be Closure instead of callable:

https://phpstan.org/r/af6ec9c9-22e1-472f-8cc6-1f937b6a0ab8

@kayw-geek
Copy link
Contributor Author

Well, if this method itself originally expected a closure, that would be the best.

@taylorotwell taylorotwell merged commit 6a3a8b2 into laravel:11.x Sep 25, 2024
31 checks passed
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.

3 participants