FEATURE: Improve api / refactor `Scripts::executeCommand`

[mhsdesign]

Scripts::executeCommand has currently an odd, i suppose historically evolved api https://github.com/neos/flow-development-collection/blob/1531a8125ad41e62324c7a85e440c14c1cb768ac/Neos.Flow/Classes/Core/Booting/Scripts.php#L682

1. its hard to get the actual output of the command, currently one needs to ob_get_clean(); which seems pretty hacky.
2. its not obvious at first what the behavior on error is. The returned status code is actually irrelevant - it will always be true because otherwise we throw an exceptions.
3. the doc commend $outputResults if false the output of this command is only echoed if the execution was not successful is lying. In case of an error the output is converted into an exception

[mhsdesign]

@kitsunet what do you think about adding a new parameter &$output to executeCommand (via a bugfix release) or are you fine with:

            ob_start();

            try {
                Scripts::executeCommand(
                    'neos.setup:setup:executeruntimehealthchecks',
                    $this->configurationManager->getConfiguration(ConfigurationManager::CONFIGURATION_TYPE_SETTINGS, 'Neos.Flow')
                );
            } catch (SubProcessException $subProcessException) {
                exit(1);
            }

            // hack see: https://github.com/neos/flow-development-collection/issues/3112
            $json = ob_get_clean();

see https://github.com/neos/setup/blob/439277485f0172dfc3b196ca6f43cd15fbcdbbc5/Classes/RequestHandler/SetupCliRequestHandler.php#L136

[kdambekalns]

Agree.

1. Add an $output by-reference variable
2. Always return true
3. I'd say this works fine as is, and the docblock should be adjusted

[mhsdesign]

Fyi the behaviour to return false was removed 10 years ago 😂 https://github.com/neos/flow-development-collection/commit/527790b182abe99a2f4be242a6932e6eb0687ee9

can we just remove the return type and have void? Hmm or mark the returntype as deprecated?

[bwaidelich]

Slightly related: I realized the other day, that an exception in the invoked command leads to an exit code of 0..

OK, frankly this is almost not related at all because it does not affect the signature of this method probably.. I'll leave it here as FYI anyways :)

[mhsdesign]

Originally posted by @bwaidelich in https://github.com/neos/flow-development-collection/issues/3116#issuecomment-1641598749

Allow me to sneak in another "feature request" (and obviously feel free to ignore it): It would be great if this code could be used "from the outside" somehow so that we could build custom command triggers more easily (e.g. for the Neos 9.0 catchup trigger). I had to copy this code multiple times already, for example for https://github.com/bwaidelich/Wwwision.BatchProcessing/blob/main/Classes/BatchProcess.php#L100

[mhsdesign]

@bwaidelich @kitsunet and me agree that it might be time for a new method, or rather a new less static implementation.

---

Originally posted by @kitsunet in https://github.com/neos/flow-development-collection/issues/3118#issuecomment-1641510127 Meh, by reference is fugly, maybe we should just rather introduce a proper new method in an actual release than doing this?

---

Originally posted by @mhsdesign in https://github.com/neos/flow-development-collection/issues/3118#issuecomment-1641526544 i would not oppose. This 10 year old code had it coming to be refactored ;)

Flags like $outputResults to control legacy behavior could be removed. Odd return types as well and the methods should not take the flow settings as argument ;) Rather be unary or in this case with only two args.

class SubProcessHelper // to be named
{
    private function __construct(
        private array $settings
    ) {
    }
    
    public static function fromFlowConfiguration(array $settings): self
    {
        return new self($settings);
    }

    /**
     * Executes the given command as a sub-request to the Flow CLI system.
     *
     * @param string $commandIdentifier E.g. neos.flow:cache:flush
     * @param array $commandArguments Command arguments
     * @return array containing every line of output from the command. {@see exec()}
     * @throws Exception\SubProcessException The execution of the sub process failed
     * @api
     */
    public function executeCommand(string $commandIdentifier, array $commandArguments = []): array;

    public function executeCommandAsync(string $commandIdentifier, array $commandArguments = []);

    public function buildPhpCommand(): string;
}

---

Originally posted by @bwaidelich in https://github.com/neos/flow-development-collection/issues/3118#issuecomment-1641604910

maybe we should just rather introduce a proper new method in an actual release than doing this?

I agree. But if we do so, we should consider using https://symfony.com/doc/current/components/process.html internally – I used that in some tests and it seems to be much more stable.

Alternatively https://github.com/reactphp/child-process is a really interesting approach, but I doubt that we are ready to go all in ReactPHP just yet :)

---

[mhsdesign]

Also, the static Scripts is currently horrible testable: TODO: Refactor Scripts class to be more testable.

https://github.com/neos/flow-development-collection/blob/ab1165a6e0863b0f63827faf6cee382216bab025/Neos.Flow/Tests/Unit/Core/Booting/ScriptsTest.php#L26

I now get why you probably shouldn't commit todos - because it will take like 10 years to resolve them by some random other dude 😂

[bwaidelich]

because it will take like 10 years to resolve them by some random other dude

That's exactly why we should add those 😂

Suggestion for a potential migration path:

1. Create some FlowCommandProcess extending the symfony Process (similar to the PhpProcess) and move that buildPhpCommand magic there
2. Mark executeCommand and executeCommandAsync deprecated and make them use the new FlowCommandProcess internally

In my (maybe naive) imagination this could look as simple as:

    /**
     * @deprecated with Neos 9.0 - bla di blubb
     */
    public static function executeCommand(string $commandIdentifier, array $settings, bool $outputResults = true, array $commandArguments = []): bool
    {
        $process = new FlowCommandProcess($commandIdentifier, $commandArguments);
        $process->run();
        if (!$process->isSuccessful()) {
          // handle error
        }
        if ($outputResults) {
            echo $process->getOutput();
        }
        return $process->isSuccessful();
    }

    /**
     * @deprecated with Neos 9.0 - bla di blubb
     */
    public static function executeCommandAsync(string $commandIdentifier, array $settings, array $commandArguments = []): void
    {
        $process = new FlowCommandProcess($commandIdentifier, $commandArguments);
        $process->start();
    }

[mhsdesign]

Ha lol, christian tried already once pulling symfony into the game: https://github.com/neos/flow-development-collection/pull/75