Updated: Comment #N
Problem/Motivation
To register event subscribers currently, you have to add a service with the "event_subscriber" tag.
Considering that we don't use tagged services for discovery consistently in core, this is exposing an unnecessary implementation detail.
I think we can improve DX by adding a dedicated events.yml with a simple schema for registering events and subscribers.
Something like this:
namespace Drupal\rdf;
final class RdfMappingEvents {
const MAP_TYPES_FROM_INPUT = 'rdf.map_types_from_input';
}
namespace Drupal\rdf\EventSubscriber;
use Drupal\rdf\RdfMappingEvents;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
class MappingSubscriber implements EventSubscriberInterface {
static function getSubscribedEvents() {
$events[RdfMappingEvents::MAP_TYPES_FROM_INPUT] = 'mapTypesFromInput';
return $events;
}
}
services:
rdf.mapping:
class: Drupal\rdf\EventSubscriber\MappingSubscriber
tags:
- { name: event_subscriber }
Proposed resolution
Could be something like this:
events:
- rdf.map_types_from_input
subscribers:
rdf.map_types_from_input: Drupal\rdf\EventSubscriber\MappingSubscriber::mapTypesFromInput
Or generically speaking:
subscribers:
config.importer.import:
- callback: config_snapshot_subscriber:onConfigImporterImport
priority: 40
config.importer.validate:
- callback: Drupal\Core\EventSubscriber\ConfigImportSubscriber::onConfigImporterValidate
priority: 40
config.save:
- callback: config.factory:onConfigSave
priority: 255
For dependency injection we can use the ContainerInjectionInterface
Remaining tasks
User interface changes
API changes
Beta phase evaluation
Issue category | Task because it improves DX |
---|---|
Issue priority | Major because it affects the way of all event subscriptions |
Disruption | Disruptive for core/contributed and custom modules/themes because it will change the event subscription logic (from tag:event_subscriber to events.yml) |
Comment | File | Size | Author |
---|---|---|---|
#131 | interdiff.txt | 546 bytes | dawehner |
#131 | event_subscriber-2023613-131.patch | 102.21 KB | dawehner |
Comments
Comment #1
tsphethean CreditAttribution: tsphethean commentedI can take a look at this.
Comment #2
dawehnerI fear this would let people move logic into their event subscribers, but yeah we can't prevent that anyway.
Comment #3
msonnabaum CreditAttribution: msonnabaum commentedI dont see how the current way discourages that.
Comment #4
yched CreditAttribution: yched commentedI'd be very very +1 to that.
We don't have that many events so far, and possibly wont have much in D8 (except contrib can still go wild on them), but if we did, the current human discoverability of "who reacts to what" is terrible. An easy to find registry would help a lot when you are trying to figure out how a random contrib / custom module works.
Comment #5
dawehnerWell, basically all IDEs has built in support for finding events: Just search for the used places of the constant.
Comment #6
tsphethean CreditAttribution: tsphethean commentedSo here's a very early stab at this, which moves event subscription for user and views modules into module.events.yml and parses them during the building of the container. Tests seem to be passing on my local.
@msonnabaum - couple of things that I wanted to clarify, in the example module.events.yml you posted there was:
was this intended to register the event, or the subscription to the event? I couldn't figure out why this would be needed for registering the events?
In this patch I've also continued adding the event_subscriber tag to the service definition for the container, so that RegisterKernelListenersPass wouldn't need modifying (it continues to add listeners by looking for tagged events in the container, it's just now we'd force the adding of the tag in the YamlFileLoader::parseEventSubscriberDefinition method) , but I don't know if this is desirable?
Also the current implementation would allow for the content of a module.events.yml to be put in a module.services.yml and still working, because I'm modifying the Yaml load and validate methods - am I better off extending the YamlFileLoader to a YamlEventFileLoader class so we separate the logic?
Grateful for any thoughts.
Comment #7
tsphethean CreditAttribution: tsphethean commentedComment #8
Crell CreditAttribution: Crell commentedI really don't see how #6 buys us anything. It's the same YAML definition; all we've done is remove the tag from it and move it to a new file, which then effectively becomes a duplicate of the services.yml file but with everything having an event_subscriber tag.
I don't see any DX benefit here that is worth the confusion that will inevitably cause. For this to be useful, we would need an entirely new event registration mechanism separate from EventSubscriberInterface. But I don't see a great deal of value to that for the confusion it would cause with other Symfony-based projects, to say nothing of the confusion within Drupal of yet another way to do things, and the work that will inevitably be required when someone decides that "Drupal must be consistent" so we have to rewrite very frickin' event subscriber we have.
Really, I don't see the point.
Comment #9
msonnabaum CreditAttribution: msonnabaum commentedI appreciate the effort, but I agree that #6 isn't buying us much.
The attached patch is more what I had in mind. It's really just exposing the ability to use symfony's event listeners rather than subscribers. They are simpler, require less boilerplate, and more closely resemble event dispatchers in other languages/frameworks.
I converted a handful of events to show how it would look. The syntax supported is the same as controllers: you can specify class::method or service:method.
Most subscribers are registered as a service soley to declare they are subscribers. These service entries are removed and they use the simple class::method notation in events.yml.
Some listeners are complex enough to warrant needing a service entry, and that's fine. This patch just decouples those ideas. It may be an extra step for the listeners who will need services, but I think they are the minority and the simplicity is worth it.
Many of the subscribers that have dependencies only have the dependency because we strictly separate every subscriber into a dedicated object. Then the subscriber's method simply delegates directly to the dependency. For the simpler classes like ParamConverterManager, I dont think it buys us much to do this. As an example, I removed ParamConverterSubscriber and just registered ParamConverterManager to handle the one event.
I dont think we should do this across the board, but I think we should in the simpler cases. It's a nice example of how contrib can avoid even more boilerplate.
Comment #10
msonnabaum CreditAttribution: msonnabaum commentedAlso, I would have preferred to avoid the compiler pass, but we dont really have a choice as long as we're using a compiled container. An event listener who's callable is an instance of a class with a method can't be dumped, so I had to create services for them dynamically in the pass.
Comment #12
Crell CreditAttribution: Crell commentedI'm confused. #9 now has a combination of event subscribers, event subscribers not tagged as event subscribers, and listeners registered only through the events.yml file. Why 3 different patterns? Even I can't figure that out, which means I doubt most module devs will either.
Comment #13
msonnabaum CreditAttribution: msonnabaum commentedForgot to add the yaml discovery stuff from #2050289: Add class to make yaml discovery reusable, which this patch depends on.
Comment #15
msonnabaum CreditAttribution: msonnabaum commented@Crell - The final patch would have everything in events.yml as event listeners unless we found a compelling reason to leave some as subscribers. The ones that are there and not tagged are simply services that need their class name changed and I didnt bother.
The question of whether to keep support for tagged event subscribers is up for debate, but I don't think it should be what we encourage contrib to use.
Comment #16
chx CreditAttribution: chx commentedThe win would be to move the whole of getEventSubscribers method into yml and compile the thing in a way that we have per event arrays right in the container so that in case of an event the poor system doesnt need to call every subscriber.
Comment #17
Crell CreditAttribution: Crell commentedMark: It's not up for debate. Even if we have a different preferred mechanism I see no reason to remove functionality that is common in Symfony, especially as that's an API change at this point.
chx: We could do a compiled EventDispatcher without using a yaml file. I did some skunkworks code on that last year; a compiled dispatcher based solely on subscriber services is totally doable.
Comment #18
msonnabaum CreditAttribution: msonnabaum commentedSure, I dont care if we leave it in.
Comment #19
effulgentsia CreditAttribution: effulgentsia commentedI agree that this is much better DX. I'd love to see this get in if it can be done without breaking BC, which seems like it can be.
Just a minor q: do we need the "subscribers" key in the events.yml file? The issue summary includes an "events" key as another possible top-level key, but #13 doesn't have it, and I'm not clear what the utility of that would be. And if there's only one possible top-level key in the file, then why not drop it?
Comment #20
effulgentsia CreditAttribution: effulgentsia commentedComment #21
tim.plunkettFixing tags
Comment #22
neclimdulmindless reroll. no changes just fixing conflict with entity services renames and the yaml discovery going in.
letting the trade offs sort of sink in still.
Comment #23
xjmDiscussion notes from Prague:
https://docs.google.com/a/acquia.com/document/d/1j72KgYFjGZQQWWAw1RPvLFy...
Comment #24
webchickAnd I think we decided the win here makes this at least major.
Comment #25
xjmDiscussed on the phone with @Dries, @webchick, @msonnabaum, @tim.plunkett, and @alexpott -- this should be considered a beta blocker given the DX impact.
Comment #26
dawehnerStarted some rerole.
Comment #27
damiankloip CreditAttribution: damiankloip commentedHang on, so to quote Dries in #2145041: Allow dynamic routes to be defined via a callback:
This totally changed what was (not anymore, not an event) the most common event used, yet this is deemed critical now?
Also, we are trying to improve DX here - What about the fact that we just rely on keys defined in YAML with this patch, which you could very easily typo. At least with using event constants you would get a fatal error straight up. This is a big reason for symfony using them. Just saying..
Comment #29
webchickYeah, I am not sure why this was moved to critical. What we discussed was making it a beta blocker because in terms of DX, the win here is actually quite large (compare the before/after code), and if we change it after beta we are not providing a stable API.
Restoring priority, keeping the tag.
Comment #30
Crell CreditAttribution: Crell commentedAs long as it's confined to just adding a simplified channel for registering events, it's not even beta-blocking as it's a pure API addition. It could be added at any time. Moving existing subscribers from services.yml to events.yml shouldn't be an API change either, since they all still end up in the container anyway.
If we're talking about removing support for registering subscribers via services.yml, then that's a huge and completely unnecessary API change that per #17 I firmly and absolutely oppose.
Comment #31
dawehnerAdded a bunch of unit tests, converted a lot of existing event_subscribers and fixed a bunch of issues which came up while manual testing.
Comment #33
dawehnerA space can change a lot.
Comment #35
dawehnerComment #37
dawehner.
Comment #38
jibranSome doc issues.
Finds
doc block missing.
Comment #39
jibranWe are done with the consideration. we are implementing it now. :)
Comment #41
dawehnerIf you look at the current patch you will realize that most of the event subscribers do have dependencies so you will end up with
one entry in the
.services.yml
file as well as one in the.events.yml
file:This requirement for 2 different places feels wrong and also not really improves the DX much, too be honest.
It would be cool to be able to do everything in the services.yml file. Symfony allows that by default using the following syntax:
we could do this a little bit more generic using event_subscriber, event (maybe skipping event_subscriber could even work).
Comment #42
Crell CreditAttribution: Crell commented#41: That would only work for subscribers that have only a single listener, no?
Comment #43
dawehner>
No, there is no problem with using multiple tags with the same name.
Damnit, I should have copied the full example from symfonfy (http://symfony.com/doc/current/cookbook/event_dispatcher/before_after_fi...):
Comment #44
dawehnerComment #45
dawehnermsonnabaum explained that we not use services but the ContainerInjectionInterface (adapted the issue summary for that).
Comment #46
Crell CreditAttribution: Crell commentedI believe (but could be shown wrong wrong) this means that two listeners in the same class would not fire on the same object; they'd be in 2 separate objects. That means they couldn't share data properties. While an argument could be made that's good (as well as a case for allowing shared object properties for cache purposes or avoiding duplicate shared logic), it's certainly not at all obvious.
Comment #47
msonnabaum CreditAttribution: msonnabaum commented@Crell - That is documented well in the symfony docs on listeners vs subscribers. We shouldn't default to subscribers, but those who need them won't have trouble finding them.
Comment #48
dawehnerWe register a service in the background, so it should be possible to only have one instance at the end.
Comment #49
damiankloip CreditAttribution: damiankloip commentedThat's if you have a service. What about if you just use the dependency injection interface hack? :)
Comment #50
msonnabaum CreditAttribution: msonnabaum commentedI think there was a miscommunication in #45. I'm not suggesting we remove any services with dependencies. It's fine to just use the existing single ":" syntax for those callbacks.
Yes, you now have to edit two files, but I dont think that's a bad thing necesarilly. Creating a service and wiring up an event listener are two totally separate things Yes, they sometimes go together, but not always. When you don't have dependencies, creating a service just so you can listen to an event is considerably more awkward, so I think the tradeoff is reasonable.
Also, in many cases, creating a class soley to listen to events will be overkill, so you can just set the callback to a method on what you would normally inject into the subscriber. I'm not suggesting we do that here though. I'm not particularly interested in that fight.
Comment #51
dawehnerWasn't the point of the issue to improve the DX? This is worse DX from my point of view, especially if we could it do better.
So good so far, though just the following ones in core.services.yml won't have some: AjaxSubscriber, AjaxResponseSubscriber. All other ones have dependencies.
Comment #52
effulgentsia CreditAttribution: effulgentsia commentedWhat docs is that? From my read of Symfony docs, it seems that "listener" means either a callable or a class/object that does not implement getSubscribedEvents(), and that "subscriber" means a class/object that does implement getSubscribedEvents(). That seems orthogonal to me to whether an object gets reused or not, declared as a service or not, etc. Speaking of which, since this patch removes that method from all the *Subscriber classes, shouldn't we rename them to *Listener as well?
So currently in this patch, we have *.events.yml files with a single top-level key of "subscribers". What if we change that to "listeners", but also add a "services" top-level key, and allow service definitions of listeners that have dependencies in there?
Comment #53
dawehnerIn order to allow that we would have to support the full container syntax. In core we already have services with sychronized method based injection. I hope we can easily reuse the existing yamlLoader functionality.
Comment #54
Crell CreditAttribution: Crell commentedWith subscribers today:
- You edit services.yml
- You edit your code file
That's 2 files.
With events.yml as proposed here:
- You edit events.yml
- You edit your code file
- You may or may not edit services.yml
That's 2-3 files.
So... what's the win here? Adding another way to register events adds more conceptual overhead for developers, since they have to be aware of another way that a module they're working with could be registering events. It's also more effort to document, write books about, etc. There needs to be a trade-off benefit for that. And so far, I'm not really seeing it.
In the generic sense, "sticking stuff in services.yml" translates to "You write your class, then you use services.yml to tell Drupal all it needs to know about your class. The rest is magic." Using events.yml doesn't change that basic equation, even if it does move it around a bit.
Comment #55
effulgentsia CreditAttribution: effulgentsia commentedThe "magic" boils down to tags. Without tags, there's no magic: you just declared a named service that doesn't automagically do anything until some code explicitly instantiates it. But tags create automagic behavior.
The issue summary says:
I think there's some truth to that: we've made up a bunch of service tags (in addition to the ones Symfony made up), and each one creates automagic in its own special way, so module authors need to learn the quirks of each one that they need to interact with.
Here are the tags currently being used by core modules, followed by the modules that define at least one service with that tag, ordered by the number of modules:
I think that especially for the highly popular tags (access_check, event_subscriber, maybe the next couple as well), it's worth asking whether:
a) they really need to be services in all cases, rather than lightweight callables in the majority/simple cases.
b) it's better DX for their definition to be buried with everything else in *.services.yml, or whether they're common/important enough buckets of Drupal to warrant their own YAML file.
That's not really an answer to #54, just additional background on the question.
Comment #56
msonnabaum CreditAttribution: msonnabaum commented@Crell - Number of files != complexity. Also, that's completely ignoring what goes *in* the files.
+1 to #55. I don't really know how to argue this issue anymore than I have previously. I just don't see why we should accept that events (what should eventually replace hooks), should be coupled to a service container.
DX isn't about the number of files you edit or the number of characters you have to type, it's about simplicity. My proposal is objectively simpler.
Comment #57
yched CreditAttribution: yched commentedI agree with #56.
The argument IMO is that
"ease of discovery for a human person evaluating a module on git.drupal.org > module author having to write one additional entry in a yml file"
It's easy to check which hooks a module implements, and that makes a large part of understanding what a module does and how it does it. That discoverability is lost with events, where it becomes much more complicated to figure out who reacts to what.
I guess there can be runtime devel tools that spit this out for you, but that only works if you download and install the module...
Comment #58
Crell CreditAttribution: Crell commentedIf you're developing with Drupal, you're downloading and installing a module anyway. That's what devel is for.
I am not convinced by #56's claim that a separate file is "objectively simpler". Certainly it also has to be weighed against the "yet another way of doing the same thing" problem it creates, which does have mental overhead for developers downstream. (i.e., you can't rely on events.yml being the canonical source for all event registrations; even if we ripped out support for tagged-service-registration, which I'd be against doing, listeners can still be registered at runtime anyway, by design.)
Too, if the real issue that is being objected to is that tagged services as a concept are too complicated, then a solution should address tagged services in general, not just events. Per #55, we have as many if not more access checker services as we do event listeners, at least in core. Whether that is the same in contrib or not I don't think any of us can predict at this point. Why are events so special, then, rather than all of the other common tagged services?
I would actually recommend we punt on this question. We have a lot of really new systems and mechanisms in D8. Let's let people try them out in the wild and see if, in practice, putting event listeners in services.yml is as obtuse as some fear it will be. I don't think it will be, in practice, as most modules won't have the zillion services that core.services.yml does, but I could certainly be wrong about that. But let's ride on the one file, one mechanism approach for a bit and get some real-world usage of it.
Then if we find that tagged services really is a big DX problem -- even with documentation, training, and a little experience -- we can add a more universal tagged services alternative in 8.1 or 8.2, with more experience to guide that decision. We have this new release cycle process coming in Drupal 8 for a reason, and this sounds like exactly the sort of situation it can help with.
Comment #59
msonnabaum CreditAttribution: msonnabaum commentedNo claim was made that "a separate file" is simpler, I said the proposal was simpler. It's the difference between specifying an event name and a callback, and registering a service (a completely unrelated concept), with a special "tag" (a symfonyism that won't be familiar to anyone outside of that community), then implementing a specific interface with a static method in addition to the callback.
I cannot imagine an argument that the former isn't simpler.
If we did this, we should use events.yml throughout core as an example of the simplest method. I would guess that this would be by far the most popular method, leaving the tagged service to only those who *really* like that method. I doubt it would be used enough to cause confusion.
Also, this is more an argument to get rid of event-like hooks than methods of registering events.
Tagged services aren't being objected to, I'm simply arguing that they should not be used for high level APIs that the majority of custom modules and contrib will be interacting with.
We've done a pretty bad job thus far of converting event-like hooks to events, which is why I believe there hasn't yet been much concern over this, but when we do convert some of the big ones, it'll be a huge difference in complexity and verbosity. So yes, there are more access checkers, but that's because we haven't converted enough hooks yet.
To illustrate the impact I'm describing, here are some examples showing how flag_node_save would work (yes, I know there are no entity events, but there *really* should be for D8, and most certainly will be for D9).
D7
D8, only method supported currently
flag.services.yml
D8 with proposed patch, using functions
flag.events.yml
D8 with proposed patch, using a class
flag.events.yml
D8 with proposed patch, using a service
flag.events.yml
flag.services.yml
Now you might be saying to yourself, "Why would we support functions and classes with static method calls, they are forbidden!" My point in showing these three versions is to point out that enforcing a coding style is none of core's business. The current method forces us to use services, when events have nothing to do with services. Even if your listener has dependencies, you dont need to use a service! The question of whether or not people should be registering their listeners as services is irrelevant. There's simply no reason we need to enforce a "best practice" at the API level. This is the kind of thing that makes people angry about D8.
Hopefully the examples above illustrate why this is not something to wait on.
Comment #60
yched CreditAttribution: yched commentedI strongly disagree. With my site builder hat on, I spend a lot of time on git.drupal.org looking at modules code. That's much more revealing that reading the project page, and a couple minutes are often enough to reject the module or take it for a live evaluation.
If D8 means you cant easily understand how a module is structured and plugs with which parts of the system by looking at its code, but instead need to actually download and run it locally so that some drush tool can analyze it for you, I'd say we have made a step backwards. Human discoverability is a feature.
@msonnabaum's examples in #59 are pretty telling on that aspect.
Comment #61
dawehnerMy proposal earlier:
Comment #62
Crell CreditAttribution: Crell commentedWhile I'd be OK with #61, I don't think that addresses msonnabaum's issues. Which, if I could simplify (Mark, correct me if I'm misstating), boil down to "events aren't services so shouldn't be in the services.yml file".
While I agree that an event listener doesn't really fit the canonical definition of a service (semi-global stateless single-purpose "doer of a thing"), the DIC we're using, in practice, handles more than pure services. Parameters aren't really services; All tagged-service use is not, strictly speaking, pure-service-logic; the current user object isn't really a service; the request as a service certainly doesn't make sense by that standard, etc. That is, the Container object we have is already being used for far more than pure-services; Event subscribers are the least of the offenders in that regard.
I'm sure this is going to sound like a classic logical fallacy ("one person did it wrong so it's OK for everyone to", whatever that fallacy is called), but my point is rather that it makes little sense, to me, to purify the container in one area but not in all the others that are, actually, worse offenders. That's inconsistent, and I believe would lead to worse DX overall. And, for better or worse, the DIC we're using actively supports these ancillary uses.
As a side note:
I agree 1000%, and am quite disappointed that we backed off on converting node hooks to events. That's such an obvious place to do so I think it was a mistake not to, but at this point in the dev cycle I don't think Dries and Co. would authorize any more such direct conversions.
Comment #63
msonnabaum CreditAttribution: msonnabaum commented@Crell - Wrong logical fallacy. I think you got straw man there :)
It's not about putting things in the DIC that aren't "pure services," it's about using the DIC as a discovery mechanism rather than a DIC.
Comment #64
msonnabaum CreditAttribution: msonnabaum commentedAlso, #61 makes the situation worse. Simplifying the registration of events means reducing the concepts and APIs involved, not reducing the number of characters you have to type.
Comment #65
dawehnerI totally agree that typing is not a proper metric for measuring DX.
What I would really hate as a developer though would be to put information into 3 different files, which you basically would all need, as 80% of all events need stuff injected. You first look into the events yml file, then you go to the services yml file to actually know which class is involved vs. just look up the stuff in the services yml file. I really can not see how your approach involves less concepts.
Comment #66
msonnabaum CreditAttribution: msonnabaum commentedThat argument could be used to say that nearly all forms of discovery should be tagged services, but surely we're not going to do that, right?
Again, having dependencies != needing an entry in the DIC. I know we've been dogmatic about it in core, but it's simply unnecessary in many cases. We can't come up with a decent estimate based on core, because as I stated before, we haven't yet converted some of our most obvious use cases. 80% is just false, we dont know yet.
My approach involves fewer concepts because it doesn't *require* a service, just as creating a controller or a views plugin doesn't require creating a service. It's an unnecessary concept to couple to discovery.
Comment #67
neetu morwani CreditAttribution: neetu morwani commentedComment #68
dawehnerI guess you are right in the case where we use events in many many places, though I still wonder whether this is worth to make it hard for the good-practise one.
Comment #69
XanoIf this file is for event listeners and not for events, can we please name it accordingly? We don't want to end up in a situation where we are working on exposing actual events and then realize we shot ourselves in the foot in January 2014.
+999 for the general idea, though :)
Comment #70
xjmThis is either critical, or not a beta blocker. Which? :)
Comment #71
msonnabaum CreditAttribution: msonnabaum commentedAt this point I'm just waiting for someone like webchick to see #59 and freak out.
Comment #72
msonnabaum CreditAttribution: msonnabaum commentedAt this point I'm just waiting for someone like webchick to see #59 and freak out.
Comment #73
effulgentsia CreditAttribution: effulgentsia commented@xjm: there isn't consensus on that. See #25 and #29.
There also isn't consensus on what the solution should be, because both #59 and #68 are good points. I'm still in favor of msonnabaum's proposal, but don't really know how to address #68 yet.
Comment #74
dawehnerI really fear that using the events.yml approach pushes people towards using \Drupal for events and or not writing decoupled components.
Comment #75
msonnabaum CreditAttribution: msonnabaum commentedAgain, the argument in #74 could be used to argue that *all* discoverables in Drupal should be tagged services, and we clearly aren't doing that.
The event system should have no opinion about coupling because it's not the event system's concern.
Comment #76
dawehnerWell, the difference is that the other discoverables aren't base components but regular swappable pieces.
Do you want to let subscribers just use services and defer the logic?
Comment #77
msonnabaum CreditAttribution: msonnabaum commentedI have no opinion on what people decide to do with their subscribers. I only care that we can register a callback for an event.
Comment #78
effulgentsia CreditAttribution: effulgentsia commentedI think what we want to optimize DX for is modules (both core and contrib) rather than for core/lib. With that in mind, here's the 9 current event subscribers in HEAD provided by core, non-test modules:
What we have is 3 subscribers with no dependencies, 3 with dependencies only on services defined in core.services.yml, and 3 with dependencies on a module-defined service (in all cases, a service defined by the same module).
My understanding of #2134513: [policy, no patch] Trait strategy is that once we fix bot to run PHP 5.4, we'll be able to write traits with getter and setter methods for all services in core.services.yml (we don't have to do all of them, but we can choose to do all the ones that make sense), where the getter method could return the service that's in
\Drupal::
if the setter method hasn't been called, and that there's nothing about this that isn't "good practice", because you can still unit test the class by having the test invoke the setter method.This would then potentially mean that 6 of the 9 subscribers have no need for dependency injection, and therefore, no need for an entry in *.services.yml.
If we manage to figure out #1972304: Add a HookEvent, then contrib will also be able to implement listeners for hooks like hook_node_view() / hook_node_view_alter(), which very often also don't have dependencies or only have dependencies on core services that could be accessed through traits.
So, I think what this issue represents is much better DX for event listeners that have no need to be in services.yml, and only marginal inconvenience for ones that do. And while it's hard to know this with any precision, based on the above, I could easily see >50% of good-practice contrib event listeners being in the former camp.
So, for me, this issue is an overall improvement. @dawehner: any thoughts on this reasoning?
Does my claim about traits address this concern? If not, do you also have this concern about route controllers, and if not, why the difference?
Comment #79
Crell CreditAttribution: Crell commentedeffulgentsia: I consider the "service traits" we are planning to add to be "application level only". (Controllers, some plugins, etc. Things that have utility base classes now.) Domain logic classes and infrastructure classes should stick to "normal" injection. Which then begs the question of whether event listeners are application level logic or not. My gut feeling is that it will vary a bit with the event listener.
Comment #80
effulgentsia CreditAttribution: effulgentsia commented@Crell: my opinion is that the 3 subscribers in #78 that say "depends only on core services" are application level. Do you agree or disagree?
Comment #81
Crell CreditAttribution: Crell commentedThe field_ui and views Route subscribers, probably. The Maintenance Mode subscriber is, I think, highly debatable whether it's application level or framework level.
Comment #82
dawehnerTraits are a really good tool for services which don't really add business value. One good example is the translation manager. In case you have a service you really should better explicit define your dependencies.
Rerolled the patch before the suggestion, people seem to disagree, this is fine.
Comment #84
dawehnerJust a couple of fixes. Note: we have to support optional services
Comment #86
catchThis is either not a beta blocker or it should be critical, can't block the beta on non-critical stuff.
Switching to beta target - if you think it should block, please bump to critical.
Comment #87
effulgentsia CreditAttribution: effulgentsia commentedReroll.
Comment #90
dawehnerAdditional two expanding the documentation this also fixes quite a bunch of the failures, hopefully.
Comment #92
dawehner.
Comment #93
tim.plunkettContains \Drupal\
Additionally
I think we can link to #2165475: Provide a generic class resolver here? And @todo
Changes like this make sense to me.
This, not so much. Where is the corresponding change?
This is pretty rough for services, since it means a third place to look for things...
Comment #94
dawehnerLet me quote from above.
Sadly the assumption of using traits for DI might not be a proper one. Personally I would like to just give people possibilities not fences.
Comment #95
effulgentsia CreditAttribution: effulgentsia commentedIn #81, Crell said that the field_ui and views route subscribers are probably application-level, rather than infrastructure or domain level. The former currently has an entity manager as the only constructor param, and the latter has entity manager and state. I think we're agreed in #2134513: [policy, no patch] Trait strategy that EntityManager and State will have traits. If that's the case, and if we're agreed on these subscribers being application level, then does that mean we can/should use the traits instead of the constructor args in them?
The patch doesn't prevent contrib from using the 'event_subscriber' tag if they want to, right? So then the question is whether we promote that as a "Drupal Approved (TM)" practice (and use it in a couple places in core) or as something we prefer people to not do. The benefit of not doing it in core, and asking contrib to not do it as a general rule, is easier discoverability (for humans) per #57. Any thoughts on how to strike that balance?
Comment #96
effulgentsia CreditAttribution: effulgentsia commentedHow about this?
- For subscribers in core/lib, we stick to what's in HEAD: i.e., an 'event_subscriber' tag and the static registration method inside the class.
- For core/modules, we generally use events.yml, unless there's a really good reason not to (TBD what those reasons might be). In cases where we don't use events.yml, we place the subscriber class in
Drupal\MODULENAME\EventSubscriber\
, which makes it easy for a human to discover all of them.Comment #97
effulgentsia CreditAttribution: effulgentsia commentedWhen we remove the static method registration in the class, they go from being subscribers to listeners. So I think we should change the top-level key in the YAML to
listeners
. Or, per #69, perhaps rename the file to*.event_listeners.yml
, and then remove the top-level key entirely?Comment #98
msonnabaum CreditAttribution: msonnabaum commentedThat's one way to look at it. Another would be, you now have a sensible place to see what events a module subscribes to, rather than it being in services, which is not at all obvious, considering we still use hooks, plugins/annotations for discovery. An argument to leave events in services.yml for discoverability is an argument to move practically all discoverables to tagged services.
Also, I still find that any talk about DI is a distraction and not relevant to the issue.
Comment #99
dawehnerOh yeah, this is certainly an error. Can we please talk about whether we need to be able to keep the RouteSubscriberBase?
Here is the latest patch
Comment #100
dawehnerAnd the interdiff because drupal.org does not allow me to upload two files at the same time, for some reason.
Comment #102
Dries CreditAttribution: Dries commentedLooking at Mark's example in #59 (link: #2023613-59: Move event registration from services.yml to events.yml), this looks like a fantastic change. It gets us from 'this is really hard' to 'this is great'. I don't believe this has to block beta, but I would really love to see us ship with this in Drupal 8.
Comment #103
dawehner.
Comment #105
dawehnerThe other ones aren't reproducable, let's have a look whether they work this time.
Comment #106
jibran105: event_subscriber-2023613-105.patch queued for re-testing.
Comment #108
effulgentsia CreditAttribution: effulgentsia commentedMerged 8.x, but might have resolved some conflicts incorrectly.
Comment #110
dawehnerFixed hopefully many of the missing pieces of the reroll.
I still think that it is a huge DX regression to require to touch 3 files in most usecases. I really think that core modules covers the common/recommended way to achieve stuff.
Just some small stats:
We do have 53 services and 19 callbacks including core and 18, 10 excluding core.
Comment #112
dawehner110: events-2023613-110.patch queued for re-testing.
Comment #115
dawehnerOn top of that the uploaded patch fixes the trailing whitespaces.
Comment #116
sunI've read the entire issue and discussion (which, fwiw, could have stopped around #20, since arguments were only repeated later on).
My impression of the current state is that @Crell still disagrees that the proposed change is an improvement, @msonnabaum mostly argues on semantic grounds, and some others like or dislike it because of DX (in either direction), and some more people didn't state anything that would help to evaluate their feedback (including @Dries).
What I like about the proposed change:
Single registry (per extension) that lists event listeners including priority.
Supports callbacks both in classes and services.
What concerns me about it:
New, additional way for doing the same thing. (defeats 1. above)
To get dependencies properly injected, the class of an event listener has to be registered in two instead of one place.
This custom approach is unknown and thus not documented anywhere outside of Drupal. — Though admittedly, our current approach using
'event_subscriber'
tags doesn't appear to be documented practice either.Various DX concerns on the actual patch/implementation:
events.yml
files is still "subscribers", even though what follows is a list of "listeners". (confusing)events.yml
can also be used to register a subscriber class? (consistency)Considering these pros/cons, I'm ambivalent on the proposed change. The net win/loss in terms of DX and architecture appears to be neutral. I could live with and without it.
Lastly, I was pointed here from https://drupal.org/node/2158571#comment-8703881 after mentioning the following...
...so I actually expected to see a much larger change (than just juggling some definitions/registrations around) — namely:
Replacing all the priorities with "before" and "after" references to other event listeners. If the referenced definitions exist at compile time, then the listeners are Graph-sorted accordingly. If none of the references exist, then the order of a listener does not matter.
I'm not sure whether that is on-topic or off-topic here. But compared to the neutral evaluation result above, that would truly resolve a major architectural problem, and the idea of declaring event listeners in a separate file would inherently make much more sense.
Comment #117
dawehnerWell, from my perspective we could need something like this issue in order to be able to resolve your more advanced plans. We need a central place to change and collect all the subscriber data,
not done on runtime, probably.
Tbh. I would really like to skip the primary key: what is the point of a primary key if there is just one. We don't use XML here.
Comment #118
Wim LeersFYI: sdboyer has been working on precisely that with his asset libraries/Assetic patch. Exactly the same pattern/logic. He might be interested in this issue as well.
Comment #119
dsnopek+1000 on the idea of "before" and "after" for events! @Wim Leers: Thanks for pointing out @sdboyer's work. I think this is the issue: #1762204: Introduce Assetic compatibility layer for core's internal handling of assets
Comment #120
sdboyer CreditAttribution: sdboyer commented@dsnopek - that is the issue, but the graph library is already in core. what may be useful to look at in that patch is the AssetGraph, which demonstrates how 'before'/'after' data can be turned into proper edges at the time a vertex is added to the graph.
fwiw, gliph is designed with speed in mind, specifically so that it is feasible for use at request time - for cases exactly like this. i'd been toying with the idea of making a patch for Symfony that creates something like this upstream, but doing it in Drupal first makes perfect sense.
Comment #121
catchWhile it might be optimized for speed, I very much doubt it'd be feasible to do event listener ordering at request time. Event listeners are a lot more numerous than assets, and they'll only get more so as time goes on.
Comment #122
sdboyer CreditAttribution: sdboyer commented@catch - depends on what's being done, and what can be cached. it's certainly more expensive than a foreach across a linear list. of course, in this case, one might also be able to take it out of the request time context by introducing a step in container building that transforms before/after information into a topologically sorted list. then we're back to linear traversal.
both of our assertions seem to be the kind that are supposed to be tested, rather than used as reasons, absent evidence, for any course of action.
EDIT: memoization is perhaps a better word than caching here. even if the linear list isn't calculated at container build time, state of a graph is easily snapshotted (just hash the containing object's data); that hash can serve as the basis for memoizing the linear tsl.
Comment #123
msonnabaum CreditAttribution: msonnabaum commentedReminder that we've been able to do this with the graph code we've had since D6: https://drupal.org/project/module_order
Comment #124
msonnabaum CreditAttribution: msonnabaum commentedAlso, while it is an expansion of scope, I think before/after is a good idea that could be done in a followup.
Comment #125
sun@catch / @sdboyer: I was told that we have 1-2 use-cases for dynamically added event subscribers/listeners in core right now, but those are extreme edge-cases and do not represent the 99%.
It should be fine to only support dependency-sorted listeners if they're registered at container compile time.
At runtime, after compilation, your possibilities are reduced to first/last (append/last being the default), and if that is not sufficient, then you need to manually introspect the full stack of listeners anyway.
In addition, I forgot to mention one major remark:
A proper before/after resolution for event listeners is essentially a newborn incarnation/resolution for the epic, unresolved idea of a "hook registry."
The only reason for why
hook_module_implements_alter()
exists today is the lack of an event listener architecture that has relative dependencies natively built-in.Properly resolving dependencies requires to have a (pre-dependency-resolved) registry readily at hand at runtime. Instead of module_implements cache, the information is precomputed and preloaded instead. There are lots of listeners + hooks, so that requires more memory, and thus negatively affects performance.
Performance was and is the only reason for why the module hook system still exists.
→ Resolve relative dependencies. → Kill
hook_module_implements_alter()
→ Kill hooks.Comment #126
Crell CreditAttribution: Crell commentedScope Creep
Sorry, someone had to say it. Fancy graph ordering of listeners is severely out of scope here. Let's also recall that we've managed with module weights for 12 years. The actual needed use cases of hook_module_implements_alter() are astonishingly few. Even listeners that don't care about order can just do nothing. Those that do already have more flexibility, today, than hooks do.
Also sun, no, the primary reason hooks still exist is we've not had anywhere close to the time to refactor them away properly.
Comment #127
dsnopekSo, I'll concede that this is probably scope creep and probably not something we can do for D8, given that we need to get to -beta and stop changing things so much. Also, I don't know how wide-spread the use cases for
hook_module_implements_alter()
(and hence fancy graph ordering) are, but...I can say that I constantly run into hook ordering issues in work on Drupal distributions (I'm one of the maintainers of Panopoly and do a lot of Open Atrium work) and seem to use
hook_module_implements_alter()
all the time. So, maybe this wouldn't help most module developers in their daily lives, but it would help me in mine. :-)Comment #128
dawehnerYeah this issue is orthogonal to the potential ordering issue.
Comment #129
dawehnerRerolled the latest patch, before the scope creeping began.
Comment #131
dawehnerRemoved the non existing listener.
Comment #133
dawehnerI still like the bit of the patch that it removes some bits of the dependency to the EventDispatcher component for your code.
I doubt though this will happen as it is too much of an API change. Anyone wants to disagree or we can just move this to 9.x directly?
Comment #134
Wim LeersComment #135
xjmTagging as a priority for the beta sprint, and removing redundant tag. Agreed that this is a pretty big API change.
Comment #136
Crell CreditAttribution: Crell commentedNote that Symfony fullstack supports adding event listeners via a subscriber class, as we do currently, or by putting the event-priority-method linkage into tags:
http://symfony.com/doc/current/components/event_dispatcher/introduction....
I don't think we support that currently, but we could. (I'm not sure of the performance implications on when the class loads, though.) That would mean we support multiple first-class ways to register an event, but does then let us use the services.yml file as an all-in-one file for event registration for those who want it.
Comment #137
dawehner@crell
Just in case you want to know that we discussed around that idea, see #2023613-61: Move event registration from services.yml to events.yml, this didn't seemed to be a really liked idea.
Afaik symfony just supports it for kernel listeners. They have separate dispatchers for different concerns, one thing we don't do at the moment.
Comment #138
Crell CreditAttribution: Crell commentedAFAIK there are only 3: The main dispatcher (used for most things), the form dispatcher (which is form-instance-specific, I think), and Doctrine (which doesn't even use the EventDispatcher component as its a stand-alone system). It's actually rather annoying that Doctrine doesn't use the same dispatcher.
And ha, yes we did. And my preference for that approach over yet another YAML file still stands. :)
Comment #139
xjm@dawehner and @effulgentsia suggested this is too much of an API change at this point. We could add support for
events.yml
in 8.1.x or another minor version so long as we retained BC for registration in theservices.yml
.So removing from the sprint list. Not actually bumping to 8.1.x yet, but we should consider doing so.
Comment #140
valthebaldAdded beta evaluation
Comment #141
Crell CreditAttribution: Crell at Palantir.net commentedPer #139.
Comment #142
MustangGB CreditAttribution: MustangGB commentedComment #143
dawehnerComment #148
andypostComment #149
fgmOne issue msonnabaum touched on is the ability to directly declare event listeners, not just subscribers.
This is a problem I encountered recently when porting Heisencache to D8: needing configurable services, the static getSubscriberEvents() wasn't usable: I needed these service instances (with their dynamic configuration) and I had to add a specific CompilationPass just to be able to register them as EventListener : a mechanism allowing a dynamic discovery beyond what EventsubscriberInterface allows would have been a boon.
Comment #150
joachim CreditAttribution: joachim at Torchbox commented> $events[RdfMappingEvents::MAP_TYPES_FROM_INPUT] = 'mapTypesFromInput';
The issue summary doesn't say how the yml will declare the priority on subscribers, or how to declare multiple methods for the same event.
Comment #151
Wim LeersThis was not my proposal, but I amended the proposal to address #150's legitimate concerns.
Comment #152
dawehnerI think the change of the proposal in #151 is a bit weird.
We should rather do one step at a time
a) Allow to define things in a yml file
b) Allow to define before and after, which is discussed in #2352351: Add before/after ordering to events
For sanity I would really suggest to not mix up both things.
Comment #153
Wim LeersThen how do you want to solve
?
Comment #154
pounardI'm sorry to flame here, but regarding:
I still don't understand this. If you're using Symfony, you should embrace it: tags are really great to use. I can't understand why you would really want to do an overly useless Drupal-ism where Symfony does solve the problem in a very clean and efficient manner.
Moreover, container tags is a widely known and understood mecanism in the Symfony community, you're, in my opinion, once again stepping back from it by adding a new pure Drupal layer that will confuse newcomers.
I don't want to be rude, but I hope this external point of view might lead you to reconsider getting away from Symfony so much whereas Drupal should embrace it.
Comment #155
fgmAIUI, the very real problem is the lack of discoverability of listeners/subscribers. However, seconding @pounard here, instead of adding a new Drupalism in these days where SF3/4 is becoming more and more widespread, we could just add discoverability for these, e.g. in devel.module, to avoid raising our entry barrier.
Here is a simple gist I just did which does just that as a Drush 9 command:
https://gist.github.com/FGM/136f68e3f30c2c2b9eb9687fd9ddcfc4
Comment #156
dawehnerIt is a bit weird that you didn't had the idea to look at the patch itself, nevermind I updated it with the last patch :)
To be honest though there are a couple of issues (including the one @pounard and @fgm mention):
Overall I think the impact of #2352351: Add before/after ordering to events would be higher, especially given that this will give additional safety.
Comment #157
pounardAnd another note, historically, Drupal 8 did provide its own version of the EventDispatcherInterface (which is not compatible with Symfony's core ContainerAwareEventDistpatcher - now just EventDispatcher in Symfony 4, this gets interesting but we'll see that later) - mainly for performance reasons. But as of today, Symfony did great improvements in that regard, and Symfony 3.4 / 4.0 even more. Good bye ContainerAwareEventDispatcher, it's now much more lazy.
Maybe at some point, as a side task (not this issue, but as we speak of it...) maybe it will be time to get rid of Drupal 8 custom implementation and rejoin the way Symfony does it - one problem would only be that they rely on a PHP-dumped compiled container, this is actually on what a lot of performance improvements on the container do rely - but I'm sure there's ways to adapt that to Drupal container.
Comment #158
andypost@pounard see related #2909185: Replace ContainerAwareEventDispatcher with Symfony EventDispatcher
Comment #159
pounard@andypost nice thanks.
Comment #163
andypostNow SF 4.3 changing dispatcher https://symfony.com/blog/new-in-symfony-4-3-simpler-event-dispatching
Comment #168
bradjones1There are a number of other issues which are close to being "done" that enable utilizing Symfony's DI constructs more natively. Specifically apropos here is #3021803: Document and add tests for service autowiring, which enables autowiring; I believe a natural next step would be #3108565: Update the Yaml file Loader functionality to simplify service configuration, which incorporates from upstream more DI functionality.
Comment #173
andypostLooks it makes no sense after #3221128: Enable service autoconfiguration for core event subscribers