Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[Console] Expose the original input arguments and options and to unparse options #57598

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

Open
wants to merge 6 commits into
base: 7.3
Choose a base branch
Loading
from

Conversation

theofidry
Copy link
Contributor

@theofidry theofidry commented Jun 29, 2024

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
Issues None
License MIT

Description

The problem this PR aims to solve, is added the necessary tools to the Symfony Console input in order to be able to forward the arguments and options to a child process with the following constraints:

  • To not include arguments or options which were not passed (i.e. exclude optional arguments and options).
  • Be able to manipulate the arguments and/or options, e.g. to exclude specific options and add new ones for the child process.

Example of usage in the wild: console-parallelization which is also used in pimcore.

Solution

Quick note: I came across #54347 beforehand to check if that would be enough to achieve what I wanted, but that is not the case unfortunately.

The solution to allow the above is as follow:

  • Add Input::getRawArguments(), which provides parsed arguments, like getArguments(), but unlike it does not include default arguments. It is important as one's code may rely on the presence of an option to do an action.
  • Add Input::getRawOptions(), analogue case.
  • Add Input::unparse($parsedOptions): array: allows to reverse Input::parse(). Edit: differently to the original MR, this does not encode back the tokens. Indeed, as noted by @stof, they aim to be passed to a process, so encoding it once would result in having them encoded once more (which would not be correct).

Considerations

The presented getRawArguments() includes bool $strip = false. I don't peculiarly need this, but this was to stay consistent with #54347 and there is use cases (as highlighted by the demo). But it's also easy to do without.

User-land alternatives

The main motivation to propose those changes core is because the user land implementation is, while possible, very awkward. Indeed, here is what it currently entails:

  • Implement a RawInput class that extends Symfony\Component\Console\Input\Input to have access to the protected #arguments and #options properties. This involves:
    • Implementing 6 methods "doing nothing" (in my case I make them throw a DomainException('Not implemented') to avoid confusion).
    • Implementing the two methods getRawArguments() and getRawOptions(). Those two should accept a InputInterface but you then need to check against Input and if it's something else you are screwed.
  • Having another utility (in my case I implemented it as a static class InputOptionsSerializer) which is able to unparse the given options.
    • Unlike Input::parse(), which is abstract because it can vary from an input type to another, unparsing is a more universal operation.
    • You basically need to reverse engineer the Input::parse(). IMO the most awkward part of that process is that you need to account for the variants of InputOption, i.e. if it is negatable, accepts a value or is an array.

IMO the above is a combination of awkward and having to keep up with fairly internal pieces that requires to reverse engineer the source code.

This PR shows the hacky roundabout code used to achieve this: webmozarts/console-parallelization#328.

Demo

This small script aims at showcase the use case mentioned in the description. It is a bit verbose because I tried to provide a scenario that captures enough of the case and allow one to easily copy/paste to tinker it.

The only dependencies required are symfony/console and symfony/process.

`demo.php`
<?php declare(strict_types=1);

namespace App;

use Symfony\Component\Console\Application;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Input\Input;
use Symfony\Component\Console\Input\InputArgument;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Input\InputOption;
use Symfony\Component\Console\Input\StringInput;
use Symfony\Component\Console\Output\BufferedOutput;
use Symfony\Component\Console\Output\OutputInterface;
use Symfony\Component\Process\Process;
use Webmozarts\Console\Parallelization\Input\InputOptionsSerializer;
use Webmozarts\Console\Parallelization\Input\RawInput;
use function array_diff_key;
use function array_fill_keys;
use function array_values;
use function sprintf;

require __DIR__.'/vendor/autoload.php';

class RawInputCommand extends Command
{
    private const FIRST_REQ_ARG = 'firstArg';
    private const SECOND_REQ_ARG = 'secondArg';
    private const OPTIONAL_ARG = 'optionalArg';

    private const FIRST_OPTION = 'firstOpt';
    private const SUB_PROCESS_OPTION = 'child';
    private const MAIN_PROCESS_ONLY_OPTION = 'mainProcessOnlyOption';

    public function __construct()
    {
        parent::__construct('raw-input');
    }

    public function configure(): void
    {
        $this->addArgument(
            self::FIRST_REQ_ARG,
            InputArgument::REQUIRED,
            'The first argument',
        );
        $this->addArgument(
            self::SECOND_REQ_ARG,
            InputArgument::REQUIRED,
            'The second argument',
        );
        $this->addArgument(
            self::OPTIONAL_ARG,
            InputArgument::OPTIONAL,
            'An optional argument',
            'default',
        );
        $this->addOption(
            self::FIRST_OPTION,
            null,
            InputOption::VALUE_NONE,
            'The first option',
        );
        $this->addOption(
            self::SUB_PROCESS_OPTION,
            null,
            InputOption::VALUE_NONE,
            'An option to indicate the command is being executed in a sub-process',
        );
        $this->addOption(
            self::MAIN_PROCESS_ONLY_OPTION,
            null,
            InputOption::VALUE_NONE,
            'An option that should not be passed to the main process',
        );
    }

    protected function execute(
        InputInterface $input,
        OutputInterface $output,
    ): int {
        $processInput = [
            ...self::getProcessArguments($input),
            ...self::getProcessOptions($input),
        ];

        $output->write(implode(' ', $processInput));

        $process = new Process([
            PHP_BINARY,
            __DIR__,
            ...$processInput,
        ]);

        if ($output->isVerbose()) {
            $output->write(
                sprintf(
                    'Running command: %s',
                    $process->getCommandLine(),
                ),
            );
        }

        $process->run();

        if ($output->isVerbose()) {
            $output->writeln($process->getOutput());
        }

        return $process->getExitCode();
    }

    /**
     * @return list<string|bool|int|float|null|array<string|bool|int|float|null>>
     */
    private static function getProcessArguments(InputInterface $input): array
    {
        // We want the same arguments as the current command
        // There is cases where the command invoked could be different though
        // in which case the command name should be explicitly added, and
        // we can use $input->getRawArguments(true) instead.
        return $input instanceof Input
            ? array_values($input->getRawArguments())
            : [];
    }

    /**
     * @return list<string>
     */
    private static function getProcessOptions(InputInterface $input): array
    {
        if (!($input instanceof Input)) {
            return [];
        }

        $options = $input->getRawOptions();
        unset($options[self::MAIN_PROCESS_ONLY_OPTION]);
        $options[self::SUB_PROCESS_OPTION] = true;


        return $input->unparse($options);
    }
}

$application = new Application();
$application->add(new RawInputCommand());
$application->setDefaultCommand('raw-input');
$application->setAutoExit(false);

$application->run(
    new StringInput('raw-input firstArg secondArg --mainProcessOnlyOption'),
    $output = new BufferedOutput(),
);

// As you can see:
// - optionalArg is not passed to the child process as was not passed by the user
// - mainProcessOnlyOption is not passed to the child process
// - firstOpt is not passed to the child process as was not passed by the user
// - child is passed to the child process
assert($output->fetch() === 'raw-input firstArg secondArg --child');

@theofidry theofidry requested a review from chalasr as a code owner June 29, 2024 12:06
@carsonbot carsonbot added this to the 7.2 milestone Jun 29, 2024
@theofidry theofidry changed the title [Console] Expose the original input arguments and options [Console] Expose the original input arguments and options and to unparse options Jun 29, 2024
@theofidry theofidry requested a review from welcoMattic as a code owner July 1, 2024 19:05
@theofidry
Copy link
Contributor Author

@chalasr updated

@theofidry theofidry force-pushed the feat/raw-options branch 2 times, most recently from d90a218 to 25f2be2 Compare July 1, 2024 19:14
@chalasr
Copy link
Member

chalasr commented Jul 13, 2024

Should the getRawX() methods introduced here replace the ArgvInput::getRawTokens()?
/cc @lyrixx - does they work for castor's usecase?

As discussed, I'm not entirely convinced we need unparse():

  • parse() is not defined by InputInterface
  • could the mentioned use case be solved through manipulating the retval of __toString()?
  • overall, use cases for parse()/unparse() are marginal, so is it worth it?

@theofidry
Copy link
Contributor Author

Should the getRawX() methods introduced here replace the ArgvInput::getRawTokens()?

They could, but to this effect if what you're looking for is what getRawTokens() provides, then it's simpler than using getRawX().

parse() is not defined by InputInterface

parse() is not, but it's caller, and why this unparse is needed, is InputInterface::bind(), it's just that ::unbind() be closer to a reset() IMO.

could the mentioned use case be solved through manipulating the retval of __toString()?

As highlighted in the description, they cannot, at least not with the same level of sketchiness as the workaround currently employed which as listed in the "user-land alternative" section. Rather than manipulating a string, I would rather use raw tokens, but that is only available for ArgvInput and you still need to work out manipulating the tokens based on the input option names and types (e.g. negatable).

overall, use cases for parse()/unparse() are marginal, so is it worth it?

Can't decide on that, but to fix it "correctly" it is certainly the right place. Personally I find weird that the framework does not provide the primitive to reverse the transformation it does.

@theofidry theofidry force-pushed the feat/raw-options branch 4 times, most recently from a009d4f to 70c0e33 Compare October 4, 2024 08:58
nicolas-grekas added a commit that referenced this pull request Oct 4, 2024
This PR was merged into the 7.2 branch.

Discussion
----------

[Console] Use `assertSame` for input tests

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no (test refactor)
| New feature?  | no
| Deprecations? | no
| Issues        | None
| License       | MIT

This is a test refactor hence I am targeting 7.2 to avoid propagating noise. I originally intended to do it as part of #57598 as per `@OskarStark` comment but it turns out there is a few side effects so I preferred a dedicated PR to avoid confusion.

Commits
-------

cb619ee [Console] Use assertSame for input tests
@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
@chalasr
Copy link
Member

