[2.2] [FrameworkBundle] Add events to console commands #3889

Closed
wants to merge 15 commits into
from

Conversation

Projects
None yet
8 participants
Contributor

flevour commented Apr 11, 2012

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: Build Status

Fixes the following tickets: #1929
Todo:

  • tests needed?

@stof stof and 1 other commented on an outdated diff Apr 11, 2012

...ymfony/Bundle/FrameworkBundle/Console/Application.php
if (true === $input->hasParameterOption(array('--shell', '-s'))) {
$shell = new Shell($this);
$shell->setProcessIsolation($input->hasParameterOption(array('--process-isolation')));
$shell->run();
- return 0;
+ $statusCode = 0;
@stof

stof Apr 11, 2012

Member

the code when using the shell should not be changed. Events should be dispatched by the commands launched inside the shell, not before and after running the shell itself

@flevour

flevour Apr 11, 2012

Contributor

Alright, I was unsure about that point. Fixed.

@stof stof and 1 other commented on an outdated diff Apr 11, 2012

...ymfony/Bundle/FrameworkBundle/Console/Application.php
}
- return parent::doRun($input, $output);
+ $dispatcher->dispatch('console.init');
@stof

stof Apr 11, 2012

Member

init at the end ?

@flevour

flevour Apr 11, 2012

Contributor

Ooops. Fixed.

@stof stof and 2 others commented on an outdated diff Apr 11, 2012

...cyInjection/Compiler/RegisterConsoleListenersPass.php
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler;
+
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
+
+/**
+ * RegisterConsoleListenersPass process tags for console listeners.
+ *
+ * @author Francesco Levorato <git@flevour.net>
+ */
+class RegisterConsoleListenersPass implements CompilerPassInterface
@stof

stof Apr 11, 2012

Member

As you are using the event dispatcher (which was rejected by @fabpot in the discussion btw), this compiler pass is totally useless as it does exactly the same than the normal listener pass, and the normal listener pass is able to register listeners to these events

@flevour

flevour Apr 11, 2012

Contributor

The whole thing here was a bit confusing for me.
@fabpot rejected adding a dependency in the Console component, but since we are in FrameworkBundle, I thought this wouldn't be an issue.
On the other hand, the kind of use case proposed by @lsmith77 suggested to me the idea of something similar to an EventDispatcher.

 tags:
            -  { name: kernel.event_listener, event: kernel.request, method: onKernelRequest }
            -  { name: cli.event_listener, event: cli.init, method: onCliInit }

If this isn't the way, how would you go for it?

@lsmith77

lsmith77 Apr 16, 2012

Contributor

@fabpot rejected adding a dependency on the event dispatcher into the console component. but this PR doesn't do that. so it seems fine to me.

@stof stof commented on an outdated diff Apr 11, 2012

...ymfony/Bundle/FrameworkBundle/Console/Application.php
@@ -66,6 +66,7 @@ public function doRun(InputInterface $input, OutputInterface $output)
{
$this->registerCommands();
+
@stof

stof Apr 11, 2012

Member

this extra line should be removed

@stof stof and 1 other commented on an outdated diff Apr 11, 2012

...ymfony/Bundle/FrameworkBundle/Console/Application.php
@@ -74,7 +75,14 @@ public function doRun(InputInterface $input, OutputInterface $output)
return 0;
}
- return parent::doRun($input, $output);
+ $dispatcher = $this->kernel->getContainer()->get('event_dispatcher');
+ $dispatcher->dispatch('console.init');
@stof

stof Apr 11, 2012

Member

you should probably pass an event object giving access to the input and the output, to give a bit more power to the listeners (at least the input)

Contributor

flevour commented Apr 12, 2012

I am bit confused here. I await some helping pointers from any core dev.
EventDispatcher yes? If no, how to do this without recreating another EventDispatcher?

@lsmith77 lsmith77 commented on an outdated diff Apr 16, 2012

