-
Notifications
You must be signed in to change notification settings - Fork 822
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
proper return types #11149
Comments
We pondered strongly-typing everything in CMS 5, but we ended up putting it in the too hard basket. We'll have more lead up time to CMS 6, so we might be able to do it there.
If you did occasional PRs to adjust the PHPDoc typing for specific method or class, we should be able to merge that. Doing the entirety of Framework, might be a bit too much to swallow all at once. Explicitly defining types could be a breaking API change depending on the context. So I would avoid jumping on those. Generics supportsWe're adding support for generic typehints in CMS 5.2. You might want to have a look at that. |
@maxime-rainville great news for 5.2, can't wait to see that :-) it's going to be very helpful |
The next major branch has been created, so any PRs targetting that branch with strong-typing could be reviewed and merged if we agree with the typing. |
great so i'll be closing this and make pr if needed |
Description
I've been using more and more phpstan recently but I've been a bit disappointed by inconsistent return types that exist across the framework.
For example
silverstripe-framework/src/Forms/FileUploadReceiver.php
Lines 81 to 90 in bcbbfdd
Obviously should be return ?DataObject or DataObject|null depending on preferences
Should these type of issues always be distinct PR or "one big sweep" across the framework would be better ?
Additional context or points of discussion
Another typical pain point are Extensions that are not easily detected
A typical situation: you have a MyMember extension, defining a return type Member&MyMember doesn't work so well in most ide, Member|MyMember is an obvious - incorrect - hack that works sometimes
Dynamic extensions, in general, don't help with proper static analysis :-) (a phpstan plugin could help there, see https://github.com/syntro-opensource/silverstripe-phpstan)
Injector is not always friendly. I've been using a simple enough work around that looks basically like this:
This also apply to DataObject in general as soon as you do something else that MyDataObject::get_by_id($id)... I do miss something like MyDataObject::get_by_field($field, $id) or get_by_index (which is a really typical use case) and have $list->first() return something valid
Validations
The text was updated successfully, but these errors were encountered: