Problem/Motivation

rest.module's ResourceTestBase is the source of a large number of PHPStan baseline entries.

We cannot add void return types easily as the tests are designed to be implemented in contrib. However, we can add @return docblocks, which satisfies PHPStan.

Symfony's DebugClassLoader can perhaps also be convinced to notify downstream users that they should actually add the return types in future, too.

Steps to reproduce

Proposed resolution

Add @return void where necessary in ResourceTestBase.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3575324

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

longwave created an issue. See original summary.

longwave’s picture

Status: Active » Needs review

We probably want to file an upstream issue with Coder so @return void does not need a pointless description.

borisson_’s picture

We probably want to file an upstream issue with Coder so @return void does not need a pointless description.

Actually, as of #3376518: Allow omitting @var, @param and @return tags or their descriptions if the variable name and type is enough, we can omit the entire @return if it's void.

longwave’s picture

But we need it to tell PHPStan that the return type should be void, in the cases where we can't add a real return type yet.

mondrake’s picture

Added question inline

mondrake’s picture

Symfony's DebugClassLoader can perhaps also be convinced to notify downstream users that they should actually add the return types in future, too.

tried that but did not get through, see IS in #3486376: Extend Symfony DebugClassLoader to report missing cross-module @return types - that issue though would implement a PHPStan alternative with a custom rule.

borisson_’s picture

#5, sorry - I didn't fully understand this issue and was overeager in pointing to that new rule :)
I agree we need to create a coder issue, I'd prefer to commit this as is, with the followup created. I think that makes more sense over adding the @phpstan-return equivalent.

I will mark this as rtbc when we can link to an issue to create the new rule, should we also add something to the coding standards queue as well?

longwave’s picture

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

With the follow up open, I think this is good to go now. Less baseline FTW

sivaji_ganesh_jojodae’s picture

Status: Reviewed & tested by the community » Needs work

Merge conflicts must be resolved.

longwave’s picture

Status: Needs work » Reviewed & tested by the community

Rebased.

sivaji_ganesh_jojodae’s picture

Status: Reviewed & tested by the community » Needs work

Merge blocked: Merge conflicts must be resolved.

mstrelan made their first commit to this issue’s fork.

mstrelan’s picture

Title: Add return type documentation to ResourceTestBase » [PP-1] Add return type documentation to ResourceTestBase
Status: Needs work » Needs review

Rebased and regenerated the baseline. I was going to say we should postpone on #3577137: Upgrade to Coder 9 to remove those @return descriptions, but there are already a bunch in core we should clean up. I opened #3579889: [PP-1] Remove @return void descriptions so we can do them all in one hit.

mstrelan’s picture

Title: [PP-1] Add return type documentation to ResourceTestBase » Add return type documentation to ResourceTestBase

Didn't mean to change the title after I changed my mind half way through commenting.

sivaji_ganesh_jojodae’s picture

Status: Needs review » Needs work

There seems to be a merge conflict in phpstan-baseline.php. It might be best to regenerate the baseline to ensure it reflects the current state of the codebase.

longwave’s picture

Status: Needs work » Needs review

Regenerated the baseline.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Avoid commit conflicts

Tried tagging "Avoid commit conflicts" because this one is easy to get offsync with the baseline

  • amateescu committed b7e4f745 on main
    task: #3575324 Add return type documentation to ResourceTestBase
    
    By:...
amateescu’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Merged this into main. Not sure whether it should go into 11.x as well, but it would need a new MR because the current one doesn't apply anyway.

Committed b7e4f74 and pushed to main. Thanks!

longwave’s picture

Status: Patch (to be ported) » Fixed

Yeah I don't think this is worth backporting, we only really care about the baseline in main - they've diverged enough now I believe that backports are already tricky sometimes.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.