refactor: Deprecated method of returning `void' in Сommands

[neznaika0]

Description
I see that the commands must follow the style in which the numeric status code is returned. Now many commands have an deprecated solution - int|void. I suggest gradually rewriting the commands and tests for them.

I want to ask which type is preferable to return?

CodeIgniter4/system/Commands/Database/MigrateRefresh.php

Line 70 in af68358

public function run(array $params)

define('EXIT_SUCCESS', 0);        // no errors
define('EXIT_ERROR', 1);          // generic error
define('EXIT_USER_INPUT', 7);     // invalid user input
define('EXIT_DATABASE', 8);       // database error

This always applies to EXIT_ERROR, EXIT_DATABASE or EXIT_USER_INPUT? Because the parameters are set by the user.
In all other cases, return EXIT_SUCCESS.

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

[paulbalandan]

I'm not sure this is the preferred way to do this. Adding the deprecation PHPDoc makes the whole method deprecated in IDEs. We have made that mistake somewhere else in the framework causing users to complain that a method is deprecated but in fact only a part or implementation is deprecated.

I'm thinking of finding where the commands are ultimately called. Not sure but I think in Console or Boot::bootSpark(). Then, introspect the return when $command->run($params); is called. If null, meaning it was returning void, then trigger an E_USER_DEPRECATED warning that can be captured in the logs.

[neznaika0]

Yes, I remember the deprecated method in the request. I was hoping that it would be possible to update all commands to 4.7 and remove @deprecated.

The logging option is not bad. But you need to find all the call points. In this case, is it possible not to specify phpdoc? And get started.

Does it make sense to rewrite what I'm suggesting?

[neznaika0]

Will the tests have to be updated with new methods or completely replaced (with run() or call()? To check the status code. Or should I not do this? As:

public function testUpdateWithIncorrectLocaleOption(): void
    {
        self::$locale = 'test_locale_incorrect';
        $this->makeLocaleDirectory();

        $status = service('commands')->run('lang:find', [
            'dir'    => 'Translation',
            'locale' => self::$locale,
        ]);

        $this->assertSame(EXIT_USER_INPUT, $status);
    }

[michalsn]

I agree that deprecating the whole method doesn't make sense and would only confuse users, especially when only the return behavior is being reconsidered.

For the framework's own commands, we can update them to consistently return int status codes in 4.7 branch.

Personally, I wouldn't make any enforced changes for userland commands. If developers want to return a status code, they're free to do so. If not, the CLI will evaluate to 0 (EXIT_SUCCESS) anyway.

For tests, going with service('commands')->run() instead of calling command() would be enough, I guess.

I'm also wondering... since we allow calling commands from other commands using call(), it may be tempting to leverage the untyped run() method to return complex data for building workflows - for example:

$params = $this->call('command_one');
$this->call('command_two', $params);

So, while technically returning int would be expected, users may choose a more creative approach with commands, and we probably should keep this in mind.

[neznaika0]

Using call() looks like an error if the user expects to receive a non-numeric result.
This is a problem of non-strict typing. Leaving everything as it is, then it is preferable to change phpdoc to run(): mixed. How upsetting the ambiguities are.

Although Commands clearly expects a numeric result:

CodeIgniter4/system/CLI/Commands.php

Lines 54 to 61 in af68358

	/**
	* Runs a command given
	*
	* @param array<int|string, string|null> $params
	*
	* @return int Exit code
	*/
	public function run(string $command, array $params)

[michalsn]

@neznaika0 I know, and I agree, but I try to put myself in the shoes of other developers. Historically, we used the mixed type.

In v4, we won't change (or actually add) the return type for this method because it would unnecessarily complicate existing applications. Therefore, it seems that we should leave it as it is and not change anything. The void in CLI is automatically treated as a success. So, the return type is needed only for errors, and it's up to the user.

But this is only my point of view. If more people think we should change the PHPDoc return type to int, then let it be.