Fix Drupal.Commenting.FunctionComment.MissingReturnType

Added hint type to first 23 class

While I think this is a good idea.

It is on the path to becoming unreviewable.

Could we make this a meta issue and then spin off issue for say the component namespace, the Drupal\Core\Entity namespace etc?

+++ b/core/lib/Drupal/Component/Gettext/PoStreamReader.php
@@ -571,10 +571,10 @@ class PoStreamReader implements PoStreamInterface, PoReaderInterface {
-   * @param $comment
+   * @param string $comment
    *   An array of strings containing a comment.

This should be type array as its an array of strings.

Add interdiff for the above patch #4

There is a phpcs rule we can enable for this. We should also not do anything else other than @return fixes in this issue.

The phpcs rule is Drupal.Commenting.FunctionComment.MissingReturnType. We should do that intead, see also #2572645: [Meta] Fix 'Drupal.Commenting.FunctionComment' coding standard for more information.

Moving this to a child of #2571965: [meta] Fix PHP coding standards in core, stage 1. I have updated the IS and the patch. Not running tests yet.

The patch in #18 includes changes from;

• #2935189: Complete docblock comments in Gettext component. which includes modification to the return comment because I did not want to lose those changes. Credit to Sutharsan
• #2606242: ProjectInfo::filterProjectInfo, add @return values and @param modification in the comment docblocks. credit to heykarthikwithu

Added some more changes.
There are still coding standard fixes to make here.

i'll fix some of the coding standard problems

So, I did the reroll of the patch because I was having some problems at applying it. I actually found only a few phpcs problems about return missing type. I left some files behind, I mean, I didn't add return because this files were changed in this issue that I worked:

3246665 - Incorrect docblock return type in Upsert::execute() and Update::execute()

So, I don't know it is a good thing fixing this files here. (The 3246665 issue is not closed as fixed but, I don't know, if it is a case of fixing here too...)

I'm sending the new patch and a interdiff too.

The testbot is returning with:

FILE: ...ww/html/core/tests/Drupal/KernelTests/Core/File/FileTestBase.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 186 | ERROR | [x] Whitespace found at end of line
     |       |     (Squiz.WhiteSpace.SuperfluousWhitespace.EndLine)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...e/modules/views/src/Plugin/views/argument/ArgumentPluginBase.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 858 | ERROR | If there is no return value for a function, there must
     |       | not be a @return tag.
     |       | (Drupal.Commenting.FunctionComment.VoidReturn)
----------------------------------------------------------------------


FILE: /var/www/html/core/modules/system/system.module
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 452 | ERROR | Return type missing for @return tag in function
     |       | comment
     |       | (Drupal.Commenting.FunctionComment.MissingReturnType)
----------------------------------------------------------------------


FILE: /var/www/html/core/lib/Drupal/Core/Database/Query/Truncate.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 53 | ERROR | Return type missing for @return tag in function
    |       | comment
    |       | (Drupal.Commenting.FunctionComment.MissingReturnType)
----------------------------------------------------------------------


FILE: /var/www/html/core/lib/Drupal/Core/Database/Query/Merge.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 357 | ERROR | Return type missing for @return tag in function
     |       | comment
     |       | (Drupal.Commenting.FunctionComment.MissingReturnType)
----------------------------------------------------------------------


FILE: ...ar/www/html/core/lib/Drupal/Component/Gettext/PoStreamReader.php
----------------------------------------------------------------------
FOUND 8 ERRORS AFFECTING 8 LINES
----------------------------------------------------------------------
 289 | ERROR | Function return type is not void, but function is
     |       | returning void here
     |       | (Drupal.Commenting.FunctionComment.InvalidReturnNotVoid)
 296 | ERROR | Function return type is not void, but function is
     |       | returning void here
     |       | (Drupal.Commenting.FunctionComment.InvalidReturnNotVoid)
 327 | ERROR | Function return type is not void, but function is
     |       | returning void here
     |       | (Drupal.Commenting.FunctionComment.InvalidReturnNotVoid)
 358 | ERROR | Function return type is not void, but function is
     |       | returning void here
     |       | (Drupal.Commenting.FunctionComment.InvalidReturnNotVoid)
 388 | ERROR | Function return type is not void, but function is
     |       | returning void here
     |       | (Drupal.Commenting.FunctionComment.InvalidReturnNotVoid)
 430 | ERROR | Function return type is not void, but function is
     |       | returning void here
     |       | (Drupal.Commenting.FunctionComment.InvalidReturnNotVoid)
 455 | ERROR | Function return type is not void, but function is
     |       | returning void here
     |       | (Drupal.Commenting.FunctionComment.InvalidReturnNotVoid)
 496 | ERROR | Function return type is not void, but function is
     |       | returning void here
     |       | (Drupal.Commenting.FunctionComment.InvalidReturnNotVoid)
----------------------------------------------------------------------

Closed #2109603: Fix throws in Drupal\Core\Database which included a fix that is duplicated here, adding credit.

@Sweetchuck, thanks for looking into the failing test. There is a Meta issue which lists the tests that have random failures, #2829040: [meta] Known intermittent, random, and environment-specific test failures. Since this issue isn't changing code, I think it is perfectly OK to comment that the failure is in a test listed on that page and setting the issue back to NR. Running the tests is a real cost to the Drupal Association and it is OK to avoid that cost when we can.

I reviewed the last patch #27 and found out it needed a reroll.
I also added return statements in 2 files that were missing.
Thank you

Hello, i reviewed this issue, checking all files that received change, and everything's look like that make sense, so i'll move this issue to RTBC

The change to the file core/phpcs.xml.dist is missing.

Updated patch for 10.0.x and updated phpcs.xml per #39.

Testbot is still not happy.

--- Commands Executed ---
core/scripts/dev/commit-code-check.sh --drupalci
Return Code: 1
--- Output ---
CSpell: passed

----------------------------------------------------------------------------------------------------

Running PHPStan on *all* files.
 ------ ----------------------------------------------------------------------- 
  Line   core/lib/Drupal/Core/Database/Query/Update.php                         
 ------ ----------------------------------------------------------------------- 
  151    Method Drupal\Core\Database\Query\Update::execute() should return int  
         but return statement is missing.                                       
 ------ ----------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------- 
  Line   core/lib/Drupal/Core/Entity/ContentEntityNullStorage.php           
 ------ ------------------------------------------------------------------- 
  76     Method Drupal\Core\Entity\ContentEntityNullStorage::save() should  
         return int but return statement is missing.                        
 ------ ------------------------------------------------------------------- 

 ------ --------------------------------------------------------------- 
  Line   core/modules/field/tests/modules/field_test/field_test.module  
 ------ --------------------------------------------------------------- 
  73     Function field_test_memorize() should return array but return  
         statement is missing.                                          
 ------ --------------------------------------------------------------- 

 ------ -------------------------------------------------------------------- 
  Line   core/modules/pgsql/src/Driver/Database/pgsql/Update.php             
 ------ -------------------------------------------------------------------- 
  82     Method Drupal\pgsql\Driver\Database\pgsql\Update::execute() should  
         return int but return statement is missing.                         
 ------ -------------------------------------------------------------------- 

 ------ ---------------------------------------------------------------------- 
  Line   core/modules/views/src/Plugin/views/filter/FilterPluginBase.php       
 ------ ---------------------------------------------------------------------- 
  1654   Method Drupal\views\Plugin\views\filter\FilterPluginBase::validate()  
         should return array but return statement is missing.                  
 ------ ---------------------------------------------------------------------- 

 [ERROR] Found 5 errors                                                         

Fixed Drupal CS errors of patch #40.

Still testbot errors: https://www.drupal.org/pift-ci-job/2386398

I rerolled #42 as it wasn't applying and tried to address the erros.

Ok, for some reason patch didn't apply because of a change on Merge.php (despite that it applied good on local). Trying again.

Ok so, trying to address the last errors.

Last patch i'll send for this issue. I'm unable to reproduce the errors locally and working against DO logs mess a lot with the issue feed.

Sorry for cluttering the feed with patches. Hope it's all solved by now.

Needs review :)

Hi,

You should also attach an interdiff if you are working with patch files. Check out this documentation page.

Also if want to check the codestyle of your core patch, check out this page on the dev tools included and hwo to run them.

@ravi.shankar, thanks for the reroll. Since this issue for coding standards only there should be no changes to the code. The phpstan errors need to be fixed in the documentation.

I rerolled this from the patch in #40 where the phpstan first appeared.

The patch looks good!

1. +++ b/core/includes/theme.inc
   @@ -245,7 +245,7 @@ function drupal_find_theme_templates($cache, $extension, $path) {
   + * @return mixed|null
   

   I think we can remove "|null" as null is part of mixed.

2. +++ b/core/lib/Drupal/Core/Database/Query/AlterableInterface.php
   @@ -87,7 +87,7 @@ public function addMetaData($key, $object);
   +   * @return mixed|null
   

   See previous.

3. +++ b/core/lib/Drupal/Core/Database/StatementInterface.php
   @@ -108,7 +108,7 @@ public function fetch($mode = NULL, $cursor_orientation = NULL, $cursor_offset =
   +   * @return bool|mixed
   

   See previous.

4. +++ b/core/lib/Drupal/Core/Datetime/Element/Datelist.php
   @@ -359,7 +359,7 @@ protected static function checkEmptyInputs($input, $parts) {
   +   * @return \Drupal\Core\Datetime\DrupalDateTime|mixed
   

   I am not happy with this one, only I do not know how to make it better.

5. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -864,7 +864,7 @@ function hook_update_last_removed() {
   + * @return return
   

   Should this not be: "@return array".

6. +++ b/core/lib/Drupal/Core/File/file.api.php
   @@ -23,7 +23,7 @@
   + * @return string[]
     *   If the user does not have permission to access the file, return -1. If the
   

   The return value can also be "-1".

Hello, i will be doing those changes specified on #52

I changed those returns

I reviewed #54 and looks like it covers everything pointed in #52.

So far so good, so I'm moving to RTBC.

Yes, this almost done!

@sophiavs, Welcome to Drupal! Thanks for making the changes to the patch. To help reviewers always add an interdiff, or a diff, whichever is appropriate. There are instructions for creating an interdiff. Thanks.

This needs a patch for 9.5.x.

Added reroll of patch #54 on Drupal 9.5.x.

@ravi.shankar, thanks but remember to run the commit code checks locally .

Add back the changes for 9.5.x for deprecated code.

To help with review, I am adding the missing interdiff.

This isn't being found by tag. Re-entering tag

It all look good to me.
All my points have been addressed.
The suppression of the rule has been removed.
The testbot is green.
For me it is RTBC.

Thanks everybody for working on this.

@daffie, thanks!

Now, make a patch for 10. This applies to 10.0 and 10.1

I checked all three patches by running rg @return$ to find empty return tags, then applying the patches and rerunning the command. After applying there are no empty return tags left.

I also read through the patches and all the changes look good and make sense. There is one exception:

+++ b/core/tests/Drupal/KernelTests/Core/File/FileTestBase.php
@@ -151,11 +151,11 @@ public function assertDirectoryPermissions($directory, $expected_mode, $message
-   * @param $path
+   * @param string $path

Technically @param is out of scope here, but this is still the correct fix so we can let this one slide.

Therefore this is RTBC.

+++ b/core/lib/Drupal/Component/Gettext/PoStreamReader.php
@@ -286,14 +286,14 @@ private function readLine() {
 
           $this->context = 'COMMENT';
-          return;
+          return NULL;
         }

Is this actually necessary for the documentation change? Would think the implicit NULL from return; would still be fine?

Making the changes for #65 for Drupal 10. The patch applied locally to 10.0 and 10.1.

And repeat for the 9.5.x patch.

#66/#67 look good, back to RTBC.

Adding followup tag for #68.

Committed and pushed b8ac0d9850 to 10.1.x and ecd67af7a4 to 10.0.x. Thanks!
Committed 452bf18 and pushed to 9.5.x. Thanks!

It'd be great if someone could create the follow-up for #68 but I don't think it not existing should prevent this going in.

Follow up was made, #3312049: [Followup] Fix Drupal.Commenting.FunctionComment.MissingReturnType returns for NULL. I am removing the tag.