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
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:
- 3575324-add-return-type-resourcetestbase
changes, plain diff MR !14879
Comments
Comment #3
longwaveWe probably want to file an upstream issue with Coder so
@return voiddoes not need a pointless description.Comment #4
borisson_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.
Comment #5
longwaveBut 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.
Comment #6
mondrakeAdded question inline
Comment #7
mondraketried 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.
Comment #8
borisson_#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?
Comment #9
longwaveOpened #3575389: Allow @return void to skip the description
Comment #10
mondrakeWith the follow up open, I think this is good to go now. Less baseline FTW
Comment #11
sivaji_ganesh_jojodae commentedMerge conflicts must be resolved.
Comment #12
longwaveRebased.
Comment #13
sivaji_ganesh_jojodae commentedMerge blocked: Merge conflicts must be resolved.
Comment #15
mstrelan commentedRebased 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.
Comment #16
mstrelan commentedDidn't mean to change the title after I changed my mind half way through commenting.
Comment #17
sivaji_ganesh_jojodae commentedThere 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.
Comment #18
longwaveRegenerated the baseline.
Comment #19
mondrakeTried tagging "Avoid commit conflicts" because this one is easy to get offsync with the baseline
Comment #21
amateescu commentedMerged 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!
Comment #23
longwaveYeah 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.