Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[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

Closed
wants to merge 1 commit into from

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Mar 9, 2017

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

@@ -24,6 +27,11 @@ class ConsoleCommandEvent extends ConsoleEvent
const RETURN_CODE_DISABLED = 113;

/**
* @internal
Copy link
Member

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.

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. also made the decorator implements InputInterface to avoid BC break regarding the return type of getInput().

Copy link
Member Author

Choose a reason for hiding this comment

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

property+getter removed

@chalasr chalasr force-pushed the consoleevent-bc-break branch 2 times, most recently from 478bb3e to 9559f62 Compare March 9, 2017 19:47

if ('setOption' === $method) {
return $this->setOption($arguments[0], $arguments[1]);
}
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks

@chalasr chalasr force-pushed the consoleevent-bc-break branch 6 times, most recently from f066336 to b7c6012 Compare March 10, 2017 00:31
@nicolas-grekas nicolas-grekas added this to the 2.8 milestone Mar 10, 2017
@pierre-tranchard
Copy link

Hello,
When I apply the code to my Symfony 3.2.5 app, I get this message

PHP Fatal error:  Declaration of Symfony\Component\Console\Event\InputDecorator::hasParameterOption($values) must be compatible with Symfony\Component\Console\Input\InputInterface::hasParameterOption($values, $onlyParams = false) in /app/www/vendor/symfony/symfony/src/Symfony/Component/Console/Event/ConsoleCommandEvent.php on line 70

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?

@chalasr
Copy link
Member Author

chalasr commented Mar 10, 2017

@pierre-tranchard The signature of InputInterface::hasParameterOption() has changed in 3.0. Given this patch targets 2.8 and you're testing on 3.2, it's normal you get this error.
Here is the patch for 3.2: 3.2...chalasr:console-event-32 in case you can try it instead (which would be great), thank you anyway.

Why don't you create a dedicated file for this class?

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.

@pierre-tranchard
Copy link

@chalasr after a new test based on the modification, i still don't get the arguments and options

@chalasr
Copy link
Member Author

chalasr commented Mar 10, 2017

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?
The best would be to provide an simple script reproducing your use case like the one provided by @bendavies in #21841 (comment).

@pierre-tranchard
Copy link

pierre-tranchard commented Mar 10, 2017

Yes I removed the manual binding.

here's my delegated input class but I can't disclosed more details

class DelegatedInput extends ArrayInput
{

    /**
     * DelegatedInput constructor.
     *
     * @param InputInterface  $source
     * @param InputDefinition $definition
     * @param array           $ignoredAttrs
     * @param array           $ignoredOpts
     */
    public function __construct(
        InputInterface $source,
        InputDefinition $definition,
        $ignoredAttrs = [],
        $ignoredOpts = []
    ) {
        $source->bind($definition);
        $this->definition = $definition;

        foreach ($source->getArguments() as $name => $value) {
            if (!in_array($name, $ignoredAttrs)) {
                $this->setArgument($name, $value);
            }
        }
        foreach ($source->getOptions() as $name => $value) {
            if (!in_array($name, $ignoredOpts)) {
                $this->setOption($name, $value);
            }
        }
        parent::__construct([]);
    }

    /**
     * @return string
     */
    public function __toString()
    {
        $pattern = implode(' ', $this->getArguments());
        if (!empty($pattern)) {
            $pattern = sprintf(" %s", $pattern);
        }
        foreach ($this->getOptions() as $option => $value) {
            if (is_string($value)) {
                $pattern .= sprintf(" --%s=%s", $option, $value);
            } elseif (is_bool($value) && $value) {
                $pattern .= sprintf(" --%s", $option);
            }
        }

        return $pattern;
    }
}

When my supervisor command calls the supervised command, a delegated input instance is created and passed to the supervised command.
If i remove the bind instruction, I don't have the options and arguments from the supervisor command (except the command argument)

@chalasr
Copy link
Member Author

chalasr commented Mar 10, 2017

I'm going to write a script reproducing your use case using the custom input you given.

@chalasr
Copy link
Member Author

chalasr commented Mar 11, 2017

@pierre-tranchard I'm able to reproduce your issue: https://github.com/chalasr/console-bcbreak
I'm on it.

edit: I'm not, I can't make it work with 3.2.4 (before BC break).

@pierre-tranchard
Copy link

ok

@chalasr
Copy link
Member Author

chalasr commented Mar 13, 2017

Could you help to make it work on 3.2.4? I surely not reproduce your use case correctly.

@pierre-tranchard
Copy link

my fix didn't work in all case either.
I'll try to find you a case so that you can reproduce the fail

@pierre-tranchard
Copy link

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.

@chalasr chalasr force-pushed the consoleevent-bc-break branch from b7c6012 to 3d183c4 Compare March 14, 2017 08:59
@chalasr
Copy link
Member Author

chalasr commented Mar 14, 2017

@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.

@chalasr
Copy link
Member Author

chalasr commented Mar 15, 2017

ping @pierre-tranchard @bendavies
It would be nice to act on this for the next patch releases. Thank you.

@pierre-tranchard
Copy link

I'll have a look on it.

@pierre-tranchard
Copy link

@chalasr sorry but it still doesn't work for me.

I got to do the following in my base supervisor class

    /**
     * @inheritdoc
     */
    public function run(InputInterface $input, OutputInterface $output)
    {
       // several things
        $this->setInputBound(false);
        parent::run($input, $output);
    }

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
Copy link
Member

fabpot commented Mar 22, 2017

I've merged the PR that reverts the fix in the meantime.

@chalasr
Copy link
Member Author

chalasr commented Mar 22, 2017

Thanks, I will repropose the whole on master.

@chalasr chalasr closed this Mar 22, 2017
@chalasr chalasr deleted the consoleevent-bc-break branch March 23, 2017 23:11
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.

6 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.