proper return types

[lekoala]

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

https://github.com/silverstripe/silverstripe-framework/blob/bcbbfddd1ad6f70a2fd954b4bf71a236fd754527/src/Forms/FileUploadReceiver.php#L81-L90

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:

/**
 * @template T
 * @param class-string<T> $class
 * @return T|mixed
 */
function ss_inject(string $class)
{
    return SilverStripe\Core\Injector\Injector::inst()->get($class);
}

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

• [X] You intend to implement the feature yourself
• [X] You have read the contributing guide
• [X] You strongly believe this feature should be in core, rather than being its own community module
• [X] You have checked for existing issues or pull requests related to this feature (and didn't find any)

[maxime-rainville]

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.

Should these type of issues always be distinct PR or "one big sweep" across the framework would be better ?

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 supports

We're adding support for generic typehints in CMS 5.2. You might want to have a look at that.

[lekoala]

@maxime-rainville great news for 5.2, can't wait to see that :-) it's going to be very helpful ok I may then make some small pr for the parts i'm mostly using

[GuySartorelli]

Explicitly defining types could be a breaking API change depending on the context. So I would avoid jumping on those.

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.

[lekoala]

great so i'll be closing this and make pr if needed