This all makes sense to me, but perhaps we can take this one or two steps further:

We could turn jobs into events (in the general sense). The only conceptual difference I see between jobs and events is that with a job, there is only one handler, and that handler is known to the code that triggers the job/event. This seems more like a problem than a feature. In my mind, it makes much more sense to have code emit events that other code can subscribe to. This reduces coupling. Event handlers for core would be registered in a kind of "listener wiring" similar to "service wiring".

To take it two steps further: Extensions should be able to register their own handlers into the "listener wiring" (and define their own events). What's the difference then between an "event" and an "action hook"? They seem to be the same thing. Can we have a unified system of "code that reacts asynchronously to events triggered by other code"?

If we finish the decoupling between JobSpecification and Job, we're pretty much done changing the concept to event. Rename JobSpecification into EventSpecification and Job into EventHandler - and you're pretty much done. And allow the handler specification to hold and array instead of of a single value.

But that's on the coding level. Conceptually, currently the boundaries are a bit incorrect in the jobs - JobSpecification and job types are much more about what needs to be done, than about what happened. Moving the boundaries so that the job spec only contained info about what happened in the system and the handler figured out what needs to be done is an even further step on this road.

In T245900#6067952, @Pchelolo wrote:

But that's on the coding level. Conceptually, currently the boundaries are a bit incorrect in the jobs - JobSpecification and job types are much more about what needs to be done, than about what happened. Moving the boundaries so that the job spec only contained info about what happened in the system and the handler figured out what needs to be done is an even further step on this road.

That's a very nice way of putting it, thank you!

To me, the question is: do we transition to JobDescription first, and then transition again to events, or should we do both at the same time? Which option is better depends on how complex (and thus, how risky) the change is. We'd have to survey about 50 Job classes to find out.

In T245900#6067787, @daniel wrote:

We could turn jobs into events (in the general sense). […] it makes much more sense to have code emit events that other code can subscribe to. This reduces coupling. […] What's the difference then between an "event" and an "action hook"?

An "action hook" (as defined in T212482) indeed acts as an event. The primary characteristics of hooks (or events) are I think that they are naturally decoupled. There might be no users of a hook (event listeners) and that is perfectly valid.

However as "event" was introduced here without definition, I think the better question is: What is the difference between a "job" and an "action hook". There, I think we are starting to see an important disctinction.

For example, when an edit is being saved then the action is "edit". After the action is completed, an action hook (or event) could let additional logic execute which is not part of the "edit" action itself, but rather in response to it, based on the domain interests of whomever subscribes to that event.

A job on the other hand is often critically part of the user action itself. The action may even be considered incomplete until the job has run. For example, if I send an e-mail, or delete a page - the implementation of that should not be an event listener to some dummy user action that by itself does nothing. When this code queues the job, it explicitly requests its own code to run. If there were no listeners, or only unrelated listeners, that must be a fatal error. Of course, if the logic is meant to be swappable, it could be backed by service class. Or if we want to allow others to also partake, it could also run an action hook once the job has completed. But, I think it is important that we not treat these all the same. That would I think remove important business logic and contracts currently expressed and clearly present in the code.

Having said that, I do think there many jobs that could be converted to simple action hook callers (event listeners). If my proposal for having action hooks be asynchronous is accepted, then there would be no need for an extension to register an action hook that merely queues a job or deferred update. Rather, it would register its callback or class directly with the action hook, which core then internally naturally runs on the job queue somehow (perhaps using a generic "Event" or "RunActionHook" job).

I agree that jobs are not always "events" or "event handler hooks" (what about jobs the subdivide or are added by maintenance scripts and so on?). There is probably a lot of stuff that can indeed be moved in that direction though.

In T245900#6068758, @Krinkle wrote:

In T245900#6067787, @daniel wrote:

A job on the other hand is often critically part of the user action itself. The action may even be considered incomplete until the job has run. For example, if I send an e-mail, or delete a page - the implementation of that should not be an event listener to some dummy user action that by itself does nothing. When this code queues the job, it explicitly requests its own code to run. If there were no listeners, or only unrelated listeners, that must be a fatal error.

I think that this kind of tight coupling between the code that triggers the action, and the code that implements it, tends to be an architectural flaw. We should not encourage it, but rather try to avoid it. We have quite a few places in the code where one subsystem reacts to an event in another (e.g. when a page is edited, the search index needs to be updated). Implementing this in a way that has the event source know about the receiving subsystem introduces undesirable dependencies.

I see your point in the email example, and perhaps we'd need the concept of "consuming" or "acknowledging" an event to make sure it is indeed handled. And in some cases, it does make conceptual sense to defer the execution of code that is tightly coupled. But it seems to me that this is rare, and most usages of jobs in our system should not work that way. Cache puring, for instance, should not be something the code that triggers the event knows about. The caching subsystem would register a listener that facilitates purging. The application logic shouldn't know or care.

This architectural discussion is not something that needs to be resolved before this ticket can go forward. I agree that it's a good thing as proposed. But we should have it at the back of our heads when thinking about this, so we don't further commit to a pattern that we would rather want to replace, at least for most use cases.

See also: T277648: JobQueueGroup::singleton() is difficult to use with dependency injection

$wgJobClasses configuration is removed or it's semantics is changed. It's replaced with a series of object specifications for ObjectFactory with required service classes. All service classes are injected into Job instance.

This would be enough to solve DI for jobs, without any of the other changes (which are all reasonable, it's just good to limit scope). Except GenericParameterJob defines the constructor, and ObjectFactory only supports constructor injection.

I see two ways to fix:

1. Replace GenericParameterJob with a similar interface that passes the parameters to the job instance via setter injection (ie. something like interface IParameterJobSpecification extends IJobSpecification { public function setParams( array $params ); }) and duplicate all the GenericParameterJob special-casing.
2. Just drop the constructor from GenericParameterJob (removing a method from an interface is not a B/C break) and use ObjectFactory::createObject( $wgJobClasses[$type], [ 'allowClassName' => true, 'allowCallable' => true, 'extraArgs' => [ $params ] ) to construct the job instance (so it gets $params as the first constructor parameter and any services defined in the ObjectFactory spec afterwards, which again is B/C with existing GenericParameterJob classes).

I'd go with the second, it's minimal effort and an interface defining a constructor was always a weird thing, even if it was temporarily useful for reducing the confusion around how jobs are defined.

Change 879525 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):

[mediawiki/core@master] Add dependency injection for Job classes

https://gerrit.wikimedia.org/r/879525

Change 879525 merged by jenkins-bot:

[mediawiki/core@master] Add dependency injection for Job classes

https://gerrit.wikimedia.org/r/879525