Problem/Motivation
It's near impossible to find what events exist.
Also, events have "priority" which behave the exact opposite as Drupal weights do: the higher the event priority, the sooner it fires. Drupal module weights, the lower it is, the sooner it fires (easiest to see in D7 system_list, ORDER BY weight ASC).
And, to make this more dire, if you pick the wrong priority on certain events your code might not work or might be insecure! In case, KernelEvents::REQUEST if you choose 33 or higher then routing didn't happen. If you choose 31 or 32 then routing did happen but access check did not. The separation of these two might be a sechole, but I have a different issue for that. @Daniel Wehner points at https://www.drupal.org/files/project-images/webprofiler4.png or similar to give people an idea of what fires when.
Proposed resolution
Document the event constants together with the Event class they use:
a) [This part is to be done on THIS ISSUE]
We will add a group/topic called
@defgroup events
This would:
1. Describe what "events" are, and point out the base Symfony class \Symfony\Component\EventDispatcher\Event
2. Describe how events are dispatched.
3. Describe how to subscribe to events.
4. Describe event priority.
5. Be linked on https://api.drupal.org/api/drupal/8 (maybe in "Additional topics")
6. Be linked on https://api.drupal.org/api/drupal/core!includes!menu.inc/group/menu/8 (which should mention event subscribers, shouldn't it? I guess we left out dynamic routes there too...).
b) (This part is being handled on #2382169: [meta] Add @Event to all events defined by drupal core)
In each Drupal Core class that defines an event constant, add
@Event
in the constant doc block. The doc block should describe:
- What the event is
- What it is used for
- What kind of $event object is passed along.
- @see links to related classes (event class, representative class where event is dispatched, representative class where event is subscribed to).
One example would be the following
/**
* Denotes an event at the very beginning of request dispatching.
*
* This event allows you to create a response for a request before any
* other code in the framework is executed. The event listener method
* receives a \Symfony\Component\HttpKernel\Event\GetResponseEvent
* instance.
*
* @Event
*
* @var string
*/
const REQUEST = 'kernel.request';
c) (This is a separate issue: #2304395: Keep track of references for constants)
In the API module, it would keep references to constants, so on any constant page, you would get "10 functions use this constant" list (similar to "10 calls to function" or "10 invokes of hook". I thought it already did that, but I'm sure not seeing it on pages like https://api.drupal.org/api/drupal/core!vendor!symfony!http-kernel!Symfon...
That would lead you both to places where the event constants are being dispatched and where they're being subscribed to.
d) (Separate issue: #2393631: Treat @Event like @ingroup events)
The API module needs to read @Event and automatically add them as if they had @ingroup events. We're doin something similar with @Annotation already, so this will not be difficult.
This is critical in the release blocking sense.
Remaining tasks on this issue
- Add @defgroup
- update https://www.drupal.org/node/1354
Comment | File | Size | Author |
---|---|---|---|
#56 | 2264049_56.patch | 4.49 KB | chx |
#56 | interdiff.txt | 1.05 KB | chx |
#51 | interdiff.txt | 2.36 KB | jhodgdon |
#51 | 2264049-51.patch | 4.4 KB | jhodgdon |
Comments
Comment #1
catchComment #2
jhodgdonCan you give me more details on the laborious process I would use if I wanted to find them? If I understand it, I may be able to make the API module list them. Thanks!
Comment #3
catchThis finds most of them I think.
https://api.drupal.org/api/drupal/core%21vendor%21symfony%21event-dispat...
https://api.drupal.org/api/drupal/core%21vendor%21symfony%21event-dispat...
grep -r "dispatch(" core/lib/*
Comment #4
jhodgdonOK then... It sounds like we need a topic/group about Events that would explain what they are, and tell people to look for classes that extend the base Event class or declare their use of it? It doesn't sound like we need to make the API module list them explicitly, like I think we need to do for services?
Comment #5
catchI don't think that's enough. It'd be equivalent to Drupal 7's hook documentation being a list of functions that call module_invoke_all() and drupal_alter().
Two things missing from that:
1. Documentation of the specific event, why it exists, what you can do with it - equivalent of hook docs in .api.php
2. Classes that subscribe to a specific event, for example https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Routing%2... is the closest I could find to a list of subscribers to the routing event - but that's only because there's a base class, which is an implementation detail.
Comment #6
jhodgdonOK... Seems useful, indeed. So, how can we do this then?
I just looked at ConfigImporterEvent...
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Config!ConfigImpo...
This class has NO DOCBLOCK whatsoever. Sigh. How is this crap getting added to core?
So... Where does the class define what the event type string is? And how does one subscribe to this event? I just simply don't know enough about this to even suggest how it should be documented.
Comment #7
catchYes this is part of why I objected to using events at all, and pushed back on nearly all of the straight event -> hook patches as well as HookEvent.
The first event usage went in without resolving any of these issues - mainly because those were events that Symfony exposes itself (that's also a problem if we add any Drupal-specific event documentation - unless we ensure we can include events that Symfony dispatches itself in that same documentation).
Documenting on the Event subclass wouldn't help the issue at all, because you don't need an Event subclass to dispatch an event (the fact you need an Event class at all is an annoyance with the entire system since lots of events/listeners won't even use it, but that's not for fixing here). It's hard to push back on documentation when there's no agreed or obvious place to put the documentation.
See #2023613: Move event registration from services.yml to events.yml for proposed improvement to event listener registration which should also explain the current mechanism.
Comment #8
jhodgdonOK, I looked at the summary for that other issue... So what I'm seeing is that currently:
a) EventSubscriberInterface classes can declare what events they are listening to by returning something from getSubscribedEvents() -- the array keys are the names of the events. As far as I can tell, these are free to be anything you want, or they can be ones provided by Symfony.
b) Subscriber classes declare themselves in services.yml files, by tagging entries with 'event_subscriber'.
c) Presumably some classes somewhere have the ability to cause a named event to "happen". I don't know what that is but it must be the case for these custom event strings.
So ... I think I understand a bit better what you're talking about now. You want to come up with a mechanism for documenting and listing the event names that various modules are defining (and that Symfony defines). Right?
So... This is kind of like the question of how to document the Form/Render elements. We have this horrible Form API reference document that never gets updated (and doesn't even include the render elements). We have a long-open issue on how to make maintainable documentation for these, but no one wants to agree on how to do it.
It seems like for events that Drupal Core defines (as opposed to Symfony), we could have a custom @tag like putting "@event" in the doc block:
This is the constant that defines the map type in the example on that other issue; I made up the docs, as I have no idea what it is really). Anyway, conceptually on the constant, you would put @event, which the API module would note as "This is an event doc block", and add to the "list of events".
Would something like that work?
Comment #9
jhodgdonOr it could, even simpler, be @ingroup events, and then we could have a @defgroup events to explain what they are etc.
Comment #10
jhodgdontim.plunkett clued me in via IRC that calls to ->dispatch(ConfigEvents::DELETE) and things like that would be where events are dispatched.
So we might be able to gather that information in the API module... let me stew on it for a bit.
Meanwhile, do you think an @ingroup events on the doc block for things like ConfigEvents::DELETE would be a good way to document the event constants themselves and a good place to write up a brief synopsis of how to program with events?
Comment #11
chx CreditAttribution: chx commented* @see \name\of\the\class\that\listens\for\it
that won't be possible if you want to list contrib listeners as well. Perhaps a defgroup for every event? (yuck)
Comment #12
chx CreditAttribution: chx commented> do you think an @ingroup events on the doc block for things like ConfigEvents::DELETE would be a good way to document the event constants themselves and a good place to write up a brief synopsis of how to program with events?
Aye. Those constants are a good place just don't forget to @see the relevant event class (which might or might not be unique per event).
Comment #13
jhodgdonRE #11 - I should have said
The point being: Make @see links to any relevant classes related to this event.
Comment #14
chx CreditAttribution: chx commentedComment #15
tim.plunkettComment #16
jhodgdonOK... Not adding this to the issue summary yet, but here's a proposed plan for how to deal with this. Thoughts?
----------------
a) We add a group/topic called
@defgroup events
This would:
1. Describe what "events" are, and point out the base Symfony class \Symfony\Component\EventDispatcher\Event
2. Describe how events are dispatched.
3. Describe how to subscribe to events.
4. List the Symfony defined event constants, or link to a Symfony docs page that lists them, or the class that includes them.
5. Describe event priority.
6. Be linked on https://api.drupal.org/api/drupal/8 (maybe in "Additional topics")
7. Be linked on https://api.drupal.org/api/drupal/core!includes!menu.inc/group/menu/8 (which should mention event subscribers, shouldn't it? I guess we left out dynamic routes there too...).
b) In each Drupal Core class that defines an event constant, add
@ingroup events
in the constant doc block. The doc block should describe:
- What the event is
- What it is used for
- @see links to related classes (event class, representative class where event is dispatched, representative class where event is subscribed to).
c) In the API module, it would keep references to constants, so on any constant page, you would get "10 functions use this constant" list (similar to "10 calls to function" or "10 invokes of hook". I thought it already did that, but I'm sure not seeing it on pages like https://api.drupal.org/api/drupal/core!vendor!symfony!http-kernel!Symfon...
That would lead you both to places where the event constants are being dispatched and where they're being subscribed to.
Comment #17
chx CreditAttribution: chx commentedThis sounds good to me. Together with c), that'd be lovely, I didn't realize API module was this smart -- does it work with use statements and all that?
Comment #18
jhodgdonThe API module does quite a lot of code parsing. That's how we get all of the "N invokes of this hook", "N functions call this", etc. stuff on api.drupal.org. (c) should be done whether or not we adopt the rest of this issue, so I went ahead and filed #2304395: Keep track of references for constants in the API module (this issue is marked as "related" on that one).
Since I've gotten one +1, I'm adding this to the issue summary and putting it at "needs review", for the "idea" or "policy". If people think this is the way to go, the next step would be to write up the defgroup/topic as a patch, and then (either in the same patch or another one), get going on documenting the event constants.
Comment #19
larowlan+1 to proposal at #16
Comment #20
dawehnerTotally +1 for the general idea of #16
Did we considered to try to put that into upstream? It is simply general documentation which could/should live inside the event dispatcher component,
as Drupal is probably not the only project using it.
Did we considered to split this up into routing and menu already?
Isn't @uses semantically more correct here? Just curious, as @uses introduces an implicit two way relationship.
So for "our" events we could put that directly in the code, but can ship with a file which describes the ones from other components?
Comment #21
dawehnerAs some people don't want to post to upstream here is one: https://github.com/symfony/symfony/issues/11878
Comment #22
larowlanFor 16a any reason we can't link to symfony components book?
Comment #23
dawehnerWell it would be nice if we could pull their generic docs into api.drupal.org, on top of it we still need some nice tag.
Comment #24
jhodgdonRE #20 - there is no @uses tag in our docs tags. @see makes a See Also section, so that is what we want.
Regarding Symfony, if they have a good reference to events in their on-line docs, by all means let's add a link in our docs to there. Their API-embedded docs (which we are pulling in to api.drupal.org) are generally lacking, from what I've seen, since they've made the decision to have their on-line book store their API reference. We don't do that...
Comment #25
dawehnerSo symfony uses @Event annotation all over the place, this should work fine for us as well.
Should we open a novice task to add this to all our events?
Comment #26
jhodgdonI just did a grep on the core/vendor code in Drupal 8 and did not find "@Event" anywhere. Can you provide an example of what they are doing and what your proposal is for Drupal code? I am confused.
Comment #27
dawehnerI'm sorry but the symfony version would be probably 2.6 beta2 ... so its not included yet in the version of core.
So this is just a quick example of how this would look like.
Comment #28
jhodgdonGreat -- that seems simple enough. Looking at the proposal currently in the issue summary, I guess the idea would be to use @Event instead of the proposed @ingroup events then, and then the API module would need to automatically find these and list them on the @defgroup events page. We are already doing something like this for @Annotation in the API module, so adding @Event is consistent with this.
It looks like the proposal in the issue summary was that besides adding a marker to the constant doc bloc, we would also be adding some documentation explaining the event... but for the example in the patch, it looks like this documentation is already present (good!).
So... I went to symfony.com to confirm this ... I didn't know where to look. ... Since this is not currently in our source code and I'm not all that familiar with Symfony, can you point to any documentation of @Event there or some of their current code that uses it, so we can confirm this is what they're planning on doing for sure (putting @Event on the event constants)?
Comment #29
dawehnerEach component hand-documents their events or not documents events at all.
When we asked in https://github.com/symfony/symfony/issues/11878#issuecomment-54996882 it became clear that they don't have such machine-readable documentation yet,
but they have been open to add it.
Right, the documentation would be aligned with the actual constant in the file itself.
Updated the issue summary based upon that, do you think this works out for us?
Comment #30
jhodgdonOK, but on this issue you've suggested adding "@Event" to our events. That issue seems to be suggesting "@category events".
Let's make sure we adopt the Symfony standard. What is it? Is there a new issue?
Comment #31
dawehnerMeh, I haven't updated the issue summary on that issue, just have a look at the final code which went in
https://github.com/symfony/symfony/pull/12299/files
So should we have one issue which adds
@Event
to all our custom events?Comment #32
jhodgdonOK.
If there are only 20 or so events, I'd say we can do them in one patch. If there are a lot more, we might want to have multiple patches. We also need to have in the patch the @defgroup described in the Proposed Resolution.
Comment #33
dawehner#2382169: [meta] Add @Event to all events defined by drupal core is one issue.
Comment #34
jhodgdonOK, great! So we can use this issue here to add the @defgroup then?
Comment #35
dawehnerSounds like a good plan.
Comment #36
xjmComment #37
YesCT CreditAttribution: YesCT commentedShould we update https://www.drupal.org/node/1354#order about where @Event should go in the order? (And I guess add a section for it there in general)
Comment #38
dawehner... Adapted the issue summary to include the example of #2382169: [meta] Add @Event to all events defined by drupal core
Do you think we agree with this policy so someone could work on a patch?
According to #2382169-8: [meta] Add @Event to all events defined by drupal core @jhodgdon also agreed with the proposal.
I would argue that we use #2382169: [meta] Add @Event to all events defined by drupal core to do everything described in b) and another issue, maybe this one, to resolve
everything which is part of a) in the issue summary.
Comment #39
jhodgdonAgreed, updated issue summary accordingly. Yes let's do (a) here.
We also probably need to add the @Event "tag" to node/1354, right?
Comment #40
jhodgdonissue link malfunction. :)
Comment #41
jhodgdonOh, will need one more task...
Comment #42
dawehnerWell sure, once we have agreed properly on the policy?
Comment #43
jhodgdonThe policy comes from "Let's adopt the same thing Symfony does" and no one but us two cared to discuss it, so I think we can probably call it "adopted". :)
Comment #44
dawehnerJust had a question, maybe you can answer it: #2401843: Move all *.api.php files except system.api.php out of system module directory
Let's start with some patch to get some momentum on this issue.
Comment #45
chx CreditAttribution: chx commentedcatch asked the behavior from #2344645: The order of event subscribers should not be affected by definition order to be documented "somewhere" and this is that somewhere.
Comment #46
jhodgdonI commented on the other issue.
Regarding this patch:
In this part of the patch, I think this link needs more context. This is just going to show up as a separate paragraph in the page saying "Events" without any explanation.
The Events topic also needs some proofreading (I can't do it right now, busy, but will try later) and some formatting fixes (like the list items - should line up with the previous paragraph, and regular paragraph lines should not be indented, and there are blank lines at the end).
I also think it needs some attention to terminology and content:
- Some places events are "thrown" and some they are "dispatched" and some they are "fired" -- let's pick one and be consistent.
- "payload" is not described/defined
- In some cases services are not injected, so I think it would be better to say you "use" the event dispatch service rather than "inject" it, and maybe link to the Services topic for more information.
- a $subsystemEvents class ? What is this? I don't get it.
- In general... I don't think if I knew nothing about events, I could read this topic and figure out what they are and how to use them. It needs more explanation and background -- not a lot, just a few sentences here and there. I can take a stab at it probably later today if no one else does.
Comment #47
dawehnerhttp://symfony.com/doc/current/components/event_dispatcher/introduction.... has some good paragraph about the general
ideas of events, would be certainly worth to link to.
Thank you for the comments, I primarily wanted to get things started here, this issue is simply not something which has to lay around for ages.
Comment #48
jhodgdonHole in my schedule opened up... I will make a quick update to the patch right now.
Comment #49
jhodgdonOK, here's a new patch. I rewrote the Event topic, moved the link in the menu topic, and added information about priorities as per @chx comment #45 and the other issue about breaking ties. At least, I think so... please check this information!
Interdiff is not useful. Every line from previous patch changed.
I'm wondering if we should write a short section for the Routing/menu topic about dynamic routes now that we have this Event topic, but I think it should be a follow-up separate issue.
Comment #50
dawehner<3
Ideally I would like to swap dispatching and subscribing given that most people would subscribe but not dispatch
Not sure about that statement: Afaik the pattern is to have a new static class for defining the constants
Note: this is just the case if the major issue gets in, but I think its fine to assume that this will be the case.
Comment #51
jhodgdonRE #50 -
item 1 - glad you liked it. I added a small bit in this patch.
item 2 - I agree most people would want to subscribe and not dispatch. However the topic/page is short and *logically* the event gets dispatched and then there are reactions... so I'm OK with either order.... but the section on listening mentions the event that is described in the section on dispatching, so I think I prefer the curren torder.
item 3 - Right! Sorry, hadn't understood that from the previous patch. :) Fixed in this new patch.
item 4, yes, and on #45 it looks like we're being asked to add that to the patch so I did.
Comment #52
dawehner@jhodgdon
I guess the dynamic routing events will be documented when we document the corresponding event constants?
Beside from that I'm really happy with the current status, it is a huge step forward IMHO.
Comment #53
jhodgdonYeah that is a good idea to talk about dynamic routing when we document the corresponding event constant. We can add a short section to the menu.inc topic then, or a link to the docs we put on the constant. It's probably the most common special case that anyone will need to do, and a fairly common need.
Anyway... I think this is probably ready to go, if you do!
Comment #54
dawehnerYeah!
Comment #55
chx CreditAttribution: chx commentedTwo things at the end of the note (I might reroll this in a few hours but for now only have time to write this):
Comment #56
chx CreditAttribution: chx commentedThe ABC order is not implemented so #51 was IMO incorrect. Also, the security aspect was nuked in AccessAwareRouter (sigh) and even if it wasn't this doesn't define individual events.
I have added a "get out of jail free" card to the docs so that we can freely change to alphabetical later or anything else if we so want. As described in the now-major issue, it's completely happenstance over quite a few services that the definition order is kept so a change might happen even if we do not change the event system itself.
Comment #57
chx CreditAttribution: chx commentedAssigning to get a sign off from catch because this essentially introduces a new policy even if a minor one.
Comment #58
dawehnerIt is implemented, it is just not reviewed!
Comment #59
chx CreditAttribution: chx commentedThat means it's not in core currently, right? When it gets in then we will need to change this, yes but until then?
Comment #60
catchI think the get out of jail free is reasonable, although I doubt we'd change the ordering in a minor release - just tagged the ordering issue as RC deadline to reflect this. However a note that it's subject to change until then seems reasonable.
Docs look like a good start to me, let's get the topic done and committed separately to the actual event listing?
Comment #61
dawehner+1 to mention that you should use priority if this matters for you
Comment #62
jhodgdon+1, thanks!
Comment #63
webchickAwesome, happy to see this documented!
Committed and pushed to 8.0.x. Thanks!