RFC: Better events/tasks for deployment automation (dev/build++), may include event-dispatcher being a core dependency

[sminnee]

Right now we have dev/build as a critical part of the a production deployment.

That's great, but it's incomplete. In particular, it is inappropriate to call this multiple times on a multi-server environment.

It would be helpful if there were a broader range of points to hook into the deployment pipeline, such as:

• Prepare the the content of the BASE_PATH for deployment (e.g. vendor-expose, GraphQL4 where code goes into BASE_PATH)
• Prepare an environment's shared resources to prior to activation of the code that this is being run on (e.g. the current dev/build)
• Warm this environment prior to activation of the code that this is being run on (e.g. flush, template rendering, GraphQL4 where code generation goes into temp path)

This would mean

• automated deployment systems could run these events without needing to know which modules were installed
• modules would have more tools to work with

An obvious choice for these hooks would be build tasks. But @unclecheese's work with event-dispatch might also be good base for this.

So the questions are:

• What hooks do we want to add to deployments?
• BuildTask or event dispatch?

Do we want event-dispatcher in core?

If we used event-dispatcher for this, it will make it a core dependency. Are we ready for that?

• Strictly speaking it could be a suggestion, and maybe only a dependency of graphql 4. If we used core build tasks to trigger these events, they could trigger the events only if event-dispatcher were included, and without event-dispatcher it could limit itself to executing only the basics (e.g. dev/build). But that's a bit messy and I'd only do that if there was an important reason to avoid making it a requirement.

• The biggest risk about pulling in event-dispatcher is not the code itself, but commitments to the API. IMO we'd want to pull it in as a 0.x dependency, which means that there are fewer stability commitments about its API.

  • This seems okay to me, as no-one is obligated to use this new API.
  • In practical terms, however, it may increase the chance that we need to make core support multiple 0.x release lines with differing APIs. In particular, if lots of sites make heavy use of the API and couldn't easily jump to 0.2 o 0.3. To be honest I think that cognitive overload is probably less than trying to invent APIs that are perfect before we try them in production. :P
  • We can avoid needing to support multiple event-dispatcher APIs in the future by including it but marking it as experimental.
  • ...which brings us back to maybe making it a suggested rather than required dependency.

[sminnee]

For the "what hooks do we want" it would be good to hear from people who are more focused on server-setup and deployment.

[chillu]

Agree this is a good idea, and it paves the way for more deterministic cache creation over time. I think ?flush=1 and ?flush=all have basically broken dev's mental models about what cache is invalidated when over the years. I could not tell you if you still need a dev/build/?flush=1 right now without looking up some intricate use cases, and I'm not 100% sure if ?flush=all still does something different than ?flush=1. It's just gotten so confusing over the years that (like everyone else) I'm doing "superstitious flushes", rather than based on sound understanding of caching. That's quite different to e.g. Laravel's caching, which is basically "no caching in dev, generate caches through tasks during deployment". So I think if we're introducing something like this, it needs to be accompanied with a "caching overview" and a "deployment guide" in our docs.

I'd say BuildTask, but extensible via event-dispatch rather than Extension based. No strong feelings about other points.

[unclecheese]

I love the idea of adding more reactive programming paradigms to the framework, but we're essentially saying the event-dispatcher module becomes a core dependency if we go that route. Have we thought all of that through? Might need a separate RFC.

[sminnee]

I'd say BuildTask, but extensible via event-dispatch rather than Extension based.

Do you mean a build task that basically just goes Dispatcher::dispatch($someEvent)? I could roll with that

I love the idea of adding more reactive programming paradigms to the framework, but we're essentially saying the event-dispatcher module becomes a core dependency if we go that route. Have we thought all of that through?

I'll change the heading of this instead ;)

[unclecheese]

prepares to scratch "write a core module" off bucket list

[sminnee]

Assuming we use event-dispatch for extensibility, which I believe is everyone's preference, I think the main choice is:

• Do we make it a required module, used extensively, and risk needing to support the current API in 4.x, alongside any future API changes we make.

• Do we make it a required module, but clearly mark it as experimental and drop the current API if it undergoes changes in the future

• Do we make it a suggested module, with its use guarded by class_exists()

[robbieaverill]

I'm not familiar with the event dispatcher module, would someone mind giving a quick summary of how it would solve the problems in this RFC?

[sminnee]

This module in question was https://github.com/silverstripe/silverstripe-event-dispatcher which was originally developed while working on versioned-snapshots.

It's based on Symfony's event system and lets you attach event listeners to a central dispatch service. Notably, listeners are attached to the central dispatch rather than individual objects. This makes it well suited to helping loosely coupled subsystems interact, but less well-suited to connecting the internals of cohesive code within a subsystem. (This is why we opted not to use it for the add/remove callback feature)

For this case you could dispatch events for the various steps in the build cycle. So rather than creating a build task. The event dispatcher provides a way of ensuring that any number of calls can be added to a single extension point.

Triggering an event requires a PHP call, so a simple wrapper would be 1 or more build tasks that do nothing other than trigger the event in question. This could also be a dedicated CLI script, but presenting as a build task probably makes it more findable for Silverstripe devs.

Without event dispatch, you'd probably need a way to, for example, group together a bunch of build tasks and execute them all at once, letting developers add tasks to the pre-existing groups as desired. That's something that event dispatch gives us for free, but if we brought it in solely for this use-case, it'd be a bit weird. So the question is really "would event dispatch be a useful pattern to have in the system" which to me seems like a good idea. I think this was why Aaron initially said "should this be a separate RFC" but I wanted to avoid ticket fragmentation.

As an example of its use, in the case of versioned snapshots, https://github.com/silverstripe/silverstripe-cms-events/ provides a bunch of events around interaction with your content manual that is slightly less granular than individual ORM operations and closer to "individual human button-clicks", while also covering cases such as GraphQL.

One thing that I'm not sure event-dispatch gives us is the ability to tweak the order of the event handlers. This is a bit of a minefield and it's probable that the are better ways of dealing with order-dependency. For example, we could get in the habit of having critical build steps trigger their own before/after events, and developers could attach listeners to those events instead of the parent step. But it needs thought.

[chillu]

@silverstripe/core-team We're moving along the path of core inclusion of silverstripe/cms-events with this new GraphQL v4 PR: https://github.com/silverstripe/silverstripe-graphql/pull/352. GraphQL v4 isn't stable yet, and not required by the core recipe. Once we do that, we'll also take on maintenance of silverstripe/cms-events (as well as a new Symfony dependency). Even with GraphQL v4 stable, I don't believe we need to call silverstripe/cms-events stable, but it should have beta releases. Notably according to our definition of a beta release, while they're targeted at bugfixes we can still change APIs.

Three core committers think that an event dispatcher in core is a good idea in general. Where we struggled before is actually rolling out such a change consistently.

Just for completeness, here's a few unresolved discussion points. Some of those might influence this RFC (given we don't want to split the deployment events from event listener), some of them might become tickets on the module.

• Event namespacing conventions (if any)
• Wildcard event listeners (if we do namespacing)
• Overlap between events and queues (see https://laravel.com/docs/8.x/events#queued-event-listeners)
• Conventions for strongly typing events
• Dispatcher singleton vs. DI (always going to be global, but making the dependency clearer)
• Making https://github.com/silverstripe/silverstripe-cms-events obsolete (inlining it into core)

[unclecheese]

Quick point of clarification here -- we're not making cms-events a dependency. We're making silverstripe/event-dispathcer a dependency.

The dispatcher provides the backend, and cms-events provides the subscribers to specific events (currently required only by versioned-snapshots).