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

[nicolas-grekas]

Right now, Command::execute() returns int|void.

I'd suggest we should deprecate returning void (or null) by throwing a deprecation when the method returns a non-int, in Application.

We would need to update all core commands to return 0; where they return; right now.

And in 5.0, we would make returning a non-int throw an exception, in preparation for adding a real return type in 6.0.

Help wanted.

[ihmels]

I think that's a good idea. Why not add a return type in 5.0 so that a TypeError would be thrown? 🤔

[jschaedl]

So we basically need to do the following:

Tasks in 4.4

First of all we need to introduce a deprecation message for nullable or non-int return values of Command::execute()

• Add a deprecation message in Command class ([Console] Add deprecation message for non-int statusCode #33775)

After that we can adjust the core commands with exchanging return; to return 0; and add a return type-hint for the overridden method protected function execute(InputInterface $input, OutputInterface $output), but only for Command classes which are declared final (@nicolas-grekas Can we include Commands annotated with@final as well?):

I checked all Bundles and Bridges which provide Commands:

• DebugBundle (only one Command, which has a proper int return value in branch 4.4)
• FrameworkBundle ([Console] Add deprecation message for non-int statusCode #33775)
• SecurityBundle (only one Command, which has a proper int return value in branch 4.4)
• TwigBundle (includes only one Command that is specialising Symfony\Bridge\Twig\Command\LintCommand, execute method is not overridden)
• WebServerBundle ([Console] Add deprecation message for non-int statusCode #33775)
• TwigBridge ([Console] Add deprecation message for non-int statusCode #33775)

Tasks in 5.0

• Throw an exception on a non-int return value from Command::execute() in the Command class ([Console] Throw a TypeError for non-int return value calling Command::execute() #33808)

[nicolas-grekas]

@ihmels see #33236 for background on the topic

@jschaedl your list looks good

Can we include Commands annotated with @final as well?

yes, unless the annotation says @final since ..., which means "not before 5.0"

[andig]

I realize its too late but the PR doesn't give any reasoning as to why this change is helpful. It does break a large number of projects (or anyway it broke all of mine) and it makes the code fairly ugly with the lone return 0 at the end of any execute() method. Personally, I find this totally counterproductive.

[halaei]

I don't get this restriction. 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?

[vekien]

According to: https://symfony.com/doc/current/contributing/community/releases.html

A new Symfony minor version (e.g. 2.8, 3.2, 4.1) comes out every six months: one in May and one in November. It contains bug fixes and new features, but it doesn't include any breaking change, so you can safely upgrade your applications;

But 4.4 this is now throwing an exception, and broke all of my commands? Why is this forced in 4.4 and not 5.0? This is the second thing now that Symfony have broke stuff in a minor version.....

[xabbuh]

Can you be a bit more precise how this broke your application?

[andig]

Can you be a bit more precise how this broke your application?

It changes the execute method's signature and that constitutes a breaking change.

I realize its too late but the PR doesn't give any reasoning as to why this change is helpful.

It will throw a notice AND break phpstan.

We would need to update all core commands to return 0; where they return; right now.

You're forcing this on every developer using symfony commands. I feel the question alone shows little empathy for the developers hit by this change.

[xabbuh]

It changes the execute method's signature and that constitutes a breaking change.

The method signature of the Command class was changed in 5.0 where we intentionally did BC breaks. But that doesn't break applications updating from 4.3 to 4.4.

[vekien]

So you can see in 4.4 the trigger_error is set here: https://github.com/symfony/console/blob/4.4/Command/Command.php#L258 while it does have a @ it wont stop it being thrown in CLI scripts.

So many of my scripts (near 100 of them) all started throwing "Return value of X::execute() should always be of type int since Symfony 4.4".

The only fix was for me to go in and update all of them, thankfully it's quite an easy fix since I can just shove return 0 on them all, but this is still a BC as the command would still stop when this was thrown. (Could it be due to try/catches? many run in Queues and need this)

Example

[xabbuh]

This looks like an issue with your setup if silenced deprecations are turned into errors.

[andig]

This looks like an issue with your setup if silenced deprecations are turned into errors.

If feel strongly that my setup should not be dictatored by libraries used.

See my comment about phpstan. It does also break the CI build process due to https://github.com/symfony/console/blob/4.4/Command/Command.php#L153.

That being said, the PR also does not provide any reason for doing so in 4.4.

[xabbuh]

Symfony is using this way to trigger deprecations since at least 2.8. If your error handler doesn't respect the silenced errors that is indeed something we cannot fix in the core.

That being said, the PR also does not provide any reason for doing so in 4.4.

We always deprecate things in a minor release (4.4 here) before removing it in the next major version (5.0 here) to provide a smooth upgrade path.

[dbrumann]

This looks like an issue with your setup if silenced deprecations are turned into errors.

If feel strongly that my setup should not be dictatored by libraries used.

By default @ will silence errors. See: https://www.php.net/manual/en/language.operators.errorcontrol.php

If you have set a custom error handler function with set_error_handler() then it will still get called, but this custom error handler can (and should) call error_reporting() which will return 0 when the call that triggered the error was preceded by an @.

It seems that your error handler decides to ignore the intention behind the operator. Symfony does not "dictate" how you do things, but if you change rightfully expected behavior, you are responsible for the consequences.

[vekien]

So because I have a try/catch it becomes BC??? That is extremely janky, libraries should expect people to have error handling...

I have no custom error handling other than a try/catch, the rest is pure symfony, an example can be followed here: https://github.com/xivapi/xivapi.com/blob/master/src/Command/GameData/SaintCoinachRedisCommand.php

[xabbuh]

Your error handler defined on line 75 of that exact class does not respect silenced errors.

[xabbuh]

You may want to update it like this:

set_error_handler(function($errno, $errstr, $errfile, $errline, array $errcontext) {
    if (0 === error_reporting()) {
         return;
    }
    # print_r($errcontext);
    throw new \Exception("{$errfile} {$errline} - {$errstr}", 0);
});

[vekien]

Thanks @xabbuh i had wrongly assumed because it forced throwing exceptions which were catch later on it would be fine, reading your post I see it removes the silence, so this is my mistake, really sorry

[xabbuh]

I am glad we were able to resolve your issue. :)

[fmonts]

I think a : int should be added here so the IDE will warn you if you override it with :void? I stayed with 4.4 for about a year and never noticed the deprecation, only now that I upgraded to 5.1 I saw the error.