chalasr commented Dec 7, 2024

Ready for another approval @symfony/mergers

@theofidry
Copy link
Contributor Author

theofidry commented Apr 18, 2025

Addressed the last pieces of feedback and rebased it.

I also updated the description. A TL:DR; for a usage in the wild is webmozarts/console-parallelization#328 which shows the hacky boilertemplate used to achieve this otherwise.

Comment on lines 216 to 222
$unparsedOption = self::unparseOption(
$this->definition->getOption($optionName),
$optionName,
$parsedOption,
);

$unparsedOptions[] = \is_array($unparsedOption) ? $unparsedOption : [$unparsedOption];
Copy link
Member

@GromNaN GromNaN Apr 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the code is simpler if you remove the private functions and you directly create arrays of strings:

Suggested change
$unparsedOption = self::unparseOption(
$this->definition->getOption($optionName),
$optionName,
$parsedOption,
);
$unparsedOptions[] = \is_array($unparsedOption) ? $unparsedOption : [$unparsedOption];
$option = $this->definition->getOption($optionName);
$unparsedOptions[] = match (true) {
$option->isNegatable() => [\sprintf('--%s%s', $value ? '' : 'no-', $name)],
!$option->acceptValue() => [\sprintf('--%s', $name)],
$option->isArray() => array_map(fn ($item) => \sprintf('--%s=%s', $name, $item), $value),
default => [\sprintf('--%s=%s', $name, $value)],
};

Copy link
Contributor Author

@theofidry theofidry Apr 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally find it harder to understand (the removing the private function part) but as you wish. Totally agree with making it an array right away though thanks

Comment on lines +195 to +205
/**
* Returns a stringified representation of the options passed to the command.
* The options must NOT be escaped as otherwise passing them to a `Process` would result in them being escaped twice.
*
* @param string[] $optionNames Names of the options returned. If null, all options are returned. Requested options
* that either do not exist or were not passed (even if the option has a default value)
* will not be part of the method output.
*
* @return list<string>
*/
public function unparse(?array $optionNames = null): array
Copy link
Member

@GromNaN GromNaN Apr 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function should be extracted to a dedicated class (SRP) so that it's easier to maintain: we can change it more easily. I don't know for the class name and namespace (we need to think about it). The method signature would be:

function format(RawInputInterface $input, ?array $optionNames = null): array { /* ... */ }

Note: webmozarts/console-parallelization has a class dedicated to serialization, and that's great.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree on principle, and more generally Input and its children are doing too much making them harder to maintain (although thankfully they don't change often, so that alleviates that).

I am not sure it is best to move this now though, because:

  • in essence it is the counter-part of ::parse() which is part of Input, hence you would put two very closely related pieces in different places
  • ::parse() is abstract, hence every children implement its own. So in theory you could have different implementations of ::unparse() to specific to an input implementation.

Comment on lines +22 to 26
* @method getRawArguments(bool $strip = false): array<string|bool|int|float|null|array<string|bool|int|float|null>> Returns all the given arguments NOT merged with the default values.
* @method getRawOptions(): array<string|bool|int|float|array<string|bool|int|float|null>|null> Returns all the given options NOT merged with the default values.
* @method unparse(): list<string> Returns a stringified representation of the options passed to the command.
*/
interface InputInterface
Copy link
Member

@GromNaN GromNaN Apr 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid any BC break, we can add a new interface for these new capabilities:

interface RawInputInterface
{
    public function getRawArguments(): array:
    public function getRawOptions(): array;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would that really help? In the end what you receive, because that is what is typed everywhere, is InputInterface. Having a separate interface would bring back something similar to this code:

    public static function getRawOptions(InputInterface $input): array
    {
        return $input instanceof RawInputInterface
            ? $input->getRawOptions()
            : [];
    }

Which... err yes in practice if we make Input implement it it will work 99% of the cases but type-wise leaves it to be desired.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify: the only affected users are users implementing InputInterface without extending Input, which AFAIK (but I can be wrong) is extremely rare, unless you're aware of more cases/usages in the wild?

@GromNaN
Copy link
Member

GromNaN commented Apr 18, 2025

Sorry to be late in the review. I think it's a good feature, but the implementation can be improved.

*
* @return list<string>
*/
public function unparse(?array $optionNames = null): array
Copy link
Member

@GromNaN GromNaN Apr 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to how you would use this method in webmozarts/console-parallelization, you have a list of option names to exclude. This parameter should be $excludedOptions?

Copy link
Contributor Author

@theofidry theofidry Apr 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In WebmozartsConsoleParallelization we know the options we do NOT want, hence to get the ones we need to forward we need to juggle with the command definition. It is a specific use case for that feature, but not the only one, for instance you could want those values for specific options.

That's why I thought it would be better in Symfony to have a more re-usable piece of code (which is also simpler) even thought that means a tiny bit more work on my side.

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

Successfully merging this pull request may close these issues.

8 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.