Problem/Motivation

Proposed resolution

These should be documented in the api.php files along with hooks in a new group

/**
 * @addtogroup events
 * @{
 */

/**
 * Act on save events in the configuration system.
 *
 * @ingroup config
 */

 {something here}

In order to fully implement a subscriber you need to create an entire class which implements EventSubscriberInterface, override getSubscribedEvents() and add one function per event you are responding to. So the question is, what do we put in for the code sample? We could just put in the event-specific code which would look like this:

  /**
   * Override configuration values with global $conf.
   *
   * @param Drupal\Core\Config\ConfigEvent $event
   *   The Event to process.
   */
  public function configInit(ConfigEvent $event) {
    global $conf;

    $config = $event->getConfig();
    if (isset($conf[$config->getName()])) {
      $config->setOverride($conf[$config->getName()]);
    }
  }

but I feel that is deceptive because it doesn't have everything you need to implement this event. Or we could include the whole class definition

<?php
namespace Drupal\Core\EventSubscriber;

use Drupal\Core\Config\Config;
use Drupal\Core\Config\ConfigEvent;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;

/**
 * Override configuration values with values in global $conf variable.
 */
class ConfigGlobalOverrideSubscriber implements EventSubscriberInterface {
  /**
   * Override configuration values with global $conf.
   *
   * @param Drupal\Core\Config\ConfigEvent $event
   *   The Event to process.
   */
  public function configInit(ConfigEvent $event) {
    global $conf;

    $config = $event->getConfig();
    if (isset($conf[$config->getName()])) {
      $config->setOverride($conf[$config->getName()]);
    }
  }

  /**
   * Implements EventSubscriberInterface::getSubscribedEvents().
   */
  static function getSubscribedEvents() {
    $events['config.init'][] = array('configInit', 30);
    return $events;
  }
}

Which seems like it is more useful, but also lacks some context and is a lot longer.

Or we could do something else I'm not thinking of.

Thoughts?
After we agree on an implementation we will need followup issues for api module and to add the appropriate documentation to the config.api.php file.

Comments

gdd’s picture

Component: configuration system » documentation
jhodgdon’s picture

I was asked to comment here, but I'm not sure exactly what the question is or what the proposal is... I'm confused by the issue summary, in other words.

gdd’s picture

The proposal is to document module-defined events in the api.php files alongside hooks. The question is, what makes sense to include for the sample code? For hooks, we list a complete function that responds to that hook. For events, you need to create a complete class with two functions in it to respond. Do we include put that entire class definition in the sample code? Or do we only list the one function that is specifically called when the event is fired?

jhodgdon’s picture

Ah. Thanks. :)

If you need to create a complete class with two functions, then I think the api.php should include the complete class with the two functions as its sample code.

Crell’s picture

Note that the class in practice needs to be namespaced and in its own file, per PSR-0. How does that work in an api.php file?

Should we perhaps be investigating a different approach to documenting events than hooks, since they don't work in the same way?

gdd’s picture

They do work the same way, kind of. It would be nice not to have to come up with an entirely new way to document these. I guess we could do a system.events.php that has a complete class, with docs, for all the available events. This seems excessive though, and would be a lot more work to implement in api.module. I'll think on it.

jhodgdon’s picture

So... let's think about this a bit.
- I don't think it makes sense to put just a disconnected method or two into an api.php file.
- To me, putting a class into an api.php file seems OK, without the namespace stuff and without following PSR-0, because we aren't loading these api.php files anyway -- we could put a note in the class documentation saying that you need to use namespaces, couldn't we?
- The other thing we would need to do is think of a naming convention... I mean, for hooks, we "define" a "function" called hook_whatever(), and it's understood that in your module, it would be mymodule_whatever() and that the hook function body is a sample. So for these... What does someone really need to do to make one of these listeners (step 1, step 2, etc.)? What does the class need to be called, and what namespace does it need to be in? Given that, what would make sense for a naming convention?

Crell’s picture

There is no implementation-defined requirement for naming, since listeners need explicit registration via a Bundle class. The class itself, and the methods, may be called anything you want, and you can even register for the same event multiple times in one class. It's annoyingly more flexible than what we have now. :-)

The convention I've seen in Symfony so far is that a method for event_name gets called onEventName(). I'm not always doing that in the existing events for the kernel, since in several places we have multiple listeners on the same event in one class, but I think that's a not-bad default recommendation. Eg, "if you're not sure what to call it, call it onEventName()".

jhodgdon’s picture

OK. So we need to figure out a standard for the dummy documentation class names then, and figure out which methods are essential to include in the dummy documentation classes. Any ideas? Maybe start the fake/docs class names with "Sample" or "Example"?

jhodgdon’s picture

Issue tags: +Coding standards

tagging

Damien Tournoud’s picture

Alternatively, we could build a EventSubscriberInterface that subscribes to events with annotations.

namespace xxx;

use Drupal\Core\Config\Config;
use Drupal\Core\Config\ConfigEvent;
use Drupal\Component\NonSuckyEventDispatcher\AnnotationEventSubscriber;

class ConfigGlobalOverrideSubscriber extends AnnotationEventSubscriber {
  /**
   * Override configuration values with global $conf.
   *
   * @Event('config.init')
   * @param ConfigEvent $event
   *   The Event to process.
   */
  public function configInit(ConfigEvent $event) {
    global $conf;

    $config = $event->getConfig();
    if (isset($conf[$config->getName()])) {
      $config->setOverride($conf[$config->getName()]);
    }
  }
}
jhodgdon’s picture

RE #11 - um. that doesn't look like an interface and doesn't look like it implements an interface?

Crell’s picture

Over in #1793520: Add access control mechanism for new router system, there is discussion of requiring the use of constants for newly defined events. Symfony is a bit inconsistent here, sometimes using a class with just constants on it and sometimes not. However, frequently a different class of event requires a new, er, class for that event.

So, what about leveraging that. Vis:

class FooEvent implements EventInterface {
  
  /**
   * Event for a bar.
   * 
   * Some longer discussion of what foo.bar is for and how to use it.
   */
  const BAR = 'foo.bar';

  // Stuff for the EventInterface here, and whatever other methods are relevant.

}

(The constants may also get split off from the Event class itself, but I don't much care for classes that exist only to hold events.)

That wouldn't include sample implementation code, but would give us a fairly good index of all defined events and their descriptions/instructions.

jhodgdon’s picture

Title: Determine standard for documenting Events and Subscribers/Listeners » [policy, then patch] Determine standard for documenting Events and Subscribers/Listeners

As a note, there is a somewhat-related documentation standards debate going on regarding standardizing the documentation for callback-type functions (of which there are many in D7; probably less in D8 -- the d7 ones are things hooks need). The proposal there is to define/document dummy/sample "functions" called callback_* [like we currently do for hook_*] in api.php files. That issue:
#1250500: [policy adopted; patch done] Find a (standard) way to document callback signatures

So... it seems like if we're going to have dummy classes as samples, then we should adopt a naming scheme for them. We have hook_* for hooks, probably (not adopted yet) callback_* for callback functions... how about if all sample classes' names start with Sample*? And maybe we can put them all into a standard "namespace", like Drupal\Samples or something? The API module (and api.drupal.org) is not yet smart about namespaces (that's the Next Big Thing I plan to work on though -- probably within a couple of weeks at the most and maybe this weekend ;) ), but when it is it would be cool to have one for these samples.

Xano’s picture

I see that \Drupal\Core\Routing\RoutingEvents (which contains the routing API's events) was added about two weeks after the previous comment in this issue, and that it follows the approach Symfony uses with KernelEvents.

I've been using KernelEvents for the past few days and I must say I like how it works. It's fairly easy to document events, although the biggest regression would be that it's harder to document the arguments that are passed on to listeners than it is to document hooks. Then again, it's hard to make modules provide dummy event listeners that are not misleading to readers, due to the complexity using classes in Drupal brings with it. If we document that every module should have a \Drupal\module\Events\ModuleEvents class, developers know where to look when searching for events, and api.module can optionally parse it and present events in a special way on api.d.o.

I'd like to raise the issue of naming conventions for events. It looks like if I write Foo.module, and it also offers a kernel.request event, this collides with Symfony's event of the same name. Symfony's documentation on event names does not explicitly mention this issue, but the fact they recommend using a prefix tells me this issue is indeed real.
I suggest using the format: drupal.$module.$event, which is based on the vendor/package convention used everywhere in PHP land. It will not only prevent collisions within the Drupalsphere, but also with events outside of it.

eojthebrave’s picture

This issue is kind of old. And I'm just coming across it randomly right now. But, I think we have a standard in place for documenting events. See https://www.drupal.org/coding-standards/docs#event

Maybe we could/should add a note there about using a naming convention, as well as here https://api.drupal.org/api/drupal/core%21core.api.php/group/events/8. I agree that having a standard like that is probably a good idea.

jhodgdon’s picture

Status: Active » Fixed

We can just close this actually.

Status: Fixed » Closed (fixed)

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