[FrameworkBundle] [Command] Event Dispatcher Debug - Display registered listeners #10388
Conversation
|
|
||
| /** | ||
| * @param $eventDispatcher | ||
| * @param null $event |
There was a problem hiding this comment.
is the event also a class or is only null?
There was a problem hiding this comment.
The event is null or a string (the name of the event)
There was a problem hiding this comment.
then it should be @param string|null $event The event name
There was a problem hiding this comment.
Right, thank you
|
Nice idea! |
| protected function configure() | ||
| { | ||
| $this | ||
| ->setName('event-dispatcher:debug') |
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
@fabpot what do you think about this naming ?
|
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 |
|
👍 |
|
@fabpot , could you give me your opinion about this ? |
|
👍 |
|
Can you rebase this on master as there are some conflicts? Thanks. |
|
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 event-dispatcher:debug event1 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; | ||
| } |
There was a problem hiding this comment.
What if you will not reach any of those ifs? Shouldn't you throw error or return empty array?
There was a problem hiding this comment.
Yes you're right, I should return $data, thanks
There was a problem hiding this comment.
Finally opted to raise an exception as if somedays it happens, we should know it instead of silently ignore it
|
@fabpot : Any chances for this to be shipped in 2.5 along with translation:debug and config:debug ? |
|
I like that kind of feature! |
|
I like it. 👍 btw: this PR is asking for the DX label (@javiereguiluz I saw you managing the other labels) |
|
@apfelbox applied the |
|
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. |
|
@stof I really like your idea on grouping all debug-related commands under |
|
@stof I really like your idea about grouping debug commands under
|
|
I like this command very much but I think the name is too long. Should we just keep |
|
@hhamon and we could even shorten it a bit more: |
|
I'm not sure the command should be named
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).
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"; |
|
@stof : PR updated, thanks for the review |
| } | ||
|
|
||
| /** | ||
| * @param callable $callable |
There was a problem hiding this comment.
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
…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
|
@weaverryan : The 2 points you mentioned earlier are now OK
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 |
|
👍 from me |
|
Awesome, thanks @stof |
| $this | ||
| ->setName('debug:event-dispatcher') | ||
| ->setDefinition(array( | ||
| new InputArgument('event', InputArgument::OPTIONAL, 'An event name (foo)'), |
There was a problem hiding this comment.
I'd remove (foo). It looks a bit like a default.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Oh, if it's there I'd keep it here too to be consistent.
|
Apart from a minor comment, big 👍 |
|
big 👍 it can be very helpful |
|
Thanks, we are almost there 🤘 ! |
|
👍 |
|
Thank you @matthieuauger. |
…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 179B 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  - *event-dispatcher:debug* **event** which displays configured listeners for this specific event (order by priority desc)  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
|
👍 |
|
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. |
|
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 :
Anyway, thanks for sharing, this debug:event-dispatcher command is very basic and can still be significantly improved ! |
|
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 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! |
[Update] The PR has been updated in order to comply with @stof comments.
Current status :
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
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.