Problem/Motivation
A return statement was added to \Drupal\Tests\rest\Functional\ResourceTestBase::recursiveKSort() in #3283794: Fix 'should return {type} but return statement is missing' PHPStan L0 errors in test code but it does not return anything.
Steps to reproduce
Proposed resolution
Revert the change and update the doc bloc
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3388204
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:
- 11.x
compare
- 3388204-fix-return-type
changes, plain diff MR !4824
Comments
Comment #4
atul4drupal commentedThe changes look ok to go ahead with.
The MR pipeline was giving error most likely because it needed to be rebased, rebased and now it all looks ok (didnt realize that rebasing will add a comment in this thread too as a new MR)
RTBC +1 from my side, cant do it myself as I got 1 MR added.
Comment #5
smustgrave commentedChange is simple enough. Also think this would of been a good one to tag for new users.
Comment #6
xjmComment #7
xjmHmm, #3283794: Fix 'should return {type} but return statement is missing' PHPStan L0 errors in test code is interesting and seems a lot could have been the wrong way around. I think it's more common to have incorrect return docs than incorrect return values.
The diffstat for the previous issue (excluding the baseline regeneration) was
+192, -85-- 277 total lines to review, so already above our recommended 100-200 and in the range that should be reserved for change sets that are scannable without context. That issue definitely is not scannable without context because you need to look up whole methods, callers, parent implementations, etc. to determine whether adding a return type or removing the docs is the correct approach, and what the type should be.For example, for this issue, I had to do:
...then open all those files and check how the method was used to see if they relied on the return value or the pass-by-reference. In all cases they relied on the pass-by-reference and did not expect a return value.
So this issue is fine, and committable by itself. However, can we first get a followup to check over #3283794: Fix 'should return {type} but return statement is missing' PHPStan L0 errors in test code and ensure we didn't introduce any other incorrect return values?
And in the future, such issues should be split into smaller logical scopes.
Comment #8
xjmSaving credits.
Comment #9
quietone commentedFollow up made, #3392534: Check returns values added in #3283794
Comment #10
longwaveCommitted and pushed 321492b8a8 to 11.x and 9164d2f354 to 10.2.x and 946c240fe7 to 10.1.x. Thanks!