From #2371709: [PP-x] Move the on-demand-table creation into the database API.
At the moment (Dec 2024), the event dispatcher is only available, as a service, once the container is fully bootstrapped.
It would be handy to have the possibility to dispatch events in early phases of the request. For example, the database API has started using events in some cases, and there are other use cases potentially coming (see parent issue). The database connection itself need to be available before the container, and therefore cannot rely on service injection. There is protective code in Connection to prevent an event being dispatched when no dispatcher is available
public function dispatchEvent(DatabaseEvent $event, ?string $eventName = NULL): DatabaseEvent {
if (\Drupal::hasService('event_dispatcher')) {
return \Drupal::service('event_dispatcher')->dispatch($event, $eventName);
}
throw new EventException('The event dispatcher service is not available. Database API events can only be fired if the container is initialized');
}
but that leaves blind spots when events cannot be dispatched - prior to bootstrap or during container rebuilds.
Add a decorator class that creates an EventDispatcher singleton on demand in early stages of the request, and re-creates it upon container bootstrap and on container rebuild. This class can be instantiated as part of the container build, or independently from it, still it always returns the EventDispatcher singleton (or scratches/recreates it as needed, transparently to calling code).
Pass the factory (not the instance of the EventDispatcher, see below) to code that needs to dispatch events early in the request process, like the database Connection.
Note: code in need of dispatching events before full bootstrap need to call the decorator and NOT instantiate/store the EventDispatcher object as a property, because it would get stale during the request processing (i.e. it would keep a subset of the listeners that are needed by the full service container).
The method above thus becomes
public function dispatchEvent(DatabaseEvent $event, ?string $eventName = NULL): DatabaseEvent {
return $this->eventDispatcher->dispatch($event, $eventName);
}
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #5
nicxvan commentedWhy not module_handler?
You can make it a hook, they are now event listeners and give a lot of the same benefits of event subscribers, they are now classes and are autowired.
I also think there's less boilerplate needed among other benefits.
Comment #6
nicxvan commentedComment #7
mondrakeComment #8
mondrake@nicxvan re #5 this is about the event dispatcher itself, not about the subscribers.
Comment #9
mondrakeComment #10
mondrakeComment #11
mondrakeComment #12
mondrakeComment #13
daffie commentedI think we should add a CR.
Comment #14
mondrakeAdded some docs inline.
Comment #15
mondrakeLet’s see if we can make one more step and make the factory a decorator of the event dispatcher.
Comment #16
mondrakeComment #17
mondrakeComment #18
mondrakeComment #19
mondrakeAdded 2 CR stubs, one #3494043: An event dispatcher instance can be available before container initialization for the generic availability of the event dispatcher before container bootstrap, and one #3494044: Database::Connection requires injection of the event dispatcher for its usage in
Database::Connection. Will complete at later stage, doing at this stage has an high risk of getting stale information in it while reviews and tuning still needs to happen.Comment #20
mondrake#2951046: Allow parsing and writing PHP class constants and enums in YAML files would help, we could pass an enum parameter in the service definition file, which we cannot presently.
Comment #21
daffie commentedComment #22
daffie commentedMaybe we should do #2951046: Allow parsing and writing PHP class constants and enums in YAML files first?
Comment #23
mondrakeThanks for review @daffie!
#22 would be nice, but I do not see it as a blocker.
Addressed the comments.
Comment #24
daffie commentedThe CI pipeline is not happy. It fails for Drupal\KernelTests\Core\Database\DatabaseEventTest
Comment #25
mondrakeRestored bot happiness. Back to NR.
Comment #26
daffie commentedThe MR looks good to me.
The IS and the CRs need to be updated.
Comment #27
nicxvan commentedIf you are trying to make this unified post bootstrap then why not have a utility class which you can call instead of the event dispatcher and does a fixed call before bootstrap and dispatches an event once the container is available?
Comment #28
mondrake@daffie thanks, if you do not mind I'd leave this to NR a bit longer to see if more reviews/input come. I will definitely do the CR and IS updates when we have settled on the final solution.
@nicxvan I am not sure I understand; feel free to open another issue branch and MR if you want to propose an alternative!
Comment #29
nicxvan commentedWhat I mean is you're jumping through all these hoops to get the event dispatcher in place. But you're only calling a fixed output for that.
You should just replace this with a utility class that before bootstrap calls the static helper you're creating preBootstrapSubscribers to handle.
So the helper would call the static preBootstrapSubscribers if you are prebootstrap and then call the event dispatcher as it does now after.
You get the benefits you're looking for with way less code.
Comment #31
nicxvan commentedPlease take a look at
MR 10598MR 10607This should show there is no need for a generic "early" event dispatcher.
If we are so early that there container is not available we can provide a list of fixed subscribers. If the issue is injecting it then we can document that you can inject it as a closure in very early services like MR 10607
Comment #34
mondrake@nicxvan thanks, I like where MR 10607 is heading, especially in the part where the early subscribers to be registered with the early dispatcher are moved to the database API.
However, this is creating an (early) event dispatcher that is attached to the Connection class instead of being unique across the request. Not a big deal for now, right, but what if in the future more code would need to dispatch events early? Cache for instance may want to dispatch an event to be responded by a database listener (again, not the case know, but to prepare for possible future). I'd rather keep a singleton somewhere outside of the Connection class, so that we do not end up with vertical dispatchers not talking to each other. That's an architecture topic, tagging so we can have a review and guidance.
Then, I think we should use a closure also to register the early subscribers in place of the const array - so to allow passing instantiated objects that may require parameters (atm in both MRs the assumption is that the subscribers have no parameters in the construction).
Comment #35
ghost of drupal pasttl;dr: YAGNI.
(emphasis mine)
When that future arrives, the fixed dispatcher perhaps could be moved into a trait. And that's pretty much what I would suggest for every other point: wait until there's a need. Possible futures are great to consider when building a new core mechanism -- but this one here is exactly about a mechanism that can't be extended and so then why provide a mechanism which can accommodate possibilities? You have the rare luxury to wait for actualities.
Comment #36
mondrake#35 thanks for your comment.
Future in this case is not so far away, and I am actually working towards making it happen here.
If my proposal in #2371709: [PP-x] Move the on-demand-table creation into the database API is accepted, we'll need a subscriber for database schema request events; if #3495728: [meta] Drop implicit autocommit on Transaction destruction goes on, I am suggesting to move the post-transaction callback execution from TransactionManager to an event subscriber that would react to a transaction commit/rollback/failure event - and that subscriber should live into the Cache API (
CacheTagsChecksumTraitis the only consumer in core of post-transaction callbacks), hence the need to have a way to add early subscribers to the early dispatcher.Comment #37
ghost of drupal pastOK then would a trait work for you?
Comment #39
nicxvan commentedThe trait approach is in: MR !10670 mergeable I am going to hide the fixed dispatcher MR for now because it is MR !10607 and that will be easy to confuse.
Comment #41
mondrake@ghost of drupal past, @nicxvan thanks - I still prefer my own approach than the trait one. Apart from the obvious, i.e. that since I wrote it I prefer it :), I think there are a couple of points that are unclear to me with the trait one:
1) IoC - IMO it's cleaner in the non-trait approach; each Connection object receives in the constructor a decorator of the EventDispatcher and does not have to care about it as it transit from pre-bootstrap to full container mode. This also makes simpler to write unit tests by mocking the event dispatcher passed to the Connection objects.
2) most important to me: unless I am mistaken, the trait will create an event dispatcher specific for the Connection object, whereas my idea is to have a singleton accessible by any class that would need pre-bootstrap event dispatching, with an orderly way to add early subscribers avoiding duplications.
Comment #42
znerol commentedI am looking from the performance / page cache angle at this issue. Any service which is instantiated before the page cache middleware has a chance to return early is obviously a concern from that PoV.
Unless I'm misunderstanding everything, the problem is this: In order to get the container, it is necessary to perform a cache lookup. If the container cache table is missing, then it has to be created and if that creation operation is supposed to fire events, then obviously the event dispatcher needs to be present.
That problem is unique to the interaction between cache and storage. I cannot see any other thing in the early bootstrap phase where that kind of circular dependency potentially exists.
In other words: There is no non-cache table/collection that needs to be created on-demand before there is a full container.
Arguing from the performance angle again, I'd prefer if the fix is tightly scoped to the problem at hand. A generic pattern IMHO isn't really beneficial. In contrary, a generic solution increases complexity in early bootstrap and potentially causes performance regressions if more "fancy" stuff lands in a position before the page cache.
My suggestion would be to split the table creation logic. Table creation of cache tables must not fire events. Everything else in the storage layer can do.
Comment #43
mondrakeThanks! Besides cache table creation, the opportunity is also to use events to wrap database transactions see #3495728: [meta] Drop implicit autocommit on Transaction destruction, and more in general allow telemetry on database transactions see #3348590: Add transaction-related events to the Database API. Both things could happen before the container is available.
Comment #44
znerol commentedRight. In that case I would revise my suggestion like this:
Creation of a cache table must be an isolated operation. It must run in its own transaction and must not fire any events.
As for the instrumentation, this shouldn't have any influence on application code. I have been evaluating contrib-auto-pdo and this is recording spans for database queries / statements from the very beginning of a request / response cycle if configured properly. No need for events or other application level code in that case.
Comment #45
nicxvan commentedIsn't this also about #2371709: [PP-x] Move the on-demand-table creation into the database API
Comment #46
mondrake#45 yes, sure - a cache table is a specific case of that more general issue.
Comment #47
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #48
mondrakeUpdated fork
Comment #49
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #50
mfbVery nice! I might play around with this myself. Note the only reason this works is because of the opentelemetry PHP extension.
Comment #51
nicxvan commentedThis needs a rebase
Comment #52
mondrakeRebased
Comment #53
mondrakeComment #54
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #56
dcam commentedRebased MR 10487
Comment #57
mondrakeComment #58
smustgrave commentedper the slack channel going to postpone on #3538212: Reintroduce the container aware dispatcher for a 10% speedup
Comment #60
catchNot really clear why this was postponed on #3538212: Reintroduce the container aware dispatcher for a 10% speedup but that issue isn't a priority now that hooks are no longer events, so I'm unpostponing it If it really does need to be postponed on that issue, would be good to write up why here.
I'm wondering why this issue doesn't talk about adding an event dispatcher to the bootstrap container vs having it as a singleton.
Comment #61
mondrakeI think this is related also to #3585294: Convert bootstrap container from array to PHP.
General ignorance of inner details of early bootstrap, I admit.
I came to this with the need to have an event dispatcher available to the Database connection as soon as possible, so that for instance we can use events in database schema operations like creating tables. Is the bootstrap container instantiated before any db operation?
Comment #62
catch#3583505: Use Symfony PhpDumper instead of a serialized array container structure is making the same bootstrap container change.
Yes!
Comment #63
mondrakeLooked a bit at it, and indeed it looks like we could add the service to the bootstrap container - then we'll have to make it accessible from outside the DrupalKernel class (or add event management related methods to it). I'll work on that in #3348590: Add transaction-related events to the Database API when time comes.
Leaving to NW if anyone wants to tackle it for other reasons.