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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Can 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!

catch’s picture

jhodgdon’s picture

OK 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?

catch’s picture

I 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.

jhodgdon’s picture

OK... 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.

catch’s picture

Yes 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.

jhodgdon’s picture

OK, 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:

final class RdfMappingEvents {
  /**
   * Event: mapping RDF types from input.
   *
   * @event
   *
   * @see \name\of\the\class\that\throws\it
   * @see \name\of\the\class\that\listens\for\it
   */
  const MAP_TYPES_FROM_INPUT = 'rdf.map_types_from_input';
}

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?

jhodgdon’s picture

Or it could, even simpler, be @ingroup events, and then we could have a @defgroup events to explain what they are etc.

jhodgdon’s picture

tim.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?

chx’s picture

* @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)

chx’s picture

> 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).

jhodgdon’s picture

RE #11 - I should have said

@see \name\of\representative\class\that\listens\for\it

The point being: Make @see links to any relevant classes related to this event.

chx’s picture

Title: Document events » Document events [security!]
Issue summary: View changes
tim.plunkett’s picture

Issue summary: View changes
jhodgdon’s picture

OK... 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.

chx’s picture

This 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?

jhodgdon’s picture

Title: Document events [security!] » [policy, then patch] Document events [security!]
Issue summary: View changes

The 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.

larowlan’s picture

+1 to proposal at #16

dawehner’s picture

Totally +1 for the general idea of #16

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.

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.

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...).

Did we considered to split this up into routing and menu already?

- @see links to related classes (event class, representative class where event is dispatched, representative class where event is subscribed to).

Isn't @uses semantically more correct here? Just curious, as @uses introduces an implicit two way relationship.

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).

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?

dawehner’s picture

As some people don't want to post to upstream here is one: https://github.com/symfony/symfony/issues/11878

larowlan’s picture

For 16a any reason we can't link to symfony components book?

dawehner’s picture

For 16a any reason we can't link to symfony components book?

Well 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.

jhodgdon’s picture

RE #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...

dawehner’s picture

So 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?

jhodgdon’s picture

I 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.

dawehner’s picture

FileSize
2 KB

I'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.

jhodgdon’s picture

Great -- 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)?

dawehner’s picture

Issue summary: View changes

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)?

Each 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.

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!).

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?

jhodgdon’s picture

OK, 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?

dawehner’s picture

OK, but on this issue you've suggested adding "@Event" to our events. That issue seems to be suggesting "@category events".

Meh, 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?

jhodgdon’s picture

OK.

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.

dawehner’s picture

jhodgdon’s picture

OK, great! So we can use this issue here to add the @defgroup then?

dawehner’s picture

Sounds like a good plan.

xjm’s picture

Issue tags: +Triaged D8 critical
YesCT’s picture

Issue summary: View changes

Should 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)

dawehner’s picture

Issue summary: View changes
Issue tags: +Ghent DA sprint

... 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.

jhodgdon’s picture

Issue summary: View changes

Agreed, updated issue summary accordingly. Yes let's do (a) here.

We also probably need to add the @Event "tag" to node/1354, right?

jhodgdon’s picture

Issue summary: View changes

issue link malfunction. :)

jhodgdon’s picture

Issue summary: View changes

Oh, will need one more task...

dawehner’s picture

We also probably need to add the @Event "tag" to node/1354, right?

Well sure, once we have agreed properly on the policy?

jhodgdon’s picture

The 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". :)

dawehner’s picture

FileSize
2.89 KB

Just 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.

chx’s picture

catch 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.

jhodgdon’s picture

Status: Active » Needs work

I commented on the other issue.

Regarding this patch:

+++ b/core/includes/menu.inc
@@ -47,6 +47,8 @@
  * https://www.drupal.org/developing/api/8/routing and
  * https://www.drupal.org/developing/api/8/menu
  *
+ * @link events Events @endlink
+ *

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.

dawehner’s picture

http://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.

jhodgdon’s picture

Hole in my schedule opened up... I will make a quick update to the patch right now.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
4.23 KB

OK, 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.

dawehner’s picture

  1. +++ b/core/modules/system/core.api.php
    @@ -1812,3 +1813,62 @@ function hook_display_variant_plugin_alter(array &$definitions) {
    +
    + * Events are part of the Symfony framework: they allow for different components
    + * of the system to interact and communicate with each other. Each event has a
    + * unique string name. One system component dispatches the event at an
    + * appropriate time. Other system components can register as event subscribers;
    + * when the event is dispatched, a method is called on each registered
    + * subscriber, allowing each one to react. For more on the general concept of
    + * events, see
    + * http://symfony.com/doc/current/components/event_dispatcher/introduction.html
    

    <3

  2. +++ b/core/modules/system/core.api.php
    @@ -1812,3 +1813,62 @@ function hook_display_variant_plugin_alter(array &$definitions) {
    + * @section sec_dispatch Dispatching events
    

    Ideally I would like to swap dispatching and subscribing given that most people would subscribe but not dispatch

  3. +++ b/core/modules/system/core.api.php
    @@ -1812,3 +1813,62 @@ function hook_display_variant_plugin_alter(array &$definitions) {
    + * interact with services). The first argument is the unique event name, which
    + * you should normally define as a constant in your class that is dispatching
    

    Not sure about that statement: Afaik the pattern is to have a new static class for defining the constants

  4. +++ b/core/modules/system/core.api.php
    @@ -1812,3 +1813,62 @@ function hook_display_variant_plugin_alter(array &$definitions) {
    + * priority is zero. If two event subscribers for the same event have the same
    + * priority, the module weight of the modules that define the subscriber
    + * services will be used to break the tie. In case there is still a tie, the
    

    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.

jhodgdon’s picture

FileSize
4.4 KB
2.36 KB

RE #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.

dawehner’s picture

@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.

jhodgdon’s picture

Yeah 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!

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Yeah!

chx’s picture

Status: Reviewed & tested by the community » Needs work

Two things at the end of the note (I might reroll this in a few hours but for now only have time to write this):

  1. Add "or not define a priority"
  2. Add what happens to events w/o priority or same priority in the same service file
chx’s picture

Title: [policy, then patch] Document events [security!] » Create an Events topic
Status: Needs work » Needs review
FileSize
1.05 KB
4.49 KB

The 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.

chx’s picture

Assigned: Unassigned » catch

Assigning to get a sign off from catch because this essentially introduces a new policy even if a minor one.

dawehner’s picture

The 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.

It is implemented, it is just not reviewed!

chx’s picture

That means it's not in core currently, right? When it gets in then we will need to change this, yes but until then?

catch’s picture

Assigned: catch » Unassigned

I 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?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

+1 to mention that you should use priority if this matters for you

jhodgdon’s picture

+1, thanks!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome, happy to see this documented!

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 3e8ab5a on 8.0.x
    Issue #2264049 by jhodgdon, dawehner, chx, catch: Create an Events topic
    

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.