Skip to content

[FrameworkBundle] Added option to specify the event dispatcher in debug:event-dispatcher #39276

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 5, 2020
Merged

[FrameworkBundle] Added option to specify the event dispatcher in debug:event-dispatcher #39276

merged 1 commit into from
Dec 5, 2020

Conversation

TimoBakx
Copy link
Member

@TimoBakx TimoBakx commented Dec 1, 2020 •
edited
Loading

Q A
Branch? 5.x
Bug fix? no
New feature? yes
Deprecations? no
Tickets N/A
License MIT
Doc PR symfony/symfony-docs#14650
Tags #SymfonyHackday

Description:

In the recent security improvements, new event dispatchers were added (one for each firewall). These were not visible in the debug:event-dispatcher command. This PR adds an option to define a event dispatcher to show the events (and listeners) for that specific event dispatcher. Without any options, the default dispatcher is shown (identical to previous working)

In order to be able to fetch specific event dispatchers, I tagged all event dispatchers with a new kernel.event-dispatcher tag.

Usage:

bin/console debug:event-dispatcher --dispatcher=[servicename] [other options] [event]

Subtasks:

  • Add dispatcher option
  • Add kernel.event_dispatcher tag to all event dispatchers
  • Add autoconfigure tag for services implementing EventDispatcherInterface
  • Update Descriptor classes to specify the chosen dispatcher
  • Update changelog
  • Add documentation

@chalasr chalasr added this to the 5.x milestone Dec 2, 2020
@TimoBakx TimoBakx changed the title [Debug] Added option to specify the firewall in debug:event-dispatcher [Debug] Added option to specify the event dispatcher in debug:event-dispatcher Dec 3, 2020
@TimoBakx TimoBakx mentioned this pull request Dec 3, 2020
Merged
wouterj
wouterj reviewed Dec 5, 2020
View reviewed changes
Copy link
Member

@wouterj wouterj left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @TimoBakx! We completely forgot to update the debug and web profiler features for multiple event dispatchers.

I think this is a great feature. We can improve the DX later on by adding a debug:firewall command or the like (allowing users to specify the firewall name, rather than a weird security.event_dispatcher.<firewall name> string - #39321).

The only thing I'm a bit worried about is that the kernel.event_dispatcher tag is going to disrupt tag autocompletion for kernel.event_subscriber/kernel.event_listener tags (causing people to use the wrong tag). Can we maybe find a name that doesn't look similar to another tag?

@wouterj wouterj mentioned this pull request Dec 5, 2020
Closed
@TimoBakx
Copy link
Member Author

TimoBakx commented Dec 5, 2020

Hi @wouterj, that is a good point.

@weaverryan and I picked kernel.event_dispatcher for the consistency with other tags. My other ideas were event.dispatcher or event_dispatcher, but those seem a bit short and out of place.

kernel.dispatcher won't do due to the message bus also dispatching.

I'm a bit out of ideas on this one.

@derrabus
Copy link
Member

derrabus commented Dec 5, 2020

Would it make sense to enable auto-configuration for the new tag?

@TimoBakx
Copy link
Member Author

TimoBakx commented Dec 5, 2020

Would it make sense to enable auto-configuration for the new tag?

It is. See https://github.com/symfony/symfony/pull/39276/files#diff-e4f52d5033296421d02e360c8355aa1931bdfb0c12579f4815ea02b724322c0cR483

@derrabus
Copy link
Member

derrabus commented Dec 5, 2020

Oh right, sorry. 🙈

@wouterj
Copy link
Member

wouterj commented Dec 5, 2020

I'm a bit out of ideas on this one.

I'm also a bit of out of ideas. Maybe event_dispatcher.dispatcher? (to match the <component name>.<subject> convention)

@TimoBakx
Copy link
Member Author

TimoBakx commented Dec 5, 2020

Maybe event_dispatcher.dispatcher? (to match the <component name>.<subject> convention)

That sound good to me. I'll make the change.

@TimoBakx TimoBakx requested review from wouterj and xabbuh December 5, 2020 13:46
@carsonbot carsonbot changed the title [Debug] Added option to specify the event dispatcher in debug:event-dispatcher [FrameworkBundle] [Debug] Added option to specify the event dispatcher in debug:event-dispatcher Dec 5, 2020
@derrabus derrabus changed the title [FrameworkBundle] [Debug] Added option to specify the event dispatcher in debug:event-dispatcher [FrameworkBundle] Added option to specify the event dispatcher in debug:event-dispatcher Dec 5, 2020
@derrabus
Copy link
Member

derrabus commented Dec 5, 2020

Thank you @TimoBakx.

@derrabus derrabus merged commit 86842ac into symfony:5.x Dec 5, 2020
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Dec 7, 2020
@javiereguiluz
minor #14650 [Debug] Added option to specify the event dispatcher in …
b288550 
…debug:event-dispatcher (TimoBakx)

This PR was merged into the 5.3-dev branch.

Discussion
----------

[Debug] Added option to specify the event dispatcher in debug:event-dispatcher

Documentation for symfony/symfony#39276

Part of #SymfonyHackday

Commits
-------

9695d7e [Debug] Added option to specify the event dispatcher in debug:event-dispatcher
@TimoBakx TimoBakx deleted the debug-eventdispatcher-firewall branch December 7, 2020 09:34
@fabpot fabpot mentioned this pull request Apr 18, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wouterj wouterj wouterj approved these changes

@derrabus derrabus derrabus approved these changes

@chalasr chalasr Awaiting requested review from chalasr

@xabbuh xabbuh Awaiting requested review from xabbuh

Assignees
No one assigned
Labels
Feature FrameworkBundle Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
6 participants
@TimoBakx @derrabus @wouterj @xabbuh @chalasr @carsonbot