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

Command::execute() should always return int - deprecate returning null #30845

Closed
halaei opened this issue Dec 15, 2019 · 5 comments
Closed

Command::execute() should always return int - deprecate returning null #30845

halaei opened this issue Dec 15, 2019 · 5 comments

Comments

@halaei
Copy link
Contributor

halaei commented Dec 15, 2019

  • Laravel Version: 5.8.35 (I think it is in 6.x as well)
  • PHP Version: 7.2.25
  • Database Driver & Version: MySql 5.7
  • Symfony/Console: >= 4.4.0

Description:

According to symfony/symfony#33747, a change in Symfony, Command::execute must always return an integer value. I don't think it was a good change, but if it is supposed to remain, in Laravel, either the doc blocks for the generated commands' handle() function should change, or add some handy code can be added in Lavavel's override of execute() to convert return values to integer. I guess the plan is to not support non-ingeter return values in Symfony 5.

Steps To Reproduce:

Run a command that doesn't return null inside tinker to see the warning:

PHP Deprecated:  Return value of "App/Console/Commands/...::execute()" should always be of the type int since Symfony 4.4, NULL returned. in ....php on line ...
@stloyd
Copy link

stloyd commented Dec 15, 2019

Even in a strictly typed language like C I can have an int function that returns nothing, it will be automatically converted to 0. Also you can define main as being void. So why C is more flexible than Symfony here?

This has nothing to do with C vs Symfony, cause Symfony is framework based on PHP, which from the beginning converts method call without declared return type to null, that the problem here.

The points to force the return type is to make code more clear (often on error there was return 1 without return 0 on success) output, easier maintenance, less potential hacks which convert null into 0, easier portability between systems etc. all of those were described in related PR/issues IIRC.

Also if you dont like that deprecation, then make your code use exactly the version before it was added, there is no force to use latest.

@halaei
Copy link
Contributor Author

halaei commented Dec 15, 2019

@stloyd If this makes the code cleaner I am fine with it, even though I really don't get it. I don't think I will stick with the old versions just because of some deprecation message. I'd rather to fix either my console commands or see some changes in the Laravel, or better in my opinion, a revert in Symfony.

@halaei
Copy link
Contributor Author

halaei commented Dec 15, 2019

If Symfony remains as it is, I think for the next major version of laravel this change will be fine:

    /**
     * Execute the console command.
     *
     * @param  \Symfony\Component\Console\Input\InputInterface  $input
     * @param  \Symfony\Component\Console\Output\OutputInterface  $output
     * @return int
     */
    protected function execute(InputInterface $input, OutputInterface $output)
    {
        $result = $this->laravel->call([$this, 'handle']);
        return is_int($result) ? $result : 0;
    }

@RFreij
Copy link

RFreij commented Dec 15, 2019

Next major version of laravel already supports symfony 5: #30610

@halaei
Copy link
Contributor Author

halaei commented Dec 15, 2019

@RFreij Oh, I didn't noticed that. I guess this issue is closed then. Thanks

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

No branches or pull requests

3 participants