Skip to content

[Console] Bind input before executing the COMMAND event #15938

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 30, 2015
Merged

[Console] Bind input before executing the COMMAND event #15938

merged 1 commit into from
Sep 30, 2015

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Sep 27, 2015

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #10695 (problem 1)
License MIT
Doc PR -

Previously, $input wasn't very usefull in the console.command event, as the input was not yet bound to the command definition.

With this PR, the input is now bound twice: Once before the event is dispatched (to make it usefull in the listeners) and once at the original location in Command#run() (to allow changing the input definition in an event listener).

@stof
Copy link
Member

stof commented Sep 27, 2015

wouldn't this break cases where the command is allowing to get arguments interactively in interact() ?

@wouterj
Copy link
Member Author

wouterj commented Sep 27, 2015

@stof afaics, this only binds, the validation is still done after interact. So I don't think it should cause trouble.

It also means listeners can add options/arguments without getting validation errors.

@wouterj
Copy link
Member Author

wouterj commented Sep 27, 2015

Failures related to #15940 and not this PR

@xabbuh
Copy link
Member

xabbuh commented Sep 27, 2015

@wouterj You could rebase as #15940 was merged (though we will still see some failures due to the security tests I think).

@Tobion
Copy link
Contributor

Tobion commented Sep 27, 2015

Please also add a test where the listener adds an option. And check if it can be used.

@Tobion
Copy link
Contributor

Tobion commented Sep 28, 2015

I was looking when validation actually happens as I saw symfony/console#15.
It seems validation can happen both

@Tobion
Copy link
Contributor

Tobion commented Sep 28, 2015

wouldn't this break cases where the command is allowing to get arguments interactively in interact() ?

So this case from @stof doesn't break because

  • wrong arguments/options, e.g. too many would already error out when binding anyway
  • missing arguments can still be added in interact as the number of arguments is only validated later

@Tobion
Copy link
Contributor

Tobion commented Sep 28, 2015

Please also add a test where the listener adds an option. And check if it can be used.

I think exactly that is still no possible with this solution. If you call a command with an option/argument that is only about to be added by a listener, it will throw an exception already when trying to parse the input via $input->bind(). So the listener won't have a chance to change the definition.

@Tobion
Copy link
Contributor

Tobion commented Sep 28, 2015

I think to solve the problem, we really need to refactor the validation. bind() should not validate but should parse everything indepently. And only validate checks if there are too many/missing/wrong arguments or options.

@wouterj
Copy link
Member Author

wouterj commented Sep 28, 2015

@Tobion yeah, or wrap things inside a try..catch loop as is done in other bind cases. (not doing validation in bind() is a BC break).

@wouterj
Copy link
Member Author

wouterj commented Sep 28, 2015

So Input#validate() only validates if required arguments are passed. Input#bind() validates if passed options/arguments are valid. Added a try..catch loop around the binding before the event listener and added a test.

@wouterj wouterj force-pushed the console_input_definition_event branch from 8bd327f to c41899f Compare September 28, 2015 21:11
Tobion
Tobion reviewed Sep 28, 2015
View reviewed changes
src/Symfony/Component/Console/Application.php
try {
$command->mergeApplicationDefinition();
$input->bind($command->getDefinition());
} catch (InvalidArgumentException $e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be easiest to just catch all Symfony\Component\Console\Exception\ExceptionInterface

Copy link
Contributor

Choose a reason for hiding this comment

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

similarly https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/Console/Command/Command.php#L234 should be updated

@wouterj wouterj force-pushed the console_input_definition_event branch from c41899f to 0af1676 Compare September 30, 2015 12:45
@wouterj
Copy link
Member Author

wouterj commented Sep 30, 2015

ping @symfony/deciders PR is now updated and ready to be merged imo

@Tobion
Copy link
Contributor

Tobion commented Sep 30, 2015

👍

Status: Reviewed

@xabbuh
Copy link
Member

xabbuh commented Sep 30, 2015

👍

Should we add a changelog entry so that people interested in changing the definition notice this is possible now?

@fabpot
Copy link
Member

fabpot commented Sep 30, 2015

Thank you @wouterj.

@fabpot fabpot merged commit 0af1676 into symfony:2.8 Sep 30, 2015
fabpot added a commit that referenced this pull request Sep 30, 2015
@fabpot
feature #15938 [Console] Bind input before executing the COMMAND even…
3b3431e 
…t (WouterJ)

This PR was merged into the 2.8 branch.

Discussion
----------

[Console] Bind input before executing the COMMAND event

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #10695 (problem 1)
| License       | MIT
| Doc PR        | -

Previously, `$input` wasn't very usefull in the `console.command` event, as the input was not yet bound to the command definition.

With this PR, the input is now bound twice: Once before the event is dispatched (to make it usefull in the listeners) and once at the original location in `Command#run()` (to allow changing the input definition in an event listener).

Commits
-------

0af1676 Bind input before executing the COMMAND event
@wouterj wouterj deleted the console_input_definition_event branch September 30, 2015 20:37
@Tobion Tobion mentioned this pull request Sep 30, 2015
Closed
@fabpot fabpot mentioned this pull request Nov 16, 2015
Merged
@wouterj wouterj mentioned this pull request Nov 22, 2015
Closed
@chalasr chalasr mentioned this pull request Mar 2, 2017
Merged
fabpot added a commit that referenced this pull request Mar 5, 2017
@fabpot
bug #21841 [Console] Do not squash input changes made from console.co…
b8b6774 
…mmand event (chalasr)

This PR was merged into the 2.8 branch.

Discussion
----------

[Console] Do not squash input changes made from console.command event

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #19441
| License       | MIT
| Doc PR        | n/a

Setting arguments/options from the `console.command` event is expected to work since #15938

Commits
-------

c8d364b [Console] Do not squash input changes made from console.command event
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Status: Reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@wouterj @stof @xabbuh @Tobion @fabpot @carsonbot