...ymfony/Bundle/FrameworkBundle/Console/Application.php
@@ -74,7 +75,14 @@ public function doRun(InputInterface $input, OutputInterface $output)
return 0;
}
- return parent::doRun($input, $output);
+ $dispatcher = $this->kernel->getContainer()->get('event_dispatcher');
+ $dispatcher->dispatch('console.init');
+
+ $statusCode = parent::doRun($input, $output);
@lsmith77

lsmith77 Apr 16, 2012

Contributor

this variable would better be named $exitCode

Contributor

lsmith77 commented Apr 16, 2012

i havent tested this branch, but in concept it seems good to me. please also add a test case. see RegisterKernelListenersPassTest

Member

stof commented Apr 16, 2012

@lsmith77 no need to add a test for the compiler pass as it should be removed (it duplicates things already possible with the existing pass for event listeners as it is exactly the same with a different tag)

Contributor

lsmith77 commented Apr 16, 2012

which "existing pass for event listeners" are you talking about?

Contributor

lsmith77 commented Apr 16, 2012

are you saying that RegisterKernelListenersPass should be refactored to also handle "console" events?

Member

stof commented Apr 16, 2012

@lsmith77 there is nothing to refactor. Simply register a listener with <tag name="kernel.event_listener" event="console.init" method="foo" />

Contributor

lsmith77 commented Apr 16, 2012

ah i see now .. since he is using $container->getDefinition('event_dispatcher'); instead of a separate event dispatcher (which would not make sense).

its maybe a bit iffy to call it kernel.event_listener now, but also not totally wrong as its just a result of the console being sort of a 2nd class citizen with our entire kernel setup.

Contributor

flevour commented Apr 16, 2012

Thanks for your feedback, I'll try to work on this PR in the next few days.

Contributor

flevour commented Apr 24, 2012

I applied the suggestions above.
Namely:

  • removed the CompilerPass
  • renamed $statusCode into $exitCode
  • implemented 2 Events (init and terminate) to let the programmer extend behaviour before and after command execution. I couldn't use 'ConsoleEvents::EXIT' as the 'exit' token is reserved.

@stof stof commented on an outdated diff Apr 24, 2012

...le/FrameworkBundle/Console/Event/ConsoleInitEvent.php
+
+namespace Symfony\Bundle\FrameworkBundle\Console\Event;
+
+use Symfony\Component\Console\Input\InputInterface;
+use Symfony\Component\Console\Output\OutputInterface;
+use Symfony\Component\HttpKernel\Event\KernelEvent;
+
+/**
+ * Allows to edit input and output of a command.
+ *
+ * Call setOutput if you need to modify the output object. The propagation of
+ * this event is stopped as soon as a new output is set.
+ *
+ * @author Francesco Levorato <git@flevour.net>
+ */
+class ConsoleInitEvent extends KernelEvent
@stof

stof Apr 24, 2012

Member

you should not extend KernelEvent as KernelEvent has the request and the request type which are totally irrelevant here

@stof stof commented on an outdated diff Apr 24, 2012

...le/FrameworkBundle/Console/Event/ConsoleInitEvent.php
+
+ /**
+ * The input received by the command.
+ *
+ * @var Symfony\Component\Console\Input\InputInterface
+ */
+ private $input;
+
+ /**
+ * The output object used by the command.
+ *
+ * @var Symfony\Component\Console\Output\OutputInterface
+ */
+ private $output;
+
+ function __construct(InputInterface $input, OutputInterface $output)
@stof

stof Apr 24, 2012

Member

missing public keyword

@stof stof commented on an outdated diff Apr 24, 2012

...ameworkBundle/Console/Event/ConsoleTerminateEvent.php
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\FrameworkBundle\Console\Event;
+
+/**
+ * Allows to receive the exit code of a command after its execution.
+ *
+ * @author Francesco Levorato <git@flevour.net>
+ */
+class ConsoleTerminateEvent extends KernelEvent
@stof

stof Apr 24, 2012

