+1

👍

I second this PR, very much needed.

What are the use-cases for this? The only one I saw was the swiftmailer
command to send emails from the spool, which currently fails when it
comes to memory spool, as it uses the kernel.terminate event.

Ideally this capability should be on top of ContainerAwareCommand not to
mess up the Application, here I've kind of blindly implemented what was
discussed on the original ticket, which was to add this on all commands
excepted cache:clear, but it just leads to a dirty hack imo.

Excepted commands one define himself, most of them are just isolated
tools for the application / developer, as such it looks weird to me to
know that potentially emails could be sent when I run any of them.

I'd suggest to have an EventDispatchingCommand extending
ContainerAwareCommand, and that's it. I'd also suggest to provide traits
support for this, which should be the best way to easily plug
capabilities to commands on the long-term whitout running into hacks.

What do you think?

I see another use case: setting the base url in the routing RequestContext so that generated urls are right.

and for Swiftmailer, it would send emails only if your command send an email. The MemorySpool will never send an email spooled in a different proces.

I don't like the idea of introducing a separate class for this. It would mean that every developer writing a command would have to decide if the command will dispatch events or no, making it confusing for people using the event ("Why don't I see the event for this command ?")

For the swiftmailer issue my fault indeed it's about sending emails from
commands with the memory setting, and for your other use case yes it's
interesting, while those use cases are very common (eg. newsletter), I
don't see why they should affect all other commands, people needing
those kind of feature could just extend a command on top of
ContainerAwareCommand (EventDispatchingCommand or even AbstractCommand
to allow new features later on),

I'm not against making all commands dispatching events in the absolute,
but unfortunately it complicates the code, remember it'd also forces us
to allow disabling it (cache clear command) or to somehow manually
disabling it for this command, while I could practically revert all the
changes of this PR if we choose my first suggestion and come up with a
single class addition.

my initial use case was a header I had to set on specific http connections that were triggered both on the web site as well as CLI.

I've just took time to look at it and I've been able to implement@stof suggestion without so much pain, so I'm fine with all commands dispatching events.

from my pov this PR is now finished.

@bamarni - I think you need to rebase this to current master as it cannot be automatically merged as it stands.

argh... it should be fixed now, thx

Is it too late for adding this to 2.2? As a reminder this in not only a new feature, but it'd provide a way to fix a bug, with the standard edition, by default, all mails sent from commands are lost (cf. symfony/symfony-standard#249).

cc@fabpot

ping

I think it would be nice to have it in 2.2 too.

+1000

+1

IMO the dialog helper should be passed through the event. I've an use case of a multitenant application that I need to ask the host before some commands are executed (like doctrine:*), in order to set the RequestContext.

+1 I was looking for such an implementation, it would be nice to have it in 2.2

@fabpot ping

I need it too

@fabpot ping

I've added the helperSet as per@marcospassos suggestion and also made the tests more robusts by checking both events are getting dispatched properly.

Up

@fabpot ping

there is no need to ping I guess :) 2.2 is in rc now, so it's obviously too late, and this is not the best moment for the core team to review 2.3 features.

Fair enough :)

i guess now its time to ping again :)

@fabpot ping ;)

closing in favor of#7466