Skip to content
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

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

Merged
merged 1 commit into from Mar 5, 2017

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Mar 2, 2017

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

@@ -859,6 +859,10 @@ protected function doRunCommand(Command $command, InputInterface $input, OutputI
// ignore invalid options/arguments for now, to allow the event listeners to customize the InputDefinition
}

// don't bind the input again as it would override any input argument/option set from the command event in
// addition of being useless
Copy link
Member

Choose a reason for hiding this comment

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

in addition to being useless

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@nicolas-grekas nicolas-grekas added this to the 2.8 milestone Mar 4, 2017
@@ -678,6 +681,11 @@ public function asXml($asDom = false)
return $output->fetch();
}

public function setInputBound($inputBound)
Copy link
Member

Choose a reason for hiding this comment

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

its always suspicious to me when a bug fix introduces a new public method - also, I don't get what this does without reading twice or more the code itself. is it me? :)

Copy link
Member Author

@chalasr chalasr Mar 4, 2017

Choose a reason for hiding this comment

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

I'm not really happy about that but adding this method seems the only way to fix it without breaking anything (changing the way the input is bound would do, never binding the input from the command too).

I don't get what this does without reading twice or more the code itself

That's the console... application change the state of commands all the time, ugly but actually necessary. #19441 (comment) explain well what happens, looking at it may make this easier to read, this too

Copy link
Member Author

Choose a reason for hiding this comment

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

made it internal

@fabpot
Copy link
Member

fabpot commented Mar 5, 2017

Thank you @chalasr.

@fabpot fabpot merged commit c8d364b into symfony:2.8 Mar 5, 2017
fabpot added a commit that referenced this pull request Mar 5, 2017
…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
@chalasr chalasr deleted the fix-cmdevent-change-input branch March 5, 2017 20:07
This was referenced Mar 6, 2017
@bendavies
Copy link
Contributor

bendavies commented Mar 9, 2017

This breaks my project on upgrade to 3.2.5.
I add an argument in the console.command event.

This PR causes the input not to be rebound after the event, which will result in an error
Not enough arguments (missing: "foo"). where foo is the argument I added in the event.

EDIT:
fixed by adding
$command->setInputBound(false);
to my listener. not great.

@chalasr
Copy link
Member Author

chalasr commented Mar 9, 2017

@bendavies Why do you add an argument from your event listener if you expect it to be removed afterwards? Isn't the foo argument useless in this case? I'm trying to understand the use case it breaks

@bendavies
Copy link
Contributor

I don't expect it to be removed afterwards. your use case is not the same as mine.
Please download and run run this melody script for a reproduction:
https://gist.github.com/bendavies/cb05d936d1d6cd6bff266aef1ad90bf5

melody run demo.php greet ben
Hello ben!

Change the version of console to 3.2.5 and rerun:

melody run demo.php greet ben

  [Symfony\Component\Console\Exception\RuntimeException]
  Not enough arguments (missing: "name").

greet [-h|--help] [-q|--quiet] [-v|vv|vvv|--verbose] [-V|--version] [--ansi] [--no-ansi] [-n|--no-interaction] [--] <command> <name>

@stof
Copy link
Member

stof commented Mar 9, 2017

this PR indeed breaks things when the listener is changing the command definition rather than changing the input.
We should reset the flag to false when changing the definition by adding arguments or options.

@chalasr
Copy link
Member Author

chalasr commented Mar 9, 2017

@bendavies Thank you for the gist, that's clear now. I'm on it.

@stof Resetting the flag to false would prevent to set the value of the newly added argument from the same listener, right? e.g:

$event->getCommand()->addArgument('name', InputArgument::REQUIRED);
$event->getInput()->setArgument('name', 'ben');

I'm not sure where the flag should be reset also

@chalasr
Copy link
Member Author

chalasr commented Mar 9, 2017

The only way I see is to don't set the flag at all (keeping the old behavior which doesn't allow to set the argument value), make the setInputBound() public and let the responsibility of calling it to the user. This means revert the change from 2.8 and do it on master

@chalasr
Copy link
Member Author

chalasr commented Mar 9, 2017

I'll propose the solution of my previous comment today if I don't find better meanwhile.

@chalasr
Copy link
Member Author

chalasr commented Mar 9, 2017

@bendavies could you please try #21953?

@pierre-tranchard
Copy link

pierre-tranchard commented Mar 9, 2017

I encountered the same problem for a different case (same exception message @bendavies got).
Consider your kernel is brand-aware and you have command to execute with a brand option (or argument, whatever).

I made a supervisor command mechanism. You call the supervisor command and it will call the supervised command with the same arguments / options with the right brand.

This PR introduced a bug in my case because the input arguments & options weren't pass to the supervised command anymore.

Hence, for my custom input, I had to bind the input to the definition manually.

Now it works, but I'm not sure your new solution won't introduce a new BC in my case.

