Explore injecting the event dispatcher in the database service

mondrake → changed the visibility of the branch 3492391-explore-injecting-the to hidden.

Why 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.

@nicxvan re #5 this is about the event dispatcher itself, not about the subscribers.

I think we should add a CR.

Added some docs inline.

Let’s see if we can make one more step and make the factory a decorator of the event dispatcher.

Added 2 CR stubs, one [#3494043] for the generic availability of the event dispatcher before container bootstrap, and one [#3494044] 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.

📌 Allow parsing and writing PHP constants and enums in YAML files Needs work would help, we could pass an enum parameter in the service definition file, which we cannot presently.

Maybe we should do 📌 Allow parsing and writing PHP constants and enums in YAML files Needs work first?

Thanks for review @daffie!

#22 would be nice, but I do not see it as a blocker.

Addressed the comments.

The CI pipeline is not happy. It fails for Drupal\KernelTests\Core\Database\DatabaseEventTest

Restored bot happiness. Back to NR.

The MR looks good to me.
The IS and the CRs need to be updated.

If 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?

@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!

What I mean is you're jumping through all these hoops to get the invent 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.

Please take a look at MR 10598

nicxvan → changed the visibility of the branch 3492391-early-fixed-event-dispatcher to hidden.

@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).

tl;dr: YAGNI.

Not a big deal for now, right, but what if in the future more code would need to dispatch events early?

When that future arrives, the fixed dispatcher could be moved into a trait.

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.

Not sure why that's a problem. These are, in a sense not "real" event dispatchers in the senses that there's no mechanism to add subscribers to them -- they are constant anyways. They are merely a convenience for the consumer not to write a separate code path where it checks whether the subscriber is ready and if not then it calls the relevant code itself. This patch could be

public function dispatchEvent(DatabaseEvent $event, ?string $eventName = NULL): DatabaseEvent {
  if ($this->eventDispatcherCallable) {
    return ($this->eventDispatcherCallable)()->dispatch($event, $eventName);;
  }
  (new StatementExecutionSubscriber)->onStatementExecutionEnd($event);
  return $event;
}

it's the exact same thing. It would be somewhat simpler to do that, too. Perhaps hardwiring the method besides the already hardwired class name is a problem and as with every problem it can be solved by adding some abstraction which in this case is this fixed event dispatcher. As proposed the fixed event dispatcher is so simple it's not worth considering the cost of it but the moment it becomes an elaborate mechanism itself the question could be raised whether this tiny amount of single responsibility principle violation is not worth it.

I think we should use a closure also to register the early subscribers in place of the const array

and what would supply such a closure? Maybe wait with that when there's a need? This is not like a normal core mechanism which needs to think about extensibility. The very point of the existence of this mechanism is the constant-ness.

#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 📌 Move the on-demand-table creation into the database API Needs work is accepted, we'll need a subscriber for database schema request events; if 🌱 [meta] Drop implicit autocommit on Transaction destruction Active 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 (CacheTagsChecksumTrait is the only consumer in core of post-transaction callbacks), hence the need to have a way to add early subscribers to the early dispatcher.

OK then would a trait work for you?

The 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.

nicxvan → changed the visibility of the branch 3492391-early-fixed-event-dispatcher2 to hidden.

@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.

I 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.

Thanks! Besides cache table creation, the opportunity is also to use events to wrap database transactions see 🌱 [meta] Drop implicit autocommit on Transaction destruction Active , and more in general allow telemetry on database transactions see ✨ Add transaction-related events to the Database API Needs work . Both things could happen before the container is available.

Right. 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.

Isn't this also about 📌 Move the on-demand-table creation into the database API Needs work

#45 yes, sure - a cache table is a specific case of that more general issue.

The 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.

Updated fork

The 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.

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.

Very nice! I might play around with this myself. Note the only reason this works is because of the opentelemetry PHP extension.