3.1 - Shell "requested" param

[HavokInspiration]

Dispatching a Shell within another shell (with Shell::dispatchShell()) makes the welcome message called twice :

Welcome to CakePHP v3.0.0 Console
---------------------------------------------------------------
App : src
Path: /home/yves/PhpStormProjects/cakephp-app/src/
---------------------------------------------------------------

First Shell output
Need to dispatch another shell inside this one

Welcome to CakePHP v3.0.0 Console
---------------------------------------------------------------
App : src
Path: /home/yves/PhpStormProjects/cakephp-app/src/
---------------------------------------------------------------

Second shell output

This PR adds a new parameter called "requested" that will prevent the welcome message to be displayed.
By extension, this parameter could be used in userland to detect if the Shell was called with the dispatchMethod.
Additionally, any call to dispatchShell() will append this parameter to the list of parameters.

More details on #6272.

[HavokInspiration]

I'll fix the failing tests

[markstory]

Couldn't you just test the truthiness of the param? Given that its always locked to a boolean it will be a yes/no condition.

[HavokInspiration]

Makes sense.

[HavokInspiration]

@markstory I've refactored it all and went with your suggestion of having a new extra arguments that can be used when dispatching a Shell. I went with passing the parameter directly to runCommand() and dispatch(). Felt simpler and avoid having the extra parameters stored somewhere. And since they are "runtime" parameter in a way, that makes more sense IMO.

It might be good to test the parsing of the dispatchShell() method. But I'll have to admit that I need some hint on how to do this, since all it returns is a bool.

[markstory]

! does implicit boolean casting for you.

[markstory]

Testing dispatchShell() is pretty painful which is why there aren't any existing tests sadly.

[HavokInspiration]

Ok, that's what I was afraid of.
What about having a method to do this part of the logic and add tests for it ? That would at least prevent regressions in arguments parsing if dispatchShell() needs modifications in the future.

[markstory]

@HavokInspiration Another method sounds like a good option to me.

[HavokInspiration]

Added a method to parse arguments given to dispatchShell() and corresponding tests.
I also did a "real" test dispatching a shell from another shell and it works okay.

It does feel flying without wings to not have tests validating that dispatchShell() works okay. I certainly would not want to be the guy who broke a part of the framework 😄

Is there no way to test this? With I don't know... A custom test shell that will overwrite out() to make it log the output, call a command that dispatch the same shell that calls out() and compares the final values of the outputed message and what we expect ?
Something like this :

class TestingShell extends Shell
{

    public $out = [];

    public function out($message = null, $newlines = 1, $level = Shell::NORMAL)
    {
        $this->out[] = $message;
    }

    public function testTask()
    {
        $this->out('I am a test task, I dispatch another Shell');
        $this->dispatchShell('testing dispatch_test_task');
    }

    public function dispatchTestTask()
    {
        $this->out('I am a dispatched Shell');
    }
}

//In test files
$TestingShell = new TestingShell();
$TestingShell->runCommand('test_task');

//Compare $TestingShell->out with our own expected array

It's a draft done quick and dirty, just to have something more concrete than my so poorly written explanation. But that might give some sort of coverage to dispatchShell(). Or maybe I'm missing something and that would not work at all.

[markstory]

Looks good to me.

[HavokInspiration]

@markstory I think this was merged too early. Since master was merged to 3.1, the dispatchShell test added on #6325 will probably fail (the welcome message will not be outputed, hence the expected value will not be correct anymore.
I planned on adding the tests this week (I should have posted a message, my bad)

I'll try to submit a quick fix for the test for the 3.1 branch with just github online editor (will check on my fork first).
I'll add more in depth tests to dispatchShell linked to this PR (hopefully, before the end of the week)

[markstory]

Whoops. Failing tests can be fixed easily enough 😄