[Console] Fix bug with $output overloading #16621
Conversation
|
Btw, not sure where the error is coming from as SymfonyStyle implements OutputInterface and the isDebug method, but this is at least more correct than before. |
|
Awesome. This fix works for me. No error so far. Should I close the issue 16622 by myself? |
|
@intermike good to hear that :) I've added your issue to the PR table in the description. When this PR is merged, the issue will be closed automatically. Thanks for reporting this to Symfony! |
| @@ -72,9 +72,9 @@ protected function configure() | |||
| /** | |||
| * {@inheritdoc} | |||
| */ | |||
| protected function execute(InputInterface $input, OutputInterface $output) | |||
| protected function execute(InputInterface $input, OutputInterface $cliOutput) | |||
There was a problem hiding this comment.
This is not named according to the parent definition. Also inheritdoc will not work this way.
There was a problem hiding this comment.
Yea, and it makes more sense to me to do the opposite: use OutputInterface $output here, then rename the other variable, like $styleOutput = new SymfonyStyle(...). That's mostly because we tend to always call the argument $output.
There was a problem hiding this comment.
@weaverryan do you have some ideas for #16000? Thanks!
@wouterj : It only does in 3.0, not in 2.8 actually. Because it's only part of the |
|
👍 to add them in 2.7. I consider it a bug that it isn't |
|
It would be kind of ugly however, as it will require checking if each method exists in the provided # OutputStyle.php
public function isDebug()
{
if (!method_exists($this->output, 'isDebug')) {
throw new \BadMethodCallException(sprintf(
'Method "%s" of "%s" does not exist',
get_class($this->output),
__METHOD__
));
}
return $this->output->isDebug();
}Also we wouldn't be able to base ourself on this API for solving this issue depending on the output instance used in the commands :/ |
|
Status: Needs work |
|
Changed it to set |
| @@ -74,7 +74,7 @@ protected function configure() | |||
| */ | |||
| protected function execute(InputInterface $input, OutputInterface $output) | |||
| { | |||
| $output = new SymfonyStyle($input, $output); | |||
| $output = new SymfonyStyle($input, $cliOutput = $output); | |||
There was a problem hiding this comment.
I don't really care, but this may not be super clear to people reading the code - maybe we can set it in its own line
|
👍 The naming - and details in this PR - are all internal things anyways that can be changed later. |
|
Thank you @wouterj. |
This PR was squashed before being merged into the 2.8 branch (closes #16621). Discussion ---------- [Console] Fix bug with $output overloading | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | https://github.com/symfony/symfony-standard/issues/887, #16622 | License | MIT | Doc PR | - This is exactly why variable overloading isn't a great idea :) Commits ------- 5764a00 [Console] Fix bug with overloading
This is exactly why variable overloading isn't a great idea :)