@chalasr
Copy link
Member Author

chalasr commented Mar 9, 2017

@pierre-tranchard Could you please try to apply the new proposed solution on your project (replacing your workaround) and tell us if it works with your original code? We will fix this BC break through a way or another, so workarounds should be removed.

edit: the new solution should work even with your workaround in place

@pierre-tranchard
Copy link

i'll give a try. I'll give my feedback on your new PR

@chalasr
Copy link
Member Author

chalasr commented Mar 9, 2017

That would be great, thank you

chalasr added a commit to chalasr/symfony that referenced this pull request Mar 16, 2017
…from console.command event (chalasr)"

This reverts commit b8b6774, reversing
changes made to 8279055.
fabpot added a commit that referenced this pull request Mar 22, 2017
…ade from console.command event (chalasr)" (chalasr)

This PR was merged into the 2.8 branch.

Discussion
----------

Revert "bug #21841 [Console] Do not squash input changes made from console.command event (chalasr)"

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

A bit frustrated to revert this change since the BC break report lacks of information, making us unable to reproduce nor to look at improving the situation.
I'm going to re-propose this on master, covering the BC break that is identified, fixed and tested using the changes made in #21953. That will let the choice for the reporter to upgrade using the 1 required LOC.

Commits
-------

5af47c4 Revert "bug #21841 [Console] Do not squash input changes made from console.command event (chalasr)"
fabpot added a commit that referenced this pull request Mar 22, 2017
* 2.8:
  [HttpFoundation] Fix missing handling of for/host/proto info from "Forwarded" header
  [Validator] Add object handling of invalid constraints in Composite
  [WebProfilerBundle] Remove uneeded directive in the form collector styles
  Revert "bug #21841 [Console] Do not squash input changes made from console.command event (chalasr)"
  [HttpFoundation] Fix Request::getHost() when having several hosts in X_FORWARDED_HOST
nicolas-grekas added a commit that referenced this pull request Mar 22, 2017
* 3.2:
  Fixed pathinfo calculation for requests starting with a question mark.
  [HttpFoundation] Fix missing handling of for/host/proto info from "Forwarded" header
  [Validator] Add object handling of invalid constraints in Composite
  [WebProfilerBundle] Remove uneeded directive in the form collector styles
  removed usage of $that
  HttpCache: New test for revalidating responses with an expired TTL
  [Serializer] [XML] Ignore Process Instruction
  [Security] simplify the SwitchUserListenerTest
  Revert "bug #21841 [Console] Do not squash input changes made from console.command event (chalasr)"
  [HttpFoundation] Fix Request::getHost() when having several hosts in X_FORWARDED_HOST
This was referenced Apr 5, 2017
ostrolucky pushed a commit to ostrolucky/symfony that referenced this pull request Mar 25, 2018
* 2.8:
  [HttpFoundation] Fix missing handling of for/host/proto info from "Forwarded" header
  [Validator] Add object handling of invalid constraints in Composite
  [WebProfilerBundle] Remove uneeded directive in the form collector styles
  Revert "bug symfony#21841 [Console] Do not squash input changes made from console.command event (chalasr)"
  [HttpFoundation] Fix Request::getHost() when having several hosts in X_FORWARDED_HOST
ostrolucky pushed a commit to ostrolucky/symfony that referenced this pull request Mar 25, 2018
* 3.2:
  Fixed pathinfo calculation for requests starting with a question mark.
  [HttpFoundation] Fix missing handling of for/host/proto info from "Forwarded" header
  [Validator] Add object handling of invalid constraints in Composite
  [WebProfilerBundle] Remove uneeded directive in the form collector styles
  removed usage of $that
  HttpCache: New test for revalidating responses with an expired TTL
  [Serializer] [XML] Ignore Process Instruction
  [Security] simplify the SwitchUserListenerTest
  Revert "bug symfony#21841 [Console] Do not squash input changes made from console.command event (chalasr)"
  [HttpFoundation] Fix Request::getHost() when having several hosts in X_FORWARDED_HOST
sandergo90 pushed a commit to sandergo90/symfony that referenced this pull request Jul 4, 2019
* 3.2:
  Fixed pathinfo calculation for requests starting with a question mark.
  [HttpFoundation] Fix missing handling of for/host/proto info from "Forwarded" header
  [Validator] Add object handling of invalid constraints in Composite
  [WebProfilerBundle] Remove uneeded directive in the form collector styles
  removed usage of $that
  HttpCache: New test for revalidating responses with an expired TTL
  [Serializer] [XML] Ignore Process Instruction
  [Security] simplify the SwitchUserListenerTest
  Revert "bug symfony#21841 [Console] Do not squash input changes made from console.command event (chalasr)"
  [HttpFoundation] Fix Request::getHost() when having several hosts in X_FORWARDED_HOST
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.

None yet

7 participants