Docblock comments on DataList shadow SS_List's more useful PHPDoc types in PHPStorm (::first, ::byID, ::find, ::filter, etc.)

[MasonD]

Module version(s) affected

5.2.1

Description

I'm not sure on the exact behaviour of phpstan, but at least PHPStorm's inbuilt analyser does not correctly type the following snippet:

$member = Member::get()->filter(['Email' => '[email protected]])->first();

This is because PHPStorm is ignoring SS_List's typings when a docblock is present on a method in DataList. PHPStorm correctly identifies the type of $member if the docblock comments of filter and first are removed, or if @inheretDoc is added to both methods' docblocks.

How to reproduce

$member = Member::get()->filter(['Email' => '[email protected]])->first();

in a fresh install of silverstripe & PHPStorm. PHPStorm doesn't recognise the type of $member and won't provide completion or recognise method calls for refactoring etc.

Possible Solution

Either add @inheretDoc to all the offending methods in DataList and its subclasses, or copy the phpdoc types from SS_List into the docblock comments directly.

Additional Context

No response

Validations

• Check that there isn't already an issue that reports the same bug
• Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)

PRs

• improve DataList and ArrayList docblock comments for PHPStorm #11263

[emteknetnz]

Closing because this is an issue with PHPStorm, not with Silverstripe

The type hinting works correctly in VSCode, at least with the PHP Intelephense plugin - with the example above I was correctly type hinted for a Member. Type hinting was greatly improved in 5.2 with generic type hints

[MasonD]

@emteknetnz I don't understand this stance? I agree that 5.2 is a huge improvement, and was running my own stubs with generic type hints before 5.2. But the reason for adding the generic type hints is to improve dev experience, and the current types don't work with PHPStorm's analysis but would work with just the inclusion of @inheritDoc.

Is there something I'm missing about @inheritDoc that would make its inclusion in the docblocks of DataList methods in order to support PHPStorm unfeasible? If it degrades the experience in other editors I'd concede that that's not a trade to make, but if all it does is improve PHPStorm support, I think that's worth it. I'd be willing to create the PR.

[kinglozzer]

Seems like a pretty trivial fix, and given the prevalence of PHPStorm I think it’s worthwhile. Some of the comments in DataList do contain a bit more (DataObject-specific) info than the methods they’re overriding, so rather than using @inheritDoc it might be better to copy the return types down in some places. Do you want to have a crack at a PR @MasonD?

[MasonD]

Sure. Which branch would you like me to target it on?

[kinglozzer]

I think 5 rather than 5.2, because I suppose this is more an enhancement for PHPStorm users than an bug fix

[emteknetnz]

Targetting 5.2 is fine for this particular enhancement, it's not changing the functional behaviour of any classes.

Means you'll get your patch release immediately instead of having to wait several months.

[emteknetnz]

Linked PR has been merged, it will be automatically tagged shortly