Member

this one should extend the ConsoleInitEvent to have the input and the output too

@stof stof commented on an outdated diff Apr 24, 2012

...ameworkBundle/Console/Event/ConsoleTerminateEvent.php
+/**
+ * Allows to receive the exit code of a command after its execution.
+ *
+ * @author Francesco Levorato <git@flevour.net>
+ */
+class ConsoleTerminateEvent extends KernelEvent
+{
+
+ /**
+ * The exit code of the command.
+ *
+ * @var integer
+ */
+ private $exitCode;
+
+ function __construct($exitCode)
@stof

stof Apr 24, 2012

Member

missing keyword

Contributor

flevour commented May 2, 2012

I probably did a bit of a mess with my last push. Do you have any more suggestions/comments to add?

@stof stof commented on an outdated diff May 2, 2012

...le/FrameworkBundle/Console/Event/ConsoleInitEvent.php
+
+/**
+ * Allows to edit input and output of a command.
+ *
+ * Call setOutput if you need to modify the output object. The propagation of
+ * this event is stopped as soon as a new output is set.
+ *
+ * @author Francesco Levorato <git@flevour.net>
+ */
+class ConsoleInitEvent extends Event
+{
+
+ /**
+ * The input received by the command.
+ *
+ * @var Symfony\Component\Console\Input\InputInterface
@stof

stof May 2, 2012

Member

you should use the short class name (the other solution would be the FQCN, i.e. with the leadaing \ but Sf2 uses the short classname everywhere). The current code will break the autocompletion is some IDE (PhpStorm would consider it as Symfony\Bundle\FrameworkBundle\Console\Event\Symfony\Component\Console\Input\InputInterface for instance)

@stof stof commented on an outdated diff May 2, 2012

...le/FrameworkBundle/Console/Event/ConsoleInitEvent.php
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\FrameworkBundle\Console\Event;
+
+use Symfony\Component\Console\Input\InputInterface;
+use Symfony\Component\Console\Output\OutputInterface;
+use Symfony\Component\EventDispatcher\Event;
+
+
+/**
+ * Allows to edit input and output of a command.
+ *
+ * Call setOutput if you need to modify the output object. The propagation of
+ * this event is stopped as soon as a new output is set.
@stof

stof May 2, 2012

Member

this comment is wrong. There is no reason to change the Output object as the commands are not about creating an Output object at all

@stof stof commented on an outdated diff May 2, 2012

...le/FrameworkBundle/Console/Event/ConsoleInitEvent.php
+ {
+ $this->input = $input;
+ $this->output = $output;
+ }
+
+ /**
+ * Sets an output object and stops event propagation
+ *
+ * @param Symfony\Component\Console\Output\OutputInterface $output
+ */
+ public function setOutput(OutputInterface $output)
+ {
+ $this->output = $output;
+
+ $this->stopPropagation();
+ }
@stof

stof May 2, 2012

Member

There is no reason to have a setter for them. Commands are not meant to change the Output object they receive

@stof

stof Jul 15, 2012

Member

however, this event could have a setExitCode method to allow returning early from the command

@stof stof commented on the diff Jul 15, 2012

...fony/Bundle/FrameworkBundle/Console/ConsoleEvents.php
+ * @var string
+ */
+ const INIT = 'console.init';
+
+
+ /**
+ * The TERMINATE event allows you to attach listeners after a command is
+ * executed by the console.
+ *
+ * The event listener method receives a Symfony\Bundle\FrameworkBundle\Console\Event\ConsoleTerminateEvent
+ * instance.
+ *
+ * @var string
+ */
+ const TERMINATE = 'console.terminate';
+
@stof

stof Jul 15, 2012

Member

extra line here

@stof stof commented on an outdated diff Jul 15, 2012

...le/FrameworkBundle/Console/Event/ConsoleInitEvent.php
+ */
+ public function getInput()
+ {
+ return $this->input;
+ }
+
+ /**
+ * Returns the output object
+ *
+ * @return Symfony\Component\Console\Output\OutputInterface
+ */
+ public function getOutput()
+ {
+ return $this->output;
+ }
+
@stof

stof Jul 15, 2012

Member

you have extra lines at the beginning and the end of the class

@stof stof commented on an outdated diff Jul 15, 2012

...le/FrameworkBundle/Console/Event/ConsoleInitEvent.php
+ */
+class ConsoleInitEvent extends Event
+{
+
+ /**
+ * The input received by the command.
+ *
+ * @var Symfony\Component\Console\Input\InputInterface
+ */
+ private $input;
+
+ /**
+ * The output object used by the command.
+ *
+ * @var Symfony\Component\Console\Output\OutputInterface
+ */
@stof

stof Jul 15, 2012

Member

the phpdoc should be removed from private properties

@stof stof commented on an outdated diff Jul 15, 2012

...le/FrameworkBundle/Console/Event/ConsoleInitEvent.php
+ * The output object used by the command.
+ *
+ * @var Symfony\Component\Console\Output\OutputInterface
+ */
+ private $output;
+
+ public function __construct(InputInterface $input, OutputInterface $output)
+ {
+ $this->input = $input;
+ $this->output = $output;
+ }
+
+ /**
+ * Sets an output object and stops event propagation
+ *
+ * @param Symfony\Component\Console\Output\OutputInterface $output
@stof

stof Jul 15, 2012

Member

this should use the short class name, relying on use statements (and it is wrong anyway currently as the leading \ is missing to make it a FQCN)

@stof stof commented on the diff Jul 15, 2012

...ameworkBundle/Console/Event/ConsoleTerminateEvent.php
+
+ public function __construct($exitCode)
+ {
+ $this->exitCode = $exitCode;
+ }
+
+ /**
+ * Returns the exit code.
+ *
+ * @return integer
+ */
+ public function getExitCode()
+ {
+ return $this->exitCode;
+ }
+
@stof

stof Jul 15, 2012

Member

this one should probably extend from the ConsoleInitEvent (which should then be renamed to ConsoleEvent) to give access to the input and output too

@stof stof commented on an outdated diff Jul 15, 2012

...ymfony/Bundle/FrameworkBundle/Console/Application.php
@@ -74,7 +77,17 @@ public function doRun(InputInterface $input, OutputInterface $output)
return 0;
}
- return parent::doRun($input, $output);
+ $dispatcher = $this->kernel->getContainer()->get('event_dispatcher');
+
+ $initEvent = new ConsoleInitEvent($input, $output);
+ $dispatcher->dispatch(ConsoleEvents::INIT, $initEvent);
+
+ $exitCode = parent::doRun($initEvent->getInput(), $initEvent->getOutput());
@stof

stof Jul 15, 2012

Member

you can use $input and $output directly as listener should not be able to replace them.

Contributor

flevour commented Aug 11, 2012

@stof: I should have taken care of all points you raised in the last batch of comments :)

This pull request fails (merged e9718bc6 into 31536c3).

This pull request passes (merged 79a1d496 into 31536c3).

@stof stof commented on an outdated diff Aug 20, 2012

...ameworkBundle/Console/Event/ConsoleTerminateEvent.php
+/**
+ * Allows to receive the exit code of a command after its execution.
+ *
+ * @author Francesco Levorato <git@flevour.net>
+ */
+class ConsoleTerminateEvent extends ConsoleEvent
+{
+
+ /**
+ * The exit code of the command.
+ *
+ * @var integer
+ */
+ private $exitCode;
+
+ public function __construct($exitCode)
@stof

stof Aug 20, 2012

Member

this is wrong. The constructor needs to receive the input and the output to call the parent constructor, otherwise getInput and getOuput will return null

stof referenced this pull request in symfony/symfony-standard Aug 20, 2012

Closed

Mails sent from commands are lost #249

Member

stof commented Aug 28, 2012

I added a similar feature in one of my projects, and I just found an issue which needs to resolved: when clearing the cache in non-debug mode, the cache:clear command is launched by a kernel using the old cache and so the old container. This breaks things when retrieving the event dispatcher if you deleted the class for a console listener or for an event subscriber (whatever events it listens too) as it will still try to use the old class (as the old container references it)

Member

stof commented Oct 13, 2012

@flevour any news on this ?

Contributor

flevour commented Oct 13, 2012

@stof thanks for the ping, this PR slipped off my radar. I'll be working on it.
How should I take your comment (#3889 (comment)) into account?

Member

stof commented Oct 13, 2012

@flevour You have to find a solution for this. I don't have one.
It works quite well in debug mode as the kernel will never instantiate an outdated container (it could still break if you remove the listener class without removing the service definition but it would explode in your face anymay in such case as the definition is wrong). But in non-debug mode, it breaks as the kernel created by the console is not updating the container yet.
My workaround is to make my deployment script delete the cached container file at the beginning of the deployment but it is not a good solution.

Member

Seldaek commented Oct 18, 2012

Dirty hack idea (because I enjoy that sport): before dispatching register a new autoloader that will create any class that does not exist with:

eval(<<<CODE
class $class
{
    public function __call($name, $args) 
    {
    }
}
CODE
);

That way services can be created and then called even if they don't exist anymore. It would mask any classname mistake though, so it should only be registered when the cache:clear command is called IMO.

Member

stof commented Oct 18, 2012

@Seldaek running any command in the prod environment with an outdated cache would also break things

Contributor

flevour commented Oct 18, 2012

I have given some thought but I still don't have any solution.
Let me recap.

When executed with --no-debug, the console application will use the currently cached kernel and container. If a listener/subscriber class gets removed (e.g.: in a deploy) the console application breaks with a fatal error, as it tries to load the event dispatcher from the cached container which in turn contains references to the not-existing-any-more class. In general all usages of the console application will break, but in particular it won't be possible to use it to clear the cache.

I can think of 3 approaches:
A. Preventive care: avoid edge cases aka fatal errors before they happen, take some countermeasures to avoid them
B. Cure the symptom: try to handle the problem when it occurs.

Here is a list of some proposals:
A

  1. implement a compiler pass to store any listener/subscriber class name in a container parameter as an array. The console application or the cache clear command, will loop through this parameter and check the existence of each element
  2. create a class ParanoidEventDispatcher which dynamically extends from %event_dispatcher.class% in order to add our own logic to the addListener/addSubscriber methods (note that it's not possible to wrap the EventDispatcher as the fatal error happens on its instantiation) and implement a compiler pass to inject the new ParanoidEventDispatcher in place of the original one.
  3. implement a new event dispatcher ParanoidEventDispatcher with the needed fixes in addListener/addSubscriber methods, make it available as a service and use it instead of the event_dispatcher service. Duplicating the logic contained in RegisterKernelListenersPass in order to attach to this new service the same listeners and subscribers of event_dispatcher.
  4. force a rm -rf before cache:clear execution
    B
  5. @Seldaek solution
  6. some creepy shutdown function to handle the fatal error and give hints to the user

I don't like any of these proposals, but if I really I had to choose, my order of choice would be A3, A1, A2.

Member

stof commented Oct 18, 2012

The cache:clear command cannot be responsible to check the instantiation of the dispatcher as the dispatcher is sued before calling the command.

Contributor

flevour commented Oct 19, 2012

@stof I assume you are commenting on A4. Yes, the code for rm -rf would need to be in the Application not in the CacheClearCommand itself.

Member

Seldaek commented Oct 19, 2012

IMO the only command that matters for the issue of "listener classes
gone" if cache:clear, if it's called maybe the events should just not be
triggered at all, or you should implement one of those hacks.

It's the same with the routing, annotations and whatever else is in the
cache, if you remove the class and you don't clear your cache in prod
environment you will have weird bugs where the framework goes further
than it should before blowing up on a missing class.

It is a predicate of the framework that if you change the code, you
should clear your prod cache before deploying, so IMO it's ok to have
that problem for other commands. But the cache:clear command must work
at all costs (including dropping listeners, maybe while outputting a
warning), because that's the way many people will use to clear/warmup
their cache.

Contributor

flevour commented Oct 30, 2012

One other (very crazy) idea could be:

A5: also cache (aka copy) the event listeners/subscribers classes and all their dependencies and configure the autoloader accordingly. Even if it's probably more hassle than it's worth, It would make some sense as a cache works only as long as all the classes it depends on still exist. The extreme result of this path is we end up caching the whole application, but if we apply this only in the case of cache:clear dependencies, it might be not as bad.

Going back to a more traditional thinking, A3 (implement a new dispatcher) might be the cleanest/easiest solution to offer console events and at the same provide useful output should something break during cache:clear.

Please share your opinions and suggestions so I can get this PR moving again!

Contributor

bamarni commented Oct 31, 2012

How about a flag on the command, just like the isEnabled method, there could be an isSilent method defaulting to false, and when set to true (eg. cache:clear command) the Application wouldn't dispatch any event?

Contributor

bamarni commented Nov 17, 2012

@flevour : I've continued your PR with this commit : bamarni/symfony@3072ee3

Are you fine with this implementation? I've not tested it yet but it should give you an idea about what I meant.

Unfortunately I can't submit a PR against your fork as github doesn't show it to me in the available symfony forks list. Maybe you can cherry-pick it?

Contributor

bamarni commented Nov 25, 2012

@flevour : did you had time to look at my commit? It should fix the issue, I guess now it only misses some small tweaks and unit tests.

If you don't have time to devote on this, I'd be happy to help you to sort this out by opening another PR, but of course I'll squash the commits with your name so that you keep the authorship for this feature!

Contributor

dlsniper commented Jan 24, 2013

@fabpot what about this? It's also needed for symfony/SwiftmailerBundle#23
Should this be rescheduled for 2.3?

Owner

fabpot commented Mar 23, 2013

Closing in favor of #6124

fabpot closed this Mar 23, 2013

@fabpot fabpot added a commit to fabpot/symfony that referenced this pull request Mar 24, 2013

@flevour @fabpot flevour + fabpot Added events for CLI commands
This adds an init and terminate event for commands. They are
dispatched from ContainerAwareCommand.

The cache:clear command can't implement this (cf. #3889 on Github).
f224102

fabpot referenced this pull request Mar 24, 2013

Merged

Console dispatcher #7466

@fabpot fabpot added a commit that referenced this pull request Mar 25, 2013

@fabpot fabpot merged branch fabpot/console-dispatcher (PR #7466)
This PR was merged into the master branch.

Discussion
----------

Console dispatcher

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #3889, #6124
| License       | MIT
| Doc PR        | symfony/symfony-docs#2352

refs #1884, #1929

This is an alternative implementation for adding events to console applications.

This implementation has the following features:

* Available for anyone using the Console component and it is not tied to
  FrameworkBundle (this is important as one thing we are trying to solve is
  email sending from a command, and frameworks like Silex using the Console
  component needs a solution too);

* Non-intrusive as the current code has not been changed (except for renaming
  an internal variable that was wrongly named -- so that's not strictly needed
  for this PR)

* The new DispatchableApplication class also works without a dispatcher,
  falling back to the regular behavior. That makes easy to create applications
  that can benefit from a dispatcher when available, but can still work
  otherwise.

* Besides the *before* and *after* events, there is also an *exception* event
  that is dispatched whenever an exception is thrown.

* Each event is quite powerful and can manipulate the input, the output, but
  also the command to be executed.

Commits
-------

4f9a55a refactored the implementation of how a console application can handle events
4edf29d added helperSet to console event objects
f224102 Added events for CLI commands
c1bd3b5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment