We have been slowly adding listeners to various kernel events over the last 2 years. That's fine, but as event subscribers register their own priority order it's not immediately obvious what the current order is, or if it's optimal.
Let's audit all kernel event subscribers and determine if we should rearrange the priority of any of them. In some cases, we may want to also move some post-routing request listeners to the controller event, per http://symfony.com/doc/current/components/http_kernel/introduction.html
After the controller callable has been determined, HttpKernel::handle dispatches the kernel.controller event. Listeners to this event might initialize some part of the system that needs to be initialized after certain things have been determined (e.g. the controller, routing information) but before the controller is executed.
Note: This issue is just about rearranging where certain actions fire. Changing what those listeners are doing is out of scope for this issue (except for syntax changes needed for changing the event, etc.)
1) Build a list of all request, controller, view, response, and terminate kernel event listeners/subscribers. That should be included in this issue summary, and potentially in a handbook page later.
2) Determine if we should move any of those listeners, either to change their priority or move some from request to controller.
3) Move anything we decide needs moving.
Any contrib modules that have a request listener with a sensitive priority MAY need to adjust as a result. However, that in most cases will be a one character change.
Comments
Comment #1
Crell commentedOf note: The router is currently request event, priority 32. 32 was chosen because that's what Symfony fullstack uses, and using that avoided a bikeshed discussion when the router/kernel were originally added. It has no special significance beyond that, so if we want to change it for some reason we can. (Although if there's no special reason to, having the same priority as Symfony fullstack is convenient.)
I think the initial audit here is a novice-friendly task.
Comment #2
dawehnerYeah you should be able to pretty much extract the data from webprofiler module
Comment #3
wim leersNote also that #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) has to change some event subscribers' priorities in order to be able to do what it needs to do, because there is no gap left between them. A long time ago, it was assumed (for good reasons) that surely nothing ever would need to sit between them, but that was wrong. Let's fix that, and not make that mistake again :)
Comment #4
dawehnerI'm curious whether we should choose a range and then spread the core subscribers equally.
Comment #5
Crell commented@dawehner: Now that we have (probably?) a final list of the listeners we'll have (although it sounds like Wim is adding one), that's reasonable to try and do. Certainly better than our previous strategy of "make this one 100,000 so it's really first!!!" :-) Say, space them every 10?
Comment #6
wim leers10 seems sufficient, but 20 seems more prudent?
Comment #7
dawehnerWhat about 3.14?
Comment #8
wim leersLet's go with 1.4142135623731.
Comment #9
lussolucaI've done a quick patch to Webprofiler to also collect event priorities: #2513814: Add a priority column to the events list
Comment #10
wim leers@lussoluca++
Sorry for the distraction in #6/#7/#8 — that's my fault. 10 is fine. Let's just do this.
Comment #11
wim leers@larowlan had an interesting idea over at #2429617-310: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!):
Comment #12
jibran+1 for the idea.
Comment #13
yched commentedSomeRouteInterface::FORM_WRAPPER_DERIVATION - 1:Sounds interesting in theory - but then in practice which are the ones worthy of having a constant, vs the ones that just position themselves around those constants, without defining a constant of their own ?
Comment #14
jibranWhy not define constants for all in core?
Comment #15
wim leers#14: we can't have constants for everything in core, because some would be defined as
OTHER_CONSTANT - X, withOTHER_CONSTANTalso depending on another constant, etc.That's equally unmaintainable.
Symfony's event system should not be using numeric priorities in the first place, but
beforeandafterrelationships instead. Then we wouldn't have this problem.Comment #16
dawehnerWell, on the other hand using before/after adds another level of coupling you ideally don't want to have. Ideally the numbers would outside of the actual executed class,
see #2023613: Move event registration from services.yml to events.yml
Comment #17
wim leersCan you give an example of before/after being problematic?
In CSS/JS asset handling, we used weights in D<8. That caused nothing but pain and trouble. Thanks to dependencies (which are effectively "after" relationships), that's now mostly gone (since few things need "before" relationships, at least in the world of assets). You just need to explicitly before/after which specific things you want to run.
I don't see — yet — why that wouldn't also work for event subscriber ordering.
Comment #18
Crell commentedWe've discussed before/after in the past. The graph dependency resolution it needs is non-trivial. Remember also that EventDispatcher allows you to add new listeners at any time, so complex precompilation as a requirement for functionality is a non-starter, and integer ordering is actually quite powerful. (Certainly better than module weights.) Also, many listeners don't actually care what order they're called in. The order is at best a performance optimization.
But all that is off topic. *All* we're doing here is auditing the current listeners. We just need someone to do that, or write the script to do that. No more. Any changes to how we define listener order are off topic.
Comment #22
Torenware commentedThis is an "auditing" task. Look at the issue summary, and do the first item to find out who is using what events (probably by searching in your text editor). Good novice issue if you're comfortable with PHP but new to Drupal contributing.
Comment #23
benjifisherComment #24
dawehner#2352351: Add before/after ordering to events provides some dependency based priority system.
Comment #26
ivan berezhnov commentedComment #36
quietone commentedIs this a task that still needs to be done?
Comment #37
quietone commentedSince there hasn't been confirmation, in the past 9 months, that this still needs to be done. I am closing this issue.
If there is work to do here, then either re-open the issue or open a new issue and reference this one. If the choice is to use this issue then add a comment change make sure to change the issue status to 'Active'.