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

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

quietone created an issue. See original summary.

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

atul4drupal’s picture

Status: Active » Needs review

The 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.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Change is simple enough. Also think this would of been a good one to tag for new users.

xjm’s picture

Title: Fix return type in \Drupal\Tests\rest\Functional\ResourceTestBase::recursiveKSort » Fix return type in \Drupal\Tests\rest\Functional\ResourceTestBase::recursiveKSort()
xjm’s picture

Issue tags: +Needs followup

Hmm, #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:

grep -rl "recursiveKSort" *

...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.

xjm’s picture

Saving credits.

quietone’s picture

longwave’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 321492b8a8 to 11.x and 9164d2f354 to 10.2.x and 946c240fe7 to 10.1.x. Thanks!

  • longwave committed 946c240f on 10.1.x
    Issue #3388204 by quietone, atul4drupal, xjm: Fix return type in \Drupal...

  • longwave committed 9164d2f3 on 10.2.x
    Issue #3388204 by quietone, atul4drupal, xjm: Fix return type in \Drupal...

  • longwave committed 321492b8 on 11.x
    Issue #3388204 by quietone, atul4drupal, xjm: Fix return type in \Drupal...

Status: Fixed » Closed (fixed)

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