-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
base: 7.3
Are you sure you want to change the base?
Conversation
335b92b
to
5399e1f
Compare
5399e1f
to
5a046cb
Compare
@chalasr updated |
d90a218
to
25f2be2
Compare
Should the As discussed, I'm not entirely convinced we need
|
They could, but to this effect if what you're looking for is what
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
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. |
a009d4f
to
70c0e33
Compare
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
70c0e33
to
085c263
Compare
085c263
to
a9ed6ca
Compare
Ready for another approval @symfony/mergers |
cc76828
to
9338ba8
Compare
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. |
$unparsedOption = self::unparseOption( | ||
$this->definition->getOption($optionName), | ||
$optionName, | ||
$parsedOption, | ||
); | ||
|
||
$unparsedOptions[] = \is_array($unparsedOption) ? $unparsedOption : [$unparsedOption]; |
There was a problem hiding this comment.
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:
$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)], | |
}; |
There was a problem hiding this comment.
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
/** | ||
* 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 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.
* @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 |
There was a problem hiding this comment.
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;
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
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 |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
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:
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:
Input::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.Input::getRawOptions()
, analogue case.Input::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 presented
getRawArguments()
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:
RawInput
class that extendsSymfony\Component\Console\Input\Input
to have access to the protected#arguments
and#options
properties. This involves:DomainException('Not implemented')
to avoid confusion).getRawArguments()
andgetRawOptions()
. Those two should accept aInputInterface
but you then need to check againstInput
and if it's something else you are screwed.InputOptionsSerializer
) which is able to unparse the given options.Input::parse()
, which is abstract because it can vary from an input type to another, unparsing is a more universal operation.Input::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 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
andsymfony/process
.`demo.php`