The error is thrown for line 1445 in core/modules/views/src/ViewExecutable.php
Line 1445 is a void return statement
return;
and the return comment is
@return array|null
There is also a return with a value towards the end of the function.
The error should be raised after checking if null is given as a possible return value. Also it should check all returns in the function and not just the first return which is what it is doing currently.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | interdiff-2722695-7-12.txt | 8.87 KB | anoopjohn |
| #12 | coder-InvalidReturnNotVoid-with-null-return-2722695-12.patch | 8.96 KB | anoopjohn |
Comments
Comment #2
anoopjohn commentedPlease find attached a patch for the same.
Comment #3
pfrenssenThanks a lot for the contribution! Before offering this for review, can you perhaps add a test? That will make it easier to assess whether the bug is conclusively fixed, and will prevent regressions from occurring in the future.
Writing a test for Coder is really easy. Please check the following files:
Comment #4
anoopjohn commentedThanks for the code review @pfrenssen and for the inputs.
Should the tests be added here or in good.php?
Can you please let me know as to when do you add tests in good.php and bad.php and bad.php.fixed versus in here?
Comment #5
pfrenssenGood question! good.php, bad.php and bad.php.fixed are old tests, they are now rarely used but we keep them around since they are still useful for detecting regressions.
We now group all tests together in a file that references the sniff that takes care of it. Your fix is in FunctionCommentSniff, so the
FunctionCommentUnitTest.*files are the correct ones for it.Comment #6
anoopjohn commentedThanks for the explanation. I have updated the patch with the tests. Adding the tests helped catch a bug as well. I have added two tests
a) Null return valid when null is an option
b) Even when null is an option you can't have all null returns.
Comment #7
borisson_Changed a little bit of the documentation added in the test.
Comment #8
klausiHm, why is that change so complicated? Can't we just check if $type contains "|null" or "null|" right at the condition before the error?
And we should move the tests to good.php, since we are collecting everything there that should not throw errors by any sniff.
Comment #9
anoopjohn commentedThe earlier logic was testing only for the first return statement that did not return any value and triggering an error when null is not an optional value. When null is an optional value then the changed logic tests for at least one return that is not a void return.
Regarding the tests, I am confused now. Should the tests be moved to good.php or should they be added there as well along with the three files mentioned in comment #3?
Comment #10
klausiFixes for regressions should always be added to good.php, since no new errors or warnings are introduced that should be tested.
Not sure we need that extra testing of what the function actually returns, so I would leave that out for now.
Comment #11
anoopjohn commentedWhen null is only one of the optional values shouldn't the function have at least one non-void return? If not, wouldn't the @return documentation be invalid in the first place? If there is no non-void return then the return should become @return null in which case it should not have any @return comment in the first place.
We still have to loop through and find all returns any way right if we are to fix the reported issue? So can't we also check for the above condition to make the tests on the return value complete?
Comment #12
anoopjohn commentedPlease find attached a patch with the test added in good.php. Fixed coder coding standard issues in the patch as well.
Added related issue as this fix is required for #2722699: Fix Drupal.Commenting.FunctionComment.InvalidReturnNotVoid
Comment #14
klausiCommitted, thanks!