-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Console] Fix BC break regarding console.command event #21953
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
Conversation
98b0d87
to
c3ac277
Compare
@@ -24,6 +27,11 @@ class ConsoleCommandEvent extends ConsoleEvent | ||
const RETURN_CODE_DISABLED = 113; | ||
|
||
/** | ||
* @internal |
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.
no need to mark a private property as being internal.
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. also made the decorator implements InputInterface
to avoid BC break regarding the return type of getInput()
.
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.
property+getter removed
478bb3e
to
9559f62
Compare
|
||
if ('setOption' === $method) { | ||
return $this->setOption($arguments[0], $arguments[1]); | ||
} |
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.
These are not neeeded as __call
isn't called on defined methods. Btw, not sure if this __call()
function is needed at all, given all input interface methods are implemented directly.
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.
thanks
f066336
to
b7c6012
Compare
Hello,
I use PHP 7.1. I tested it for this issue #21841 (comment) By the way, I don't get why you set a final class in the console event file? Why don't you create a dedicated file for this class? |
@pierre-tranchard The signature of
That's because we only need this class for this event, nobody should use it directly, nor other place in the core (like this you don't get this file listed in your IDE). Others may think differently but IMHO it's not worth having a dedicated file for this class. |
@chalasr after a new test based on the modification, i still don't get the arguments and options |
Well, the patch comes with a test case proving that arguments/options added to the definition through the event are properly considered when running the command. Did you removed the manual binding added to workaround the BC break? |
Yes I removed the manual binding. here's my delegated input class but I can't disclosed more details
When my supervisor command calls the supervised command, a delegated input instance is created and passed to the supervised command. |
I'm going to write a script reproducing your use case using the custom input you given. |
@pierre-tranchard I'm able to reproduce your issue: https://github.com/chalasr/console-bcbreak edit: I'm not, I can't make it work with 3.2.4 (before BC break). |
ok |
Could you help to make it work on 3.2.4? I surely not reproduce your use case correctly. |
my fix didn't work in all case either. |
to fix my bug, I had to remove the bind instruction from my delegated input class and I had to set the input bound to false in my supervisor base class run method. |
b7c6012
to
3d183c4
Compare
@pierre-tranchard I have no idea of what your code looks like but anyway, if your original code is still broken after this patch, I think the first fix should be reverted from maintenance branches and re-discussed on master, including this. Last chance before doing so, could you please try 3.2...chalasr:console-event-32 on your original code? I updated it and this to mark the input as bound only if arguments/options values are set from the command event, it should be ok. |
ping @pierre-tranchard @bendavies |
I'll have a look on it. |
@chalasr sorry but it still doesn't work for me. I got to do the following in my base supervisor class
|
…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)"
I've merged the PR that reverts the fix in the meantime. |
Thanks, I will repropose the whole on master. |
Uh oh!
There was an error while loading. Please reload this page.