Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

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

Sign up
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
theofidry wants to merge6 commits intosymfony:7.4
base:7.4
Choose a base branch
Loading
fromtheofidry:feat/raw-options

Conversation

theofidry
Copy link
Contributor

@theofidrytheofidry commentedJun 29, 2024
edited
Loading

QA
Branch?7.3
Bug fix?no
New feature?yes
Deprecations?no
IssuesNone
LicenseMIT

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 inpimcore.

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:

  • AddInput::getRawArguments(), which provides parsed arguments, likegetArguments(), 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.
  • AddInput::getRawOptions(), analogue case.
  • AddInput::unparse($parsedOptions): array: allows to reverseInput::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 presentedgetRawArguments() includesbool $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 aRawInput class that extendsSymfony\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 aDomainException('Not implemented') to avoid confusion).
    • Implementing the two methodsgetRawArguments() andgetRawOptions(). Those two should accept aInputInterface but you then need to check againstInput and if it's something else you are screwed.
  • Having another utility (in my case I implemented it as a static classInputOptionsSerializer) which is able to unparse the given options.
    • UnlikeInput::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 theInput::parse(). IMO the most awkward part of that process is that you need to account for the variants ofInputOption, i.e. if it is negatable, accepts a value or is an array.

IMO the above is a combination ofawkward and having tokeep 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 aresymfony/console andsymfony/process.

`demo.php`
<?phpdeclare(strict_types=1);namespaceApp;useSymfony\Component\Console\Application;useSymfony\Component\Console\Command\Command;useSymfony\Component\Console\Input\Input;useSymfony\Component\Console\Input\InputArgument;useSymfony\Component\Console\Input\InputInterface;useSymfony\Component\Console\Input\InputOption;useSymfony\Component\Console\Input\StringInput;useSymfony\Component\Console\Output\BufferedOutput;useSymfony\Component\Console\Output\OutputInterface;useSymfony\Component\Process\Process;useWebmozarts\Console\Parallelization\Input\InputOptionsSerializer;useWebmozarts\Console\Parallelization\Input\RawInput;usefunctionarray_diff_key;usefunctionarray_fill_keys;usefunctionarray_values;usefunctionsprintf;require__DIR__.'/vendor/autoload.php';class RawInputCommandextends Command{privateconstFIRST_REQ_ARG ='firstArg';privateconstSECOND_REQ_ARG ='secondArg';privateconstOPTIONAL_ARG ='optionalArg';privateconstFIRST_OPTION ='firstOpt';privateconstSUB_PROCESS_OPTION ='child';privateconstMAIN_PROCESS_ONLY_OPTION ='mainProcessOnlyOption';publicfunction__construct()    {parent::__construct('raw-input');    }publicfunctionconfigure():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',        );    }protectedfunctionexecute(InputInterface$input,OutputInterface$output,    ):int {$processInput = [            ...self::getProcessArguments($input),            ...self::getProcessOptions($input),        ];$output->write(implode('',$processInput));$process =newProcess([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>>     */privatestaticfunctiongetProcessArguments(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$inputinstanceof Input            ?array_values($input->getRawArguments())            : [];    }/**     * @return list<string>     */privatestaticfunctiongetProcessOptions(InputInterface$input):array    {if (!($inputinstanceof Input)) {return [];        }$options =$input->getRawOptions();        unset($options[self::MAIN_PROCESS_ONLY_OPTION]);$options[self::SUB_PROCESS_OPTION] =true;return$input->unparse($options);    }}$application =newApplication();$application->add(newRawInputCommand());$application->setDefaultCommand('raw-input');$application->setAutoExit(false);$application->run(newStringInput('raw-input firstArg secondArg --mainProcessOnlyOption'),$output =newBufferedOutput(),);// 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 processassert($output->fetch() ==='raw-input firstArg secondArg --child');

@carsonbotcarsonbot added this to the7.2 milestoneJun 29, 2024
@theofidrytheofidry changed the title[Console] Expose the original input arguments and options[Console] Expose the original input arguments and options and to unparse optionsJun 29, 2024
@theofidry
Copy link
ContributorAuthor

@chalasr updated

@theofidrytheofidryforce-pushed thefeat/raw-options branch 2 times, most recently fromd90a218 to25f2be2CompareJuly 1, 2024 19:14
@chalasr
Copy link
Member

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

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

  • parse() is not defined byInputInterface
  • 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
ContributorAuthor

Should thegetRawX() methods introduced here replace theArgvInput::getRawTokens()?

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

parse() is not defined by InputInterface

parse() is not, but it's caller, and why this unparse is needed, isInputInterface::bind(), it's just that::unbind() be closer to areset() 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 forArgvInput 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.

@theofidrytheofidryforce-pushed thefeat/raw-options branch 4 times, most recently froma009d4f to70c0e33CompareOctober 4, 2024 08:58
nicolas-grekas added a commit that referenced this pull requestOct 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       | MITThis 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
@fabpotfabpot modified the milestones:7.2,7.3Nov 20, 2024
@chalasr
Copy link
Member

Ready for another approval @symfony/mergers

@theofidry
Copy link
ContributorAuthor

theofidry commentedApr 18, 2025
edited
Loading

Addressed the last pieces of feedback and rebased it.

I also updated the description. A TL:DR; for a usage in the wild iswebmozarts/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

@GromNaNGromNaNApr 18, 2025
edited
Loading

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
ContributorAuthor

@theofidrytheofidryApr 19, 2025
edited
Loading

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

@GromNaNGromNaNApr 18, 2025
edited
Loading

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:

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

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

Copy link
ContributorAuthor

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 generallyInput 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 ofInput, 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

@GromNaNGromNaNApr 18, 2025
edited
Loading

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{publicfunction getRawArguments():array:publicfunctiongetRawOptions():array;}

Copy link
ContributorAuthor

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, isInputInterface. Having a separate interface would bring back something similar to this code:

publicstaticfunctiongetRawOptions(InputInterface$input):array    {return$inputinstanceof RawInputInterface            ?$input->getRawOptions()            : [];    }

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

Copy link
ContributorAuthor

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 implementingInputInterface without extendingInput, which AFAIK (but I can be wrong) isextremely rare, unless you're aware of more cases/usages in the wild?

@GromNaN
Copy link
Member

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

@GromNaNGromNaNApr 18, 2025
edited
Loading

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 methodinwebmozarts/console-parallelization, you have a list of option names to exclude. This parameter should be$excludedOptions?

Copy link
ContributorAuthor

@theofidrytheofidryApr 19, 2025
edited
Loading

Choose a reason for hiding this comment

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

InWebmozartsConsoleParallelization 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.

@fabpotfabpot modified the milestones:7.3,7.4May 26, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@GromNaNGromNaNGromNaN left review comments

@OskarStarkOskarStarkOskarStark left review comments

@chalasrchalasrchalasr approved these changes

@welcoMatticwelcoMatticAwaiting requested review from welcoMattic

@stofstofAwaiting requested review from stof

Assignees
No one assigned
Projects
None yet
Milestone
7.4
Development

Successfully merging this pull request may close these issues.

8 participants
@theofidry@chalasr@GromNaN@stof@welcoMattic@OskarStark@fabpot@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp