Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Console] Do not squash input changes made from console.command event#21841
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| } | ||
| // don't bind the input again as it would override any input argument/option set from the command event in | ||
| // addition of being useless |
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 addition to being useless
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.
fixed
65fa1e6 tof363265Compare| return$output->fetch(); | ||
| } | ||
| publicfunctionsetInputBound($inputBound) |
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.
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? :)
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'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
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.
made it internal
f363265 toc8d364bComparefabpot commentedMar 5, 2017
Thank you@chalasr. |
…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/aSetting arguments/options from the `console.command` event is expected to work since#15938Commits-------c8d364b [Console] Do not squash input changes made from console.command event
bendavies commentedMar 9, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This breaks my project on upgrade to 3.2.5. This PR causes the input not to be rebound after the event, which will result in an error EDIT: |
chalasr commentedMar 9, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@bendavies Why do you add an argument from your event listener if you expect it to be removed afterwards? Isn't the |
bendavies commentedMar 9, 2017
I don't expect it to be removed afterwards. your use case is not the same as mine. Change the version of console to |
stof commentedMar 9, 2017
this PR indeed breaks things when the listener is changing the command definition rather than changing the input. |
chalasr commentedMar 9, 2017
@bendavies Thank you for the gist, that's clear now. I'm on it. @stof Resetting the flag to $event->getCommand()->addArgument('name', InputArgument::REQUIRED);$event->getInput()->setArgument('name','ben'); I'm not sure where the flag should be reset also |
chalasr commentedMar 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 |
chalasr commentedMar 9, 2017
I'll propose the solution of my previous comment today if I don't find better meanwhile. |
chalasr commentedMar 9, 2017
@bendavies could you please try#21953? |
pierre-tranchard commentedMar 9, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I encountered the same problem for a different case (same exception message@bendavies got). 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 commentedMar 9, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 commentedMar 9, 2017
i'll give a try. I'll give my feedback on your new PR |
chalasr commentedMar 9, 2017
That would be great, thank you |
…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/aA 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)"
* 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
* 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
* 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 "bugsymfony#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
* 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 "bugsymfony#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
Uh oh!
There was an error while loading.Please reload this page.
Setting arguments/options from the
console.commandevent is expected to work since#15938