The order of event subscribers should not be affected by definition order

note @sun's proposal in this comment #1509164-29: Use Symfony EventDispatcher for hook system
magic numbers aren't good, before/after then module dependencies is better

This will require a container rebuild, so tagging D8 upgrade path per #2341575: [meta] Provide a beta to beta/rc upgrade path.

Also more descriptive title.

Uh, the title is misleading. This is not a problem with the YAML or the definition order but a problem with priorities missing for the events in AuthenticationSubscriber::getResponse() vs. FinishResponseSubscriber().

I don't think the title is misleading.

For hooks we order them by module weight, name - that gives a reliable execution order which won't change unless you set a different weight or rename a module - which are conscious actions known to affect hook ordering.

Having the order affected by the YAML definition is considerably more brittle. If we didn't have test coverage this could have been done in a 'clean up' patch with no obvious side effects.

We might well want to set explicit weights for those two event listeners but that's not what the critical issue is for - could split that into a separate critical issue if necessary.

The Symfony event dispatcher maintains a list of callables (listeners) ordered by priority. Listeners can also be closures. Like pointed out by chx the order of the event listeners with the same priority depends on the implementation. Given that listeners can basically be any callable we cannot rely on any metadata (module name, class name, function name) which could be used as a second sort-key.

Instead I propose that event subscribers like the authentication subscriber or the access aware router subscriber can be tagged as exclusive in the service definition. This information could be used to ensure from within the compiler pass, that no other subscribers are registered with the same priority as an exclusive subscriber.

@znerol, yes it's an unreliable model, and we may have to implement only a subset of the features to make it work with a modular system like Drupal - for example throwing an exception if no explicit weight is defined, or adding before/after instead of weights and using that, then documenting the behaviour properly.

Please keep in mind that the service definition of a subscriber is not the only place where listeners/subscribers can be added to the dispatcher. It is also possible to manipulate the list of listeners for an event at runtime using the methods defined in the interface.

I agree that it might be much easier if we did not have to define priorities but instead just would declare dependencies. Such a system might calculate the priorities from within a compiler pass before adding the listeners to the dispatcher. But that would make it difficult to add/remove listeners at runtime through the Symfony interface because priorities are not easily determinable anymore. That interface is the base line, let's just accept that.

Throwing an exception when priorities clash is the way to go - but not for all listeners/subscribers. Instead let's just do that for "milestones". Adding a subscriber with the same priority as the router subscriber is just a plain stupid idea and that should be punished. Other events are much less suspectible to execution order, like e.g. the TERMINATE event.

Talking about priorities, I just want to point out that git grep KernelEvents::REQUEST reveals the following pattern:

c/v/s/h/S/C/H/E/ProfilerListener.php:            KernelEvents::REQUEST => array('onKernelRequest', 1024),
c/v/s/h/S/C/H/E/SessionListener.php:            KernelEvents::REQUEST => array('onKernelRequest', 128),
c/v/s/h/S/C/H/E/FragmentListener.php:            KernelEvents::REQUEST => array(array('onKernelRequest', 48)),
c/v/s/h/S/C/H/E/RouterListener.php:            KernelEvents::REQUEST => array(array('onKernelRequest', 32)),
c/v/s/h/S/C/H/E/LocaleListener.php:            KernelEvents::REQUEST => array(array('onKernelRequest', 16)),
c/v/s/h/S/C/H/E/ErrorsLoggerListener.php:        return array(KernelEvents::REQUEST => 'injectLogger');

If there is only one 1 in the binary representation of the priority, then this is a milestone...

Since this issue was assigned to me at one point...

We've already made the statement, including straight from Dries' mouth, that longer-term (D9) we should be moving away from hooks to events. Therefore any statement of "we can't have hooks and events together" must imply that we eliminate hooks, not that we eliminate events. We have already backed off on even partial hook-to-event conversions in the interest of not introducing "unnecessary" changes (a decision I have long disagreed with). So D8 living with hooks and events coexisting is, really, no longer a negotiable situation.

Weird ordering of things is hardly a new concept in Drupal; hook order has always been a challenge, to the point that we kept trying to add per-hook weights for cases where module weights were insufficient. Well, events now have per-hook weights, essentially. So rather than use them, the proposal is to avoid events? That does not make even the slightest bit of sense.

If the order of a given event matters, specify a weight. If it doesn't, well, it doesn't. If you don't think it does but it actually does, add a weight when you figure that out.

While a before/after ordering would be lovely, that's far out of scope for Drupal 8, and really ought to be done upstream in Symfony anyway.

This issue is a won't-fix for me.

This issue is ballooning out of control. First it was fixing event subscribers, then it was removing events, now you're suggesting removing HttpKernel and the routing system?
Do you think we're going to reintroduce hook_menu?!

Let's get this issue back on track, where it was in #1 and #2.

Let's try a new issue with more focus and less hyperbolic silliness.

So that might be a stupid question but:

- We still have modules
- We still have weights
- We have module names
- We have the class name

D7 ordering (without hook_mod_impl_alter):

- Module Weight
- Module Name

=> consistent, only one hook per module

Proposed D8 ordering:

Use Drupal or System as "module name" for any events not belonging to a module.

Because we have consistent namespaces, we should be able to figure out to what module an event subscriber belongs to.

- Priority
- Module Weight
- Module Name
- Class Name

=> Consistent and working, changeable and predictable

As we persist services during the compiler pass and we have all modules loaded there (obviously), I cannot see why we cannot store the order per EventSubscriber?

Such module implements alter to change weights is kinda implemented as in Drupal 7, but just by instead changing the tagged services in the compiler pass.

Like I recently saw in a patch for the AcceptNegotiation Middleware where some format was subscribed to by changing compiler kernel run pass.

Is there anything I am missing here?

It is important to understand that listeners / subscribers can be added at runtime - not only from within the compiler pass. If something uses addListener the event dispatcher cannot possibly determine its origin.

class MyController  {
  public function action(Request $request) {
    $this->eventDispatcher->addListener(KernelEvents::TERMINATE, function () { /* do some work after the request already has been sent to the browser */ });
    return new Response('here is your response, some server side work still going on');
  }
}

It is okay for them to be added at runtime:

But thanks for telling me how to implement BigPipe in core cleanly :-D - that was what I wondered how to do.

Anyway back to the issue:

If they have a unique priority, they get ordered by priority, if they don't we run them in arbitrary order either before or after all our compiled event subscribers.

I would vote for after, because if you need before, just increase priority.

And if that dynamic order is a problem => upstream symfony problem

but as you can set priorities that is as low a use case problem as can get, so I don't think we will ever hit that case.

Any more concerns?

Let me make sure I understand the issue:

In D7, the order of hook invocations is deterministic but sometimes obtuse, based on module weight, then module name.

In D8, the order of event invocations is deterministic but sometimes obtuse, based on priority, then module weight, then module name, then order of the event subscriber in services.yml. (Because we still parse through modules in the same order when parsing the container.) Right?

First off, there's no way that's critical. I'm half tempted to call it minor.

Secondly, I am inclined to agree with FabianX that if you find yourself in a situation where the order matters that much, use a priority and your problems go away. So I'm not convince there's an actual bug to address here.

If we do want to actually do something, finagling the service order to be alphabetic by class in via a compiler pass seems like a reasonable and still deterministic solution.

This is critical as moving services in services.yml should not break something - unless there is a contract somewhere that that is not allowed.

If a service is not used always, this can lead to very difficult to find bugs and its a region where you don't expect change to break something. (I would totally accept a patch as a GK that reordered services.yml to group some services more logically and 2 weeks later I am bitten on production that some service fails ... )

If we support module weight implicitly due to the order how the services.yml is parsed, this should be fine and all we need to do is to additionally sort within system / the same module by class name.

So just a little sort adding and we should be done.

The only reason this is a new problem in D8 is that modules never did have a hook twice.

Started with just writing down the tests how its expected to work. While doing that I realized that the current class is horrible named.

- Priority
- Module Weight
- Module Name
- Class Name

I think we could get rid of the module name if we just consider the full namespace + classname for the sorting.

Let the tests begin to fail :)

There we go

There we go.

So sadly the order of the events change which seems to let them all fail.

.

Interesting bug.

Worked a bit on it, and figured out that we have still some inconsistency in DrupalKernel::$moduleList.
It is sometimes considered to be a hashmap of module extensions keyed by module name, and sometimes a hashmap of module weights, keyed by module weight.

Let's see how much failures we do have left ... at least one of the web tests pass already.

.

Fixed nearly all notices, working on the last remaining one (probably).

Please note that if definition-dependent ordering is really an issue, then there is much more stuff affected by it. Not only the event dispatcher but basically anything that implements tagged services, i.e. most of the stuff implementing a chain-of-responsibility pattern.

If we really want to rely on the alphabetic order of modules, then it should be enough to sort the service definitions by module name before compiling the container.

Alright, looked at the resulting order, and fixed it.

Can someone fix the summary?

Totally valid point!

1. +++ b/core/includes/common.inc
   @@ -2768,7 +2768,7 @@ function drupal_flush_all_caches() {
   +  \Drupal::service('kernel')->updateModules(\Drupal::config('core.extension')->get('module'), $files);
   

   What is this change about? And why not use the module handler (which is assigned below/after atm)?

2. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterEventListenersPass.php
   @@ -19,8 +22,43 @@ public function process(ContainerBuilder $container) {
   +      if ($drupal == 'Drupal' && isset($module_parameter[$provider]['weight'])) {
   

   A comment here that you are using the set weight for a module if available would be good.

3. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterEventListenersPass.php
   @@ -19,8 +22,43 @@ public function process(ContainerBuilder $container) {
   +    // Sort the IDs first by module weight, than by full classname. Using the
   

   then

4. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterEventListenersPass.php
   @@ -19,8 +22,43 @@ public function process(ContainerBuilder $container) {
   +    // full classname takes into account the module name automatically.
   

   class name

5. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterEventListenersPass.php
   @@ -31,6 +69,7 @@ public function process(ContainerBuilder $container) {
   +      // @todo Implement sorting by module weight and class name.
   

   Isn't that what's happening above in the compiler pass?

6. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -78,14 +78,16 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface {
       * List of available modules and installation profiles.
   ...
   +   * Its a list of extensions keyed by module name.
   

   These could probably jsut be rolled into one line?

What is this change about? And why not use the module handler (which is assigned below/after atm)?

Well, it uses the kernel as before. Note: We fixed the parameters of updateModules

I pointed out to @dawehner in IRC and also summarized in #52 that this approach is invalid. I've named several reasons, but the most important is that it only addresses one particular case of where definition order influences execution order.

If we really care about definition order influencing execution order, then it is not enough to "fix" the event listener compiler pass. We'd need to apply the same approach on most of the other tagged services also.

This issue has been started in a rage and subsequent discussion was loaded with considerable emotion. Please let us reconsider the scope and the severity of this issue from pure a technical perspective.

Adding triage-needed tag. I personally think we should see if #52 is doable.

If we can't find a good resolution here, then we at least need to clearly document somewhere that the order of the services in the file affects the order of execution and get that clarified upstream.

Adding triage-needed tag. I personally think we should see if #52 is doable.

The main difference is that other tagged services are basically always independent, in contrast to our various event subscribers

I bet upstream would say: use a priority if the order of execution matters.

I'm sure upstream would say that, they don't have a module system though.

Thinking about this more, I think we should do the following:

1. Document somewhere that this is the behaviour so at least it's somewhere other than this issue.

2. Downgrade this to major, but if we can fix the ordering I'd probably allow that to happen right up until release candidate since any pain from contrib modules then would be less than people are going to run into later.

Adding RC deadline tag, don't think we want to change event order after release.

tagging for review during critical hours today

+++ b/core/tests/Drupal/Tests/Core/DependencyInjection/Compiler/RegisterEventListenersPassTest.php
@@ -0,0 +1,202 @@
+    // @FIXME Introduce the weight of the modules.

Still needs doing? Looks like the test includes module weights so perhaps old comment?

I don't see that 'service_collector' tag is being addressed yet - is that in scope here or not?

I don't see that 'service_collector' tag is being addressed yet - is that in scope here or not?

Can you describe what you mean by that point?

Still needs doing? Looks like the test includes module weights so perhaps old comment?

You are right, in testProcessWithMultipleSubscribersWithModuleWeightAndSamePriorities we deal with it already.

Also renamed from EventListeners to EventSubscribers, from former feedback of @chx

Can you describe what you mean by that point?

So two breadcrumb builders in the same file with the same priority.
I think the answer is use a priority, but not sure how that differs from the overall OP.

So two breadcrumb builders in the same file with the same priority.
I think the answer is use a priority, but not sure how that differs from the overall OP.

Ah I see, well yeah, better use priority. They might also not conflict with each other in the first place though.

Let's do it

Someone can re-roll in office hours

straight re-roll

The latest patch still does not address #59 / #60.

Not sure why this is assigned to me.

Since this issue is no longer critical, untagging for critical office hours. Don't know if we have or should start a "Major Office Hours" or similar tag.

This issue was marked as a beta target for the 8.0.x beta, but is not applicable as an 8.1.x beta target, so untagging.

This sounds disruptive still, so moving to 8.2.x.

Updating tag.