Console dispatcher #7466

Merged
merged 3 commits into from Mar 25, 2013

Conversation

Projects
None yet
@fabpot
Member

fabpot commented Mar 24, 2013

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.

flevour and others added some commits Apr 11, 2012

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

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Mar 24, 2013

Member
Member

fabpot commented Mar 24, 2013

@fabpot fabpot referenced this pull request in symfony/symfony-docs Mar 24, 2013

Merged

added documentation for the dispatchable console application #2352

@dcsg

This comment has been minimized.

Show comment
Hide comment
@dcsg

dcsg Mar 24, 2013

Member

👍

Member

dcsg commented Mar 24, 2013

👍

@stloyd

View changes

src/Symfony/Component/Console/DispatchableApplication.php
+use Symfony\Component\EventDispatcher\EventDispatcher;
+
+/**
+ * Optionnally dispatches events during the life time of a command run.

This comment has been minimized.

@stloyd

stloyd Mar 24, 2013

Contributor

Typo: Optionally

@stloyd

stloyd Mar 24, 2013

Contributor

Typo: Optionally

@stloyd

View changes

src/Symfony/Component/Console/Event/ConsoleForExceptionEvent.php
+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ */
+class ConsoleForExceptionEvent extends ConsoleEvent

This comment has been minimized.

@stloyd

stloyd Mar 24, 2013

Contributor

Why not extend the ConsoleTerminateEvent class and reuse exitCode part?

@stloyd

stloyd Mar 24, 2013

Contributor

Why not extend the ConsoleTerminateEvent class and reuse exitCode part?

This comment has been minimized.

@fabpot

fabpot Mar 24, 2013

Member

Because of the setExitCode method that does not belong to this class.

@fabpot

fabpot Mar 24, 2013

Member

Because of the setExitCode method that does not belong to this class.

@Swop

This comment has been minimized.

Show comment
Hide comment
@Swop

Swop Mar 24, 2013

Yeah it would be awesome! 👍

Swop commented Mar 24, 2013

Yeah it would be awesome! 👍

@Koc

This comment has been minimized.

Show comment
Hide comment
@Koc

Koc Mar 24, 2013

Contributor

Will services tagged as "cache warmer" replaced with listeners for cache:clear command?

Contributor

Koc commented Mar 24, 2013

Will services tagged as "cache warmer" replaced with listeners for cache:clear command?

@beberlei

This comment has been minimized.

Show comment
Hide comment
@beberlei

beberlei Mar 24, 2013

Contributor

best way to introduce this feature by a longshot without changing existing code, +1 from me.

Contributor

beberlei commented Mar 24, 2013

best way to introduce this feature by a longshot without changing existing code, +1 from me.

+ $event = new ConsoleForExceptionEvent($command, $input, $output, $e, $event->getExitCode());
+ $this->dispatcher->dispatch(ConsoleEvents::EXCEPTION, $event);
+
+ throw $event->getException();

This comment has been minimized.

@lyrixx

lyrixx Mar 24, 2013

Member

It could be useful to be able to not throw the exception ; like in the HttpKernel.

Use case : you have a long running process like a rabbitmq consumer. With this feature, we could delegate the exception process into a dedicated event listener and make the "raw" command cleaner.

@lyrixx

lyrixx Mar 24, 2013

Member

It could be useful to be able to not throw the exception ; like in the HttpKernel.

Use case : you have a long running process like a rabbitmq consumer. With this feature, we could delegate the exception process into a dedicated event listener and make the "raw" command cleaner.

+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ */
+class DispatchableApplication extends Application

This comment has been minimized.

@schmittjoh

schmittjoh Mar 24, 2013

Contributor

DispatchingApplication or EventDispatchingApplication seem better names as the application itself is not dispatched, but is dispatching events.

In general, I think it would make more sense to use composition here and not inheritance to add the event dispatching behavior.

$app = new EventDispatchingApplication(new Application(), new EventDispatcher());

This also allows to add/compose other behaviors more easily.

@schmittjoh

schmittjoh Mar 24, 2013

Contributor

DispatchingApplication or EventDispatchingApplication seem better names as the application itself is not dispatched, but is dispatching events.

In general, I think it would make more sense to use composition here and not inheritance to add the event dispatching behavior.

$app = new EventDispatchingApplication(new Application(), new EventDispatcher());

This also allows to add/compose other behaviors more easily.

This comment has been minimized.

@fabpot

fabpot Mar 24, 2013

Member

I wanted to use composition at first, but as the Application class has so many public methods (about 30 IIRC), it would make the implementation very verbose.

@fabpot

fabpot Mar 24, 2013

Member

I wanted to use composition at first, but as the Application class has so many public methods (about 30 IIRC), it would make the implementation very verbose.

This comment has been minimized.

@schmittjoh

schmittjoh Mar 24, 2013

Contributor

Unfortunately, this will make something like the following very hard:
https://github.com/schmittjoh/JMSJobQueueBundle/blob/master/Console/Application.php

I understand that duplicating 30 methods is not exactly user-friendly/maintainable. Maybe this is a sign that we should refactor the Application class? We could keep the old one untouched and deprecate it, and then add a new one with a more focused interface.

@schmittjoh

schmittjoh Mar 24, 2013

Contributor

Unfortunately, this will make something like the following very hard:
https://github.com/schmittjoh/JMSJobQueueBundle/blob/master/Console/Application.php

I understand that duplicating 30 methods is not exactly user-friendly/maintainable. Maybe this is a sign that we should refactor the Application class? We could keep the old one untouched and deprecate it, and then add a new one with a more focused interface.

This comment has been minimized.

@Koc

Koc Mar 24, 2013

Contributor

Is this events not enough and you need anyway to create your own Application class?

@Koc

Koc Mar 24, 2013

Contributor

Is this events not enough and you need anyway to create your own Application class?

This comment has been minimized.

@fabpot

fabpot Mar 24, 2013

Member

I don't think this is a sign of anything and stability is more important here than anything else. As the new Applicaiton class is able to work with and without an event dispatcher, we can even merge everything back to the main Application class without any BC break. What do you/others think about this possibility?

@fabpot

fabpot Mar 24, 2013

Member

I don't think this is a sign of anything and stability is more important here than anything else. As the new Applicaiton class is able to work with and without an event dispatcher, we can even merge everything back to the main Application class without any BC break. What do you/others think about this possibility?

This comment has been minimized.

@schmittjoh

schmittjoh Mar 24, 2013

Contributor

Yeah, having just a single class seems better.

@schmittjoh

schmittjoh Mar 24, 2013

Contributor

Yeah, having just a single class seems better.

This comment has been minimized.

@fabpot

fabpot Mar 24, 2013

Member

@schmittjoh see f7336d0

@fabpot

fabpot Mar 24, 2013

Member

@schmittjoh see f7336d0

This comment has been minimized.

@schmittjoh

schmittjoh Mar 24, 2013

Contributor

Looks good, thanks.

On Sun, Mar 24, 2013 at 12:20 PM, Fabien Potencier <notifications@github.com

wrote:

In src/Symfony/Component/Console/DispatchableApplication.php:

+use Symfony\Component\Console\Input\InputInterface;
+use Symfony\Component\Console\Output\OutputInterface;
+use Symfony\Component\Console\Command\Command;
+use Symfony\Component\Console\ConsoleEvents;
+use Symfony\Component\Console\Event\ConsoleCommandEvent;
+use Symfony\Component\Console\Event\ConsoleTerminateEvent;
+use Symfony\Component\Console\Event\ConsoleForExceptionEvent;
+use Symfony\Component\EventDispatcher\EventDispatcher;
+
+/**

  • * Optionally dispatches events during the life time of a command run.
  • * @author Fabien Potencier fabien@symfony.com
  • */
    +class DispatchableApplication extends Application

@schmittjoh https://github.com/schmittjoh see f7336d0f7336d0


Reply to this email directly or view it on GitHubhttps://github.com/symfony/symfony/pull/7466/files#r3503492
.

@schmittjoh

schmittjoh Mar 24, 2013

Contributor

Looks good, thanks.

On Sun, Mar 24, 2013 at 12:20 PM, Fabien Potencier <notifications@github.com

wrote:

In src/Symfony/Component/Console/DispatchableApplication.php:

+use Symfony\Component\Console\Input\InputInterface;
+use Symfony\Component\Console\Output\OutputInterface;
+use Symfony\Component\Console\Command\Command;
+use Symfony\Component\Console\ConsoleEvents;
+use Symfony\Component\Console\Event\ConsoleCommandEvent;
+use Symfony\Component\Console\Event\ConsoleTerminateEvent;
+use Symfony\Component\Console\Event\ConsoleForExceptionEvent;
+use Symfony\Component\EventDispatcher\EventDispatcher;
+
+/**

  • * Optionally dispatches events during the life time of a command run.
  • * @author Fabien Potencier fabien@symfony.com
  • */
    +class DispatchableApplication extends Application

@schmittjoh https://github.com/schmittjoh see f7336d0f7336d0


Reply to this email directly or view it on GitHubhttps://github.com/symfony/symfony/pull/7466/files#r3503492
.

@beberlei

This comment has been minimized.

Show comment
Hide comment
@beberlei

beberlei Mar 24, 2013

Contributor

@Koc that doesn't make sense imho, Cache Warming should work independent of console listeners.

Contributor

beberlei commented Mar 24, 2013

@Koc that doesn't make sense imho, Cache Warming should work independent of console listeners.

@Koc

This comment has been minimized.

Show comment
Hide comment
@Koc

Koc Mar 24, 2013

Contributor

@fabpot can we get all output as a string in terminate event?

Contributor

Koc commented Mar 24, 2013

@fabpot can we get all output as a string in terminate event?

@XWB

This comment has been minimized.

Show comment
Hide comment
@XWB

XWB Mar 24, 2013

Contributor

👍

Contributor

XWB commented Mar 24, 2013

👍

@marcqualie

This comment has been minimized.

Show comment
Hide comment
@marcqualie

marcqualie Mar 24, 2013

👍 I've been working with a few Symfony Console apps lately and this functionality would be awesome!

👍 I've been working with a few Symfony Console apps lately and this functionality would be awesome!

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Mar 24, 2013

Member

@Koc: you can wrap/change the output instance in the command event, so that you can then get what was displayed in the terminate event. This PR gives you the flexibility, it's then up to you to use it the way you want.

Member

fabpot commented Mar 24, 2013

@Koc: you can wrap/change the output instance in the command event, so that you can then get what was displayed in the terminate event. This PR gives you the flexibility, it's then up to you to use it the way you want.

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Mar 24, 2013

Member

(I repost my comment, github hide it for now)

It could be useful to be able to not throw the exception in Application::doRunCommand ; like in the HttpKernel.

Use case : you have a long running process like a rabbitmq consumer. With this feature, we could delegate the exception process into a dedicated event listener and make the "raw" command cleaner.

Member

lyrixx commented Mar 24, 2013

(I repost my comment, github hide it for now)

It could be useful to be able to not throw the exception in Application::doRunCommand ; like in the HttpKernel.

Use case : you have a long running process like a rabbitmq consumer. With this feature, we could delegate the exception process into a dedicated event listener and make the "raw" command cleaner.

@bamarni

This comment has been minimized.

Show comment
Hide comment
@bamarni

bamarni Mar 24, 2013

Contributor

good to see this moving forward 👍

@stof had previously spotted a bug with this feature and the cache:clear command, see (#3889 (comment) for the beginning of the discussion), basically we came out to the conclusion that the cache:clear command shouldn't have this event dispatching capability, otherwise it would require some dirty hacks, do you think it will be easily doable when integrating this to the framework (as I can see this PR is only about the component part)?

Contributor

bamarni commented Mar 24, 2013

good to see this moving forward 👍

@stof had previously spotted a bug with this feature and the cache:clear command, see (#3889 (comment) for the beginning of the discussion), basically we came out to the conclusion that the cache:clear command shouldn't have this event dispatching capability, otherwise it would require some dirty hacks, do you think it will be easily doable when integrating this to the framework (as I can see this PR is only about the component part)?

+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ */
+class ConsoleForExceptionEvent extends ConsoleEvent

This comment has been minimized.

@bamarni

bamarni Mar 24, 2013

Contributor

Why "For"? "ConsoleExceptionEvent" seems more consistent with the other ones.

@bamarni

bamarni Mar 24, 2013

Contributor

Why "For"? "ConsoleExceptionEvent" seems more consistent with the other ones.

This comment has been minimized.

@Seldaek

Seldaek Mar 28, 2013

Member

@fabpot did you just miss this? I don't understand the class name either.

@Seldaek

Seldaek Mar 28, 2013

Member

@fabpot did you just miss this? I don't understand the class name either.

@bamarni

This comment has been minimized.

Show comment
Hide comment
@bamarni

bamarni Mar 24, 2013

Contributor

I'd also have a remark about the ability to change the command to be executed through the pre-event, do you have any use case in mind? Imo this doesn't make so much sense.

I can see it'd use the same input object, It means that the changed command should have the same arguments / options than the first one, or basically that it should be able to work with the same input, it looks like a pretty restricting condition.

It is also not always so easy to instantiate a command object, for example when using this feature with the framework, one would always need to have the container so that it can passes it to the command (as usually they are container aware), it looks like all this would be kind of a "sub-application".

Contributor

bamarni commented Mar 24, 2013

I'd also have a remark about the ability to change the command to be executed through the pre-event, do you have any use case in mind? Imo this doesn't make so much sense.

I can see it'd use the same input object, It means that the changed command should have the same arguments / options than the first one, or basically that it should be able to work with the same input, it looks like a pretty restricting condition.

It is also not always so easy to instantiate a command object, for example when using this feature with the framework, one would always need to have the container so that it can passes it to the command (as usually they are container aware), it looks like all this would be kind of a "sub-application".

@@ -80,6 +85,11 @@ public function __construct($name = 'UNKNOWN', $version = 'UNKNOWN')
}
}
+ public function setDispatcher(EventDispatcher $dispatcher)

This comment has been minimized.

@stof

stof Mar 24, 2013

Member

the typehint should be the interface

@stof

stof Mar 24, 2013

Member

the typehint should be the interface

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Mar 24, 2013

Member

@fabpot Would you use the same listener than for the kernel when integrating it in a Sf2 project ? Because in this case, we would face the bug I reported in the previous PR: if you deleted a subscriber class and the kernel is in non-debug mode, creating the event dispatcher object will break (in debug mode, the container would be refreshed without the subscriber). But the cache:clear command should be able to be used even when you have an outdated cache as its goal is to clear it.

Member

stof commented Mar 24, 2013

@fabpot Would you use the same listener than for the kernel when integrating it in a Sf2 project ? Because in this case, we would face the bug I reported in the previous PR: if you deleted a subscriber class and the kernel is in non-debug mode, creating the event dispatcher object will break (in debug mode, the container would be refreshed without the subscriber). But the cache:clear command should be able to be used even when you have an outdated cache as its goal is to clear it.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Mar 24, 2013

Member

otherwise, 👍 for this implementation.

Should the listener flushing the swiftmailer memory spool be added in the bridge in this PR or in a subsequent one ?

Member

stof commented Mar 24, 2013

otherwise, 👍 for this implementation.

Should the listener flushing the swiftmailer memory spool be added in the bridge in this PR or in a subsequent one ?

+use Symfony\Component\Console\Output\OutputInterface;
+
+/**
+ * Allows to do things before the command is executed or to change the command to execute altogether.

This comment has been minimized.

@Crell

Crell Mar 24, 2013

Contributor

This is not correct English grammar. I don't think the 3rd person verb fits here as a class description. You probably want something more like "Event object for pre-execution changes. \n\n Listeners may take actions before the command is executed, including changing the command to execute."

@Crell

Crell Mar 24, 2013

Contributor

This is not correct English grammar. I don't think the 3rd person verb fits here as a class description. You probably want something more like "Event object for pre-execution changes. \n\n Listeners may take actions before the command is executed, including changing the command to execute."

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Mar 25, 2013

Member

@bamarni You're right. I've removed the possibility to change the command in a console.command listener.

Member

fabpot commented Mar 25, 2013

@bamarni You're right. I've removed the possibility to change the command in a console.command listener.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Mar 25, 2013

Member

@lyrixx not sure how to implement that. Let's do that in another PR.

Member

fabpot commented Mar 25, 2013

@lyrixx not sure how to implement that. Let's do that in another PR.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Mar 25, 2013

Member

@stof @bamarni I'm aware of the possible problems with cache:clear. But we already have such issues today. When upgrading Symfony where a new parameter in the DIC is required without default value, you have the same problem. So, I propose to deal with that in another PR.

see #6519 for instance

Member

fabpot commented Mar 25, 2013

@stof @bamarni I'm aware of the possible problems with cache:clear. But we already have such issues today. When upgrading Symfony where a new parameter in the DIC is required without default value, you have the same problem. So, I propose to deal with that in another PR.

see #6519 for instance

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Mar 25, 2013

Member

@stof The Swiftmailer listener should be done in another PR, especially because it is not that easy (the best would be do be able to move the listener from the bundle to the bridge).

Member

fabpot commented Mar 25, 2013

@stof The Swiftmailer listener should be done in another PR, especially because it is not that easy (the best would be do be able to move the listener from the bundle to the bridge).

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

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

@fabpot fabpot merged commit 4f9a55a into symfony:master Mar 25, 2013

1 check failed

default Scrutinizer: 1 Comments, 0 Changed Files
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment