Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: add/fix @return types in CLI #6388

Merged
merged 1 commit into from
Aug 18, 2022
Merged

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Aug 18, 2022

Description
Ref #6310

  • add/fix @return

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • [] Unit testing, with >80% coverage
  • [] User guide updated
  • Conforms to style guide

@kenjis kenjis added the 4.3 label Aug 18, 2022
Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While these are correct in the context of their usage, my concern is when we bump to PHP 8.0+ and utilized union types in return declarations, we cannot use void in union with other types. It will be a parse error. Void can only be used as a standalone type

I suggest using null as replacement for void if we need to capture the return.
See https://3v4l.org/YnYc4 and https://3v4l.org/NQ82o

@kenjis
Copy link
Member Author

kenjis commented Aug 18, 2022

I suggest using null as replacement for void if we need to capture the return.

What do you mean? @return int|null?

But if we change the method to public function run(...): int|null,
wouldn't it break all existing commands that return void?

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏

@paulbalandan
Copy link
Member

I suggest using null as replacement for void if we need to capture the return.

What do you mean? @return int|null?

Yes, for the PHPDocs only.

But if we change the method to public function run(...): int|null, wouldn't it break all existing commands that return void?

Yes, it would break if we declare the type as int|null. But it wouldn't happen sooner so the return might change in the future.

@MGatner
Copy link
Member

MGatner commented Aug 18, 2022

Sorry, I missed the comments before reviewing. The void union thing is a mess. I will do some more reading, but last time I looked into it there was not a solution. Using null instead is not suitable without changing the code: https://3v4l.org/S7pug#v801

phpDoc also doesn't support the swap:

The value contained, or returned, is literally null. This type is not to be confused with void, which is the total absence of a variable or value (usually used with the @return tag). https://docs.phpdoc.org/2.9/guides/types.html

So string|void is correct, but PHP does not support native union void types. More when I have it...

@MGatner
Copy link
Member

MGatner commented Aug 18, 2022

Relevant discussion on semi-related 8.2 RFC: "Distinction Between Null and Void" https://wiki.php.net/rfc/null-false-standalone-types

@MGatner
Copy link
Member

MGatner commented Aug 18, 2022

Nope, same conclusion as before: no good answer. I think a possible solution is when the return is mixed to update return; to return null; but I'm not sure what sort of consequences this will have broadly.

@kenjis
Copy link
Member Author

kenjis commented Aug 18, 2022

@paulbalandan If we change void to null in PHPDocs,
PhpStorm complains with yellow marker.
Screenshot 2022-08-18 20 27 28

Also PHPStan reports errors.

