Skip to content
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

[FrameworkBundle] [Command] Event Dispatcher Debug - Display registered listeners #10388

Conversation

Projects
None yet
@matthieuauger
Copy link
Contributor

commented Mar 5, 2014

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT

[Update] The PR has been updated in order to comply with @stof comments.

Current status :

  • New event dispatcher Descriptor
  • Manage all callables
  • Unit tests
  • Text description
  • XML description
  • Json description
  • Markdown description

Hi. In some big applications with lots of events, it's often hard to debug which classes listen to which events, and what is the order of theses listeners. This PR allows to run

  • event-dispatcher:debug which displays all configured listeners + the events they listen to

capture d cran de 2014-03-07 20 13 56

  • event-dispatcher:debug event which displays configured listeners for this specific event (order by priority desc)

capture d cran de 2014-03-07 20 14 31

The output is similar to container:debug command and is available in all supported formats (txt, xml, json and markdown).

I found another PR with same goal (#8234), but the approach looks too complicated to me plus I think we should fetch the listeners directly with the event_dispatcher.

/**
* @param $eventDispatcher
* @param null $event

This comment has been minimized.

Copy link
@cordoval

cordoval Mar 5, 2014

Contributor

is the event also a class or is only null?

This comment has been minimized.

Copy link
@matthieuauger

matthieuauger Mar 5, 2014

Author Contributor

The event is null or a string (the name of the event)

This comment has been minimized.

Copy link
@romainneutron

romainneutron Apr 9, 2014

Member

then it should be @param string|null $event The event name

This comment has been minimized.

Copy link
@matthieuauger

matthieuauger Apr 9, 2014

Author Contributor

Right, thank you

@saro0h

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2014

Nice idea!

protected function configure()
{
$this
->setName('event-dispatcher:debug')

This comment has been minimized.

Copy link
@stof

stof Mar 6, 2014

Member

@fabpot what about grouping the debug command in a debug: namespace rather than having lots of namespaces with a single debug command in it ? We can use aliases for BC for existing commands

This comment has been minimized.

Copy link
@hhamon

hhamon Mar 22, 2014

Contributor

+1

This comment has been minimized.

Copy link
@stof

stof Jun 18, 2014

Member

@fabpot what do you think about this naming ?

This comment has been minimized.

Copy link
@weaverryan
$groupedListeners = array();
foreach ($eventDispatcher->getListeners() as $event => $eventListeners) {
foreach ($eventListeners as $eventListener) {
list($object, $method) = $eventListener;

This comment has been minimized.

Copy link
@stof

stof Mar 6, 2014

Member

what if the event listener is not build as array($object, $method) ? Not all PHP callables are build this way

This comment has been minimized.

Copy link
@matthieuauger

matthieuauger Mar 6, 2014

Author Contributor

Thanks @stof you're right. I will update this to manage all callable types, probably by adding a describeCallable() to the base Descriptor class

if (null !== $event) {
foreach ($eventDispatcher->getListeners($event) as $listener) {
list($object, $method) = $listener;
$className = get_class($object);

This comment has been minimized.

Copy link
@stof

stof Mar 6, 2014

Member

This logic is broken when listeners are not array($object, $method) callables

if (null !== $event) {
foreach ($eventDispatcher->getListeners($event) as $listener) {
list($object, $method) = $listener;
$className = get_class($object);

This comment has been minimized.

Copy link
@stof

stof Mar 6, 2014

Member

This logic is broken when listeners are not array($object, $method) callables


foreach ($eventDispatcher->getListeners($event) as $listener) {
list($object, $method) = $listener;
$className = get_class($object);

This comment has been minimized.

Copy link
@stof

stof Mar 6, 2014

Member

This logic is broken when listeners are not array($object, $method) callables

if (null !== $event) {
foreach ($eventDispatcher->getListeners($event) as $listener) {
list($object, $method) = $listener;
$className = get_class($object);

This comment has been minimized.

Copy link
@stof

stof Mar 6, 2014

Member

This logic is broken when listeners are not array($object, $method) callables

@matthieuauger

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2014

The PR has been updated. The descriptor now manages all known callables (found 7 different types) and unit tests have been added.

Thanks for the feedback

@elnur

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2014

👍

@matthieuauger

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2014

@fabpot , could you give me your opinion about this ?

@Miliooo

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2014

👍

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 28, 2014

Can you rebase this on master as there are some conflicts? Thanks.

@matthieuauger

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2014

The PR is finished, all formats are now supported and all descriptors tested.

You can see the output in the test files :

event-dispatcher:debug
Text
Xml
Json
Markdown

event-dispatcher:debug event1
Text
Xml
Json
Markdown

The diff is quite big due to all the formats but the functionnality is now fully working.

Feedbacks always welcomed

$data['name'] = get_class($callable);
return $data;
}

This comment has been minimized.

Copy link
@stloyd

stloyd Apr 10, 2014

Contributor

What if you will not reach any of those ifs? Shouldn't you throw error or return empty array?

This comment has been minimized.

Copy link
@matthieuauger

matthieuauger Apr 10, 2014

Author Contributor

Yes you're right, I should return $data, thanks

This comment has been minimized.

Copy link
@matthieuauger

matthieuauger Apr 10, 2014

Author Contributor

Finally opted to raise an exception as if somedays it happens, we should know it instead of silently ignore it

$string = '';
if (is_array($callable)) {
$string .= "\n".'- Type: `function`';

This comment has been minimized.

Copy link
@stloyd

stloyd Apr 10, 2014

Contributor

Instead of merging two strings, you can simply re-use that already opened one.

This comment has been minimized.

Copy link
@matthieuauger

matthieuauger Apr 10, 2014

Author Contributor

Not sure to understand what you mean. Are you suggesting this ?
$string .= "\n- Type: function";

This comment has been minimized.

Copy link
@stloyd

stloyd Apr 10, 2014

Contributor

Yes, it's exactly same approach you do few lines below =)

This comment has been minimized.

Copy link
@matthieuauger

matthieuauger Apr 10, 2014

Author Contributor

Ok, did it when the following string is static but not into the sprintf() statements, I think it's more readable to have the \n aligned vertically (and generally prefer single-quoted strings).

Thanks for your feedback !

@matthieuauger

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2014

@fabpot : Any chances for this to be shipped in 2.5 along with translation:debug and config:debug ?

@matthieuauger

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2014

This is visibly not going to be shipped in 2.5 due to feature freeze, but it would still be much appreciated to have feedback of the core team decision makers @jakzal @stof @Seldaek @lsmith77

@jeremyFreeAgent

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2014

I like that kind of feature!

@apfelbox

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2014

I like it. 👍

btw: this PR is asking for the DX label (@javiereguiluz I saw you managing the other labels)

@javiereguiluz javiereguiluz added the DX label Jun 18, 2014

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Jun 18, 2014

@apfelbox applied the DX label. Thanks for noticing it.

@stof

This comment has been minimized.

Copy link
Member

commented Jun 18, 2014

I like the feature, however I think discussing the naming of the command is important for DX (see #10388 (comment)) so I'm waiting for this discussion to complete before voting on the PR.

@apfelbox

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2014

@stof I really like your idea on grouping all debug-related commands under debug:, btw.

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Jun 18, 2014

@stof I really like your idea about grouping debug commands under debug namespace. I'd love to do something similar with the check namespace. That's why I proposed to create a check:security command and not security:check. We could create the following:

  • check:security looks for known security vulnerabilities
  • check:permissions checks if the project folders have the right permissions to execute Symfony
  • check:yaml would deprecate yaml:lint
  • check:twig would deprecate twig:lint
  • check:doctrine-settings would deprecate doctrine:ensure-production-settings
  • check:configuration would check if there is any problem in all your project configuration files
  • etc.
@hhamon

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2014

I like this command very much but I think the name is too long. Should we just keep dispatcher:debug instead of event-dispatcher:debug?

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Jun 18, 2014

@hhamon and we could even shorten it a bit more: debug:events or debug:listeners

@matthieuauger

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2014

I'm not sure the command should be named debug:something. Actually, if we tried to establish a name convention from the existing commands, i would say the pattern is

vendor:object:action

  • doctrine:database:create
  • doctrine:mapping:convert
  • swiftmailer: email:send

Plus an exception for the framework-related commands where the vendor is omitted for simplicity i guess (but the action is almost everytime at the end).

  • cache:clear
  • assetic:dump
  • router:match

The advantage of this pattern is that we follow the general idea of namespaces where the vendor is first.

Grouping all the debug commands in a debug namespace is really convenient, but shouldn't it be the alias instead of the command name ?

if (false === strpos($callable, '::')) {
$string .= "\n".sprintf('- Name: `%s`', $callable);
$string .= "\n- Global: yes";

This comment has been minimized.

Copy link
@stof

stof Jul 16, 2014

Member

I would remove it here as well

/**
* @param EventDispatcherInterface $eventDispatcher
* @param null $event

This comment has been minimized.

Copy link
@stof

stof Jul 16, 2014

Member

string|null

private function getEventDispatcherListenersDocument(EventDispatcherInterface $eventDispatcher, $event = null)
{
$dom = new \DOMDocument('1.0', 'UTF-8');
$dom->appendChild($eventDispatcherXML = $dom->createElement('event_dispatcher'));

This comment has been minimized.

Copy link
@stof

stof Jul 16, 2014

Member

I would use event-dispatcher for the node name to follow the XML conventions

$registeredListeners = $eventDispatcher->getListeners($event);
if (null !== $event) {
foreach ($registeredListeners as $order => $listener) {

This comment has been minimized.

Copy link
@stof

stof Jul 16, 2014

Member

$order is unused

if (false === strpos($callable, '::')) {
$callableXML->setAttribute('name', $callable);
$callableXML->setAttribute('global', 'true');

This comment has been minimized.

Copy link
@stof

stof Jul 16, 2014

Member

same here about global

@matthieuauger

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2014

@stof : PR updated, thanks for the review

}
/**
* @param callable $callable

This comment has been minimized.

Copy link
@stof

stof Aug 13, 2014

Member

missing @return \DOMDocument

This comment has been minimized.

Copy link
@matthieuauger

matthieuauger Aug 14, 2014

Author Contributor

Done. I rechecked all the PHPDoc to ensure that nothing is missing. I just didn't add the "@throws \InvalidArgumentException" on the describeCallable function because developers should not have to handle this exception but I can add it if you think it's necessary

/**
* @param callable $callable
*/
protected function getCallableDocument($callable)

This comment has been minimized.

Copy link
@stof

stof Aug 13, 2014

Member

this should be private

This comment has been minimized.

Copy link
@stof

stof Aug 13, 2014

Member

btw, instead of creating a DOMDocument with a single child which is then imported in another document, I would rather pass the DOMDocument as argument to use it to create a DOMElement (your $callableXML variable) and retrun the DOMElement itself. This way, you would not need to import it again in the right document when using it.

This comment has been minimized.

Copy link
@matthieuauger

matthieuauger Aug 14, 2014

Author Contributor

In fact this is the same logic applied for the router and container descriptions (https://github.com/matthieuauger/symfony/blob/feature-framework-bundle-event-dispatcher-command/src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/XmlDescriptor.php#L144). We should keep consistency between theses functions, don't you think ?

Another thing is that you can describe a callable alone (https://github.com/matthieuauger/symfony/blob/feature-framework-bundle-event-dispatcher-command/src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/Descriptor.php#L73). In this case the creation of the DOMDocument here seems pertinent

This comment has been minimized.

Copy link
@stof

stof Aug 14, 2014

Member

ah, I missed that it can be describe standalone. Forget these comments then

nicolas-grekas added a commit that referenced this pull request Aug 13, 2014

minor #11627 [FrameworkBundle] [TwigBundle] Move debug commands to de…
…bug namespace (matthieuauger)

This PR was merged into the 2.6-dev branch.

Discussion
----------

[FrameworkBundle] [TwigBundle] Move debug commands to debug namespace

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT

Instead of having several namespaces with only one debug command (container:debug, event-dispatcher:debug), move all these debug commands to a new **debug** namespace.

Related to #10388 (comment)

I don't how to tag these aliases as deprecated as there are only here for backward compatibility.
The renaming should also be done in the Swiftmailer Bundle.

Commits
-------

fd0e229 Move debug commands to debug namespace
@matthieuauger

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2014

@weaverryan : The 2 points you mentioned earlier are now OK

Action items:

A) Rename the command to debug:event-dispatcher
B) On a separate PR, move existing debug commands into the debug:* namespace and give them aliases for BC

The A) is updated here and B) has been merged to master a week ago. The only missing point here is 👍 or new feedback from the core team decision makers

@stof

This comment has been minimized.

Copy link
Member

commented Aug 20, 2014

👍 from me

@matthieuauger

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2014

Awesome, thanks @stof

@matthieuauger

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2014

ping @symfony/deciders ( @jakzal @Seldaek @lsmith77 )

$this
->setName('debug:event-dispatcher')
->setDefinition(array(
new InputArgument('event', InputArgument::OPTIONAL, 'An event name (foo)'),

This comment has been minimized.

Copy link
@jakzal

jakzal Sep 12, 2014

Member

I'd remove (foo). It looks a bit like a default.

This comment has been minimized.

Copy link
@matthieuauger

matthieuauger Sep 13, 2014

Author Contributor

Thanks for your feedback, I copied that from the debug:container command (https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Command/ContainerDebugCommand.php#L46). I should be able to remove it by tomorrow

This comment has been minimized.

Copy link
@jakzal

jakzal Sep 14, 2014

Member

Oh, if it's there I'd keep it here too to be consistent.

@jakzal

This comment has been minimized.

Copy link
Member

commented Sep 12, 2014

Apart from a minor comment, big 👍

@Nicofuma

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2014

big 👍 it can be very helpful

@matthieuauger

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2014

Thanks, we are almost there 🤘 !

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 15, 2014

👍

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 15, 2014

Thank you @matthieuauger.

@fabpot fabpot merged commit ce53c8a into symfony:master Sep 15, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

fabpot added a commit that referenced this pull request Sep 15, 2014

feature #10388 [FrameworkBundle] [Command] Event Dispatcher Debug - D…
…isplay registered listeners (matthieuauger)

This PR was merged into the 2.6-dev branch.

Discussion
----------

[FrameworkBundle] [Command] Event Dispatcher Debug - Display registered listeners

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT

------------------------------------------
[Update] The PR has been updated in order to comply with @stof comments.

Current status :
- [x] New event dispatcher Descriptor
- [x] Manage all callables
- [x] Unit tests
- [x] Text description
- [x] XML description
- [x] Json description
- [x] Markdown description

-----------------------------------------
Hi. In some big applications with lots of events, it's often hard to debug which classes listen to which events, and what is the order of theses listeners. This PR allows to run

- *event-dispatcher:debug* which displays all configured listeners + the events they listen to

![capture d cran de 2014-03-07 20 13 56](https://f.cloud.github.com/assets/1172099/2361104/40a86a62-a62d-11e3-9ccd-360a8d75b2a4.png)

- *event-dispatcher:debug* **event** which displays configured listeners for this specific event (order by priority desc)

![capture d cran de 2014-03-07 20 14 31](https://f.cloud.github.com/assets/1172099/2361100/31e0d12c-a62d-11e3-963b-87623d05642c.png)

The output is similar to *container:debug* command and is available in all supported formats (txt, xml, json and markdown).

I found another PR with same goal (#8234), but the approach looks too complicated to me plus I think we should fetch the listeners directly with the event_dispatcher.

Commits
-------

ce53c8a [FrameworkBundle] Add Event Dispatcher debug command

@matthieuauger matthieuauger deleted the matthieuauger:feature-framework-bundle-event-dispatcher-command branch Sep 15, 2014

@ErikSaunier

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2014

👍

@egulias

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2014

Maybe I'm a bit later to this, but you may know https://github.com/egulias/ListenersDebugCommandBundle which is a command that does exactly this since 2012, and has been added as dependency of ez Publish. It allows to order on priority, filter by event name and gather detailed information to display into the console.
I have released https://github.com/egulias/TagDebugCommandBundle (and its lib) which adds flexibility when inspecting tags in the container.

@matthieuauger

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2014

Hi @egulias ! IMO this feature has place in core, you could have open a PR for that 2 years ago !

I think we implemented this in two different ways :

  • Your command is deeply coupled with the Symfony DIC (definitions, alias, tags, visibility). The EventDispatcher is a standalone component and we should be able to describe listeners just with an EventDispatcher object (without knowledge of the DIC behind)
  • You fetch the listeners by filtering the services tagged with kernel.event_listener tag, but what if listeners are not added within the DI ? For example in a CompilerPass ?

Anyway, thanks for sharing, this debug:event-dispatcher command is very basic and can still be significantly improved !

@egulias

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2014

Hi @matthieuauger yes, probably I should have done that :).

Yes, we did and I can't remember why I went the DIC way.

For the record, the tag name inspected is a regex for any tag name ending with .event_listener or event_subscriber.

This command won't see doctrine's listeners since they use their own event dispatcher, right? I think is the expected behaviour.

May be I can PR with the addition of the DIC to the events list, now that the logic is in a lib of its own.

In any case, nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.