Rewrite EventDispatcher introduction #6229
Conversation
| * Use only lowercase letters, numbers, dots (``.``) and underscores (``_``); | ||
| * Prefix names with a namespace followed by a dot (e.g. ``store.``); | ||
| * End names with a verb that indicates what action has been taken (e.g. | ||
| ``order_placed``). |
There was a problem hiding this comment.
It's often more common to write event names in the past tense. An event notifies the system that something had happend. However, this is against the Symfony core (which kinda misuses events at the moment). I would like what you think about this.
There was a problem hiding this comment.
I agree. That's how I usually name my events too.
There was a problem hiding this comment.
I like this list of proposals, but in my opinion it mixes mandatory things (characters which can be included in the event name) with recommendations. What about rewording it as follows?
Original:
The unique event name can be any string, but optionally follows a few simple
naming conventions:
* Use only lowercase letters, numbers, dots (``.``) and underscores (``_``);
* Prefix names with a namespace followed by a dot (e.g. ``store.``);
* End names with a verb that indicates what action has been taken (e.g.
``order_placed``).
Proposal:
The event name, which must be unique in the application, is a string which
can only contain letters, numbers, dots (``.``) and underscores (``_``).
Optionally, you can follow these naming conventions:
* Use only lowercase letters;
* Prefix names with a namespace followed by a dot (e.g. ``store.*``, ``user.*``);
* End names with a verb that indicates what action has been taken (e.g.
``order.placed``, ``user.created``).
There was a problem hiding this comment.
The string of the event name contain any character that can be added to a simple PHP string. So there is no mandatory thing in the list.
| ``Event`` object so that the listeners have the needed information. In such | ||
| case, a special subclass that has additional methods for retrieving and | ||
| overriding information can be passed when dispatching an event. For example, | ||
| the ``kernel.response`` uses a |
There was a problem hiding this comment.
[...] the kernel.response event uses [...]
|
I left some minor comments, but overall I love your changes. 👍 |
| have the same priority, they are executed in the order that they were | ||
| added to the dispatcher. | ||
| #. The event name (string) that this listener wants to listen to; | ||
| #. A PHP callable that will be notified when an event is thrown that it listens |
There was a problem hiding this comment.
I'd change this:
#. A PHP callable that will be notified when an event is thrown that it listens
to;
by this:
#. A PHP callable that will be executed when the listened event is triggered;
|
👍 I left some minor comments and suggestions, but overall this is great. @wouterj you did a great job here. Thanks! |
wouterj
force-pushed
the
component-eventdispatcher-remove-bundle
branch
from
df71946 to
e148cb4
Compare
|
Updated the PR |
|
👍 |
|
Thank you @wouterj. |
This PR was merged into the 2.3 branch. Discussion ---------- Rewrite EventDispatcher introduction | Q | A | --- | --- | Doc fix? | yes | New docs? | no | Applies to | 2.3+ | Fixed tickets | - The event dispatcher docs were a bit old. I discovered it talked about bundles (while it's the components section). While reviewing the article, I've also removed some redundant things (creating special sub event classes was mentioned many times). I've also tried to reword things a bit and introduce new (more commonly used) concepts for events. Commits ------- e148cb4 Rewrite EventDispatcher introduction
The event dispatcher docs were a bit old. I discovered it talked about bundles (while it's the components section).
While reviewing the article, I've also removed some redundant things (creating special sub event classes was mentioned many times). I've also tried to reword things a bit and introduce new (more commonly used) concepts for events.