$ composer analyze
> phpstan analyse
Note: Using configuration file /Users/kenji/work/codeigniter/CodeIgniter4/phpstan.neon.dist.
 441/441 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ ------------------------------------------------------------------------------------------------- 
  Line   system/CLI/Commands.php                                                                          
 ------ ------------------------------------------------------------------------------------------------- 
  57     Method CodeIgniter\CLI\Commands::run() should return int|null but empty return statement found.  
 ------ ------------------------------------------------------------------------------------------------- 

 ------ -------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Cache/ClearCache.php                                                                          
 ------ -------------------------------------------------------------------------------------------------------------- 
  71     Method CodeIgniter\Commands\Cache\ClearCache::run() should return int|null but empty return statement found.  
  81     Method CodeIgniter\Commands\Cache\ClearCache::run() should return int|null but empty return statement found.  
  85     Method CodeIgniter\Commands\Cache\ClearCache::run() should return int|null but return statement is missing.   
 ------ -------------------------------------------------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Cache/InfoCache.php                                                                          
 ------ ------------------------------------------------------------------------------------------------------------- 
  63     Method CodeIgniter\Commands\Cache\InfoCache::run() should return int|null but empty return statement found.  
  86     Method CodeIgniter\Commands\Cache\InfoCache::run() should return int|null but return statement is missing.   
 ------ ------------------------------------------------------------------------------------------------------------- 

 ------ --------------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Database/CreateDatabase.php                                                                          
 ------ --------------------------------------------------------------------------------------------------------------------- 
  84     Method CodeIgniter\Commands\Database\CreateDatabase::run() should return int|null but return statement is missing.   
  119    Method CodeIgniter\Commands\Database\CreateDatabase::run() should return int|null but empty return statement found.  
  134    Method CodeIgniter\Commands\Database\CreateDatabase::run() should return int|null but empty return statement found.  
  142    Method CodeIgniter\Commands\Database\CreateDatabase::run() should return int|null but empty return statement found.  
 ------ --------------------------------------------------------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Database/Migrate.php                                                                         
 ------ ------------------------------------------------------------------------------------------------------------- 
  77     Method CodeIgniter\Commands\Database\Migrate::run() should return int|null but return statement is missing.  
 ------ ------------------------------------------------------------------------------------------------------------- 

 ------ --------------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Database/MigrateRefresh.php                                                                          
 ------ --------------------------------------------------------------------------------------------------------------------- 
  77     Method CodeIgniter\Commands\Database\MigrateRefresh::run() should return int|null but empty return statement found.  
  85     Method CodeIgniter\Commands\Database\MigrateRefresh::run() should return int|null but return statement is missing.   
 ------ --------------------------------------------------------------------------------------------------------------------- 

 ------ ---------------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Database/MigrateRollback.php                                                                          
 ------ ---------------------------------------------------------------------------------------------------------------------- 
  76     Method CodeIgniter\Commands\Database\MigrateRollback::run() should return int|null but empty return statement found.  
  88     Method CodeIgniter\Commands\Database\MigrateRollback::run() should return int|null but return statement is missing.   
 ------ ---------------------------------------------------------------------------------------------------------------------- 

 ------ -------------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Database/MigrateStatus.php                                                                          
 ------ -------------------------------------------------------------------------------------------------------------------- 
  150    Method CodeIgniter\Commands\Database\MigrateStatus::run() should return int|null but empty return statement found.  
  163    Method CodeIgniter\Commands\Database\MigrateStatus::run() should return int|null but return statement is missing.   
 ------ -------------------------------------------------------------------------------------------------------------------- 

 ------ ---------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Database/Seed.php                                                                         
 ------ ---------------------------------------------------------------------------------------------------------- 
  76     Method CodeIgniter\Commands\Database\Seed::run() should return int|null but return statement is missing.  
 ------ ---------------------------------------------------------------------------------------------------------- 

 ------ -------------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Database/ShowTableInfo.php                                                                          
 ------ -------------------------------------------------------------------------------------------------------------------- 
  113    Method CodeIgniter\Commands\Database\ShowTableInfo::run() should return int|null but empty return statement found.  
  119    Method CodeIgniter\Commands\Database\ShowTableInfo::run() should return int|null but empty return statement found.  
  140    Method CodeIgniter\Commands\Database\ShowTableInfo::run() should return int|null but empty return statement found.  
  143    Method CodeIgniter\Commands\Database\ShowTableInfo::run() should return int|null but return statement is missing.   
 ------ -------------------------------------------------------------------------------------------------------------------- 

 ------ -------------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Encryption/GenerateKey.php                                                                          
 ------ -------------------------------------------------------------------------------------------------------------------- 
  87     Method CodeIgniter\Commands\Encryption\GenerateKey::run() should return int|null but empty return statement found.  
  94     Method CodeIgniter\Commands\Encryption\GenerateKey::run() should return int|null but empty return statement found.  
  104    Method CodeIgniter\Commands\Encryption\GenerateKey::run() should return int|null but return statement is missing.   
 ------ -------------------------------------------------------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------------------------------------------------------------ 
  Line   system/Commands/Generators/CommandGenerator.php                                                                         
 ------ ------------------------------------------------------------------------------------------------------------------------ 
  86     Method CodeIgniter\Commands\Generators\CommandGenerator::run() should return int|null but return statement is missing.  
 ------ ------------------------------------------------------------------------------------------------------------------------ 

 ------ ----------------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Generators/ConfigGenerator.php                                                                         
 ------ ----------------------------------------------------------------------------------------------------------------------- 
  82     Method CodeIgniter\Commands\Generators\ConfigGenerator::run() should return int|null but return statement is missing.  
 ------ ----------------------------------------------------------------------------------------------------------------------- 

 ------ --------------------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Generators/ControllerGenerator.php                                                                         
 ------ --------------------------------------------------------------------------------------------------------------------------- 
  88     Method CodeIgniter\Commands\Generators\ControllerGenerator::run() should return int|null but return statement is missing.  
 ------ --------------------------------------------------------------------------------------------------------------------------- 

 ------ ----------------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Generators/EntityGenerator.php                                                                         
 ------ ----------------------------------------------------------------------------------------------------------------------- 
  82     Method CodeIgniter\Commands\Generators\EntityGenerator::run() should return int|null but return statement is missing.  
 ------ ----------------------------------------------------------------------------------------------------------------------- 

 ------ ----------------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Generators/FilterGenerator.php                                                                         
 ------ ----------------------------------------------------------------------------------------------------------------------- 
  82     Method CodeIgniter\Commands\Generators\FilterGenerator::run() should return int|null but return statement is missing.  
 ------ ----------------------------------------------------------------------------------------------------------------------- 

 ------ --------------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Generators/MigrateCreate.php                                                                         
 ------ --------------------------------------------------------------------------------------------------------------------- 
  88     Method CodeIgniter\Commands\Generators\MigrateCreate::run() should return int|null but return statement is missing.  
 ------ --------------------------------------------------------------------------------------------------------------------- 

 ------ -------------------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Generators/MigrationGenerator.php                                                                         
 ------ -------------------------------------------------------------------------------------------------------------------------- 
  90     Method CodeIgniter\Commands\Generators\MigrationGenerator::run() should return int|null but return statement is missing.  
 ------ -------------------------------------------------------------------------------------------------------------------------- 

 ------ ---------------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Generators/ModelGenerator.php                                                                         
 ------ ---------------------------------------------------------------------------------------------------------------------- 
  86     Method CodeIgniter\Commands\Generators\ModelGenerator::run() should return int|null but return statement is missing.  
 ------ ---------------------------------------------------------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Generators/ScaffoldGenerator.php                                                                         
 ------ ------------------------------------------------------------------------------------------------------------------------- 
  119    Method CodeIgniter\Commands\Generators\ScaffoldGenerator::run() should return int|null but return statement is missing.  
 ------ ------------------------------------------------------------------------------------------------------------------------- 

 ------ ----------------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Generators/SeederGenerator.php                                                                         
 ------ ----------------------------------------------------------------------------------------------------------------------- 
  82     Method CodeIgniter\Commands\Generators\SeederGenerator::run() should return int|null but return statement is missing.  
 ------ ----------------------------------------------------------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------------------------------------------------------------ 
  Line   system/Commands/Generators/SessionMigrationGenerator.php                                                                
 ------ ------------------------------------------------------------------------------------------------------------------------ 
  84     Method CodeIgniter\Commands\Generators\SessionMigrationGenerator::run() should return int|null but return statement is  
         missing.                                                                                                                
 ------ ------------------------------------------------------------------------------------------------------------------------ 

 ------ --------------------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Generators/ValidationGenerator.php                                                                         
 ------ --------------------------------------------------------------------------------------------------------------------------- 
  82     Method CodeIgniter\Commands\Generators\ValidationGenerator::run() should return int|null but return statement is missing.  
 ------ --------------------------------------------------------------------------------------------------------------------------- 

 ------ -------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Help.php                                                                          
 ------ -------------------------------------------------------------------------------------------------- 
  79     Method CodeIgniter\Commands\Help::run() should return int|null but empty return statement found.  
  83     Method CodeIgniter\Commands\Help::run() should return int|null but return statement is missing.   
 ------ -------------------------------------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------------------------------------------------------------ 
  Line   system/Commands/Housekeeping/ClearDebugbar.php                                                                          
 ------ ------------------------------------------------------------------------------------------------------------------------ 
  63     Method CodeIgniter\Commands\Housekeeping\ClearDebugbar::run() should return int|null but empty return statement found.  
  68     Method CodeIgniter\Commands\Housekeeping\ClearDebugbar::run() should return int|null but return statement is missing.   
 ------ ------------------------------------------------------------------------------------------------------------------------ 

 ------ -------------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Housekeeping/ClearLogs.php                                                                          
 ------ -------------------------------------------------------------------------------------------------------------------- 
  73     Method CodeIgniter\Commands\Housekeeping\ClearLogs::run() should return int|null but empty return statement found.  
  84     Method CodeIgniter\Commands\Housekeeping\ClearLogs::run() should return int|null but empty return statement found.  
  89     Method CodeIgniter\Commands\Housekeeping\ClearLogs::run() should return int|null but return statement is missing.   
 ------ -------------------------------------------------------------------------------------------------------------------- 

 ------ --------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Server/Serve.php                                                                         
 ------ --------------------------------------------------------------------------------------------------------- 
  111    Method CodeIgniter\Commands\Server\Serve::run() should return int|null but return statement is missing.  
 ------ --------------------------------------------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Utilities/Environment.php                                                                          
 ------ ------------------------------------------------------------------------------------------------------------------- 
  89     Method CodeIgniter\Commands\Utilities\Environment::run() should return int|null but empty return statement found.  
  99     Method CodeIgniter\Commands\Utilities\Environment::run() should return int|null but empty return statement found.  
  106    Method CodeIgniter\Commands\Utilities\Environment::run() should return int|null but empty return statement found.  
  113    Method CodeIgniter\Commands\Utilities\Environment::run() should return int|null but empty return statement found.  
  124    Method CodeIgniter\Commands\Utilities\Environment::run() should return int|null but return statement is missing.   
 ------ ------------------------------------------------------------------------------------------------------------------- 

 ------ ----------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Utilities/Namespaces.php                                                                         
 ------ ----------------------------------------------------------------------------------------------------------------- 
  88     Method CodeIgniter\Commands\Utilities\Namespaces::run() should return int|null but return statement is missing.  
 ------ ----------------------------------------------------------------------------------------------------------------- 

 ------ --------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Utilities/Publish.php                                                                          
 ------ --------------------------------------------------------------------------------------------------------------- 
  80     Method CodeIgniter\Commands\Utilities\Publish::run() should return int|null but empty return statement found.  
  83     Method CodeIgniter\Commands\Utilities\Publish::run() should return int|null but return statement is missing.   
 ------ --------------------------------------------------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------------------------------------------------- 
  Line   system/Commands/Utilities/Routes.php                                                                         
 ------ ------------------------------------------------------------------------------------------------------------- 
  165    Method CodeIgniter\Commands\Utilities\Routes::run() should return int|null but return statement is missing.  
 ------ ------------------------------------------------------------------------------------------------------------- 

                                                                                                                        
 [ERROR] Found 54 errors                                                                                                
                                                                                                                        

Script phpstan analyse handling the analyze event returned with error code 1

@kenjis
Copy link
Member Author

kenjis commented Aug 18, 2022

If we change the return type in the future, we should probably change it to int only, but that would break all existing command classes. Changing to int|null would also break. I don't think it is worth such a destructive change.

@MGatner
Copy link
Member

MGatner commented Aug 18, 2022

My approval stands for now, since this is just naming what is already happening. A refactor of these methods is in order but need not be handled here. To @paulbalandan's concern, we should plan to deal with these actual return types when we refactor for PHP 8.

@kenjis kenjis merged commit f5ed714 into codeigniter4:4.3 Aug 18, 2022
@kenjis kenjis deleted the fix-phpdocs-CLI branch August 18, 2022 21:15
@MGatner
Copy link
Member

MGatner commented Aug 21, 2022

Ocramius has an elegant solution using a docblock conditional return type:

 * @return ($return is true ? int : void)

@paulbalandan
Copy link
Member

phpstan has this support already

@kenjis
Copy link
Member Author

kenjis commented Aug 22, 2022

See

* @return array|string
* @phpstan-return ($item is array ? array : string)

@MGatner
Copy link
Member

MGatner commented Aug 22, 2022

Is it worth applying this logic here? Ultimately I don't know that it matters, since I think the real fix is a native type and return null, but if this addresses @paulbalandan's concerns I'm open to the added complexity.

@kenjis
Copy link
Member Author

kenjis commented Aug 24, 2022

Is it worth applying this logic here?

No, it cannot be applied. Because here is not conditional.
Speaking of run() of a Command class, if a dev returns int, it returns int. If a dev don't write return, it returns void.
That's all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants