Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.3k
Rewrite EventDispatcher introduction#6229
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Rewrite EventDispatcher introduction#6229
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree. That's how I usually name my events too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 simplenaming 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 whichcan 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[...] thekernel.response event uses [...]
xabbuh commentedFeb 9, 2016
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd change this:
#. A PHP callable that will be notified when an event is thrown that it listensto;by this:
#. A PHP callable that will be executed when the listened event is triggered;javiereguiluz commentedFeb 12, 2016
👍 I left some minor comments and suggestions, but overall this is great. @wouterj you did a great job here. Thanks! |
df71946 toe148cb4Comparewouterj commentedFeb 13, 2016
Updated the PR |
xabbuh commentedFeb 14, 2016
👍 |
xabbuh commentedFeb 15, 2016
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.