Closed (fixed)
Project:
Drupal core
Version:
11.x-dev
Component:
other
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
19 Sep 2023 at 09:12 UTC
Updated:
30 May 2026 at 12:14 UTC
Jump to comment: Most recent
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!