Add CronSubscriberInterface so that services can execute cron tasks directly

Investigating the scheduler a bit more, I feel like it's probably a bit early for the scheduler. Also looks like each scheduler is its own transport and needs to be run as a worker, I'm not sure if that fits into Drupal. Plus, it's experimental and the documentation is still literally a 404 page.

@andypost: queue issues are only relevant if we would go with the Symfony solution, which I think is fairly unrealistic at this point. And if we do, every single issue that touches the queue system would be "related" as we'd completely replace.

pushed a proof of concept to the merge request, a bit weird with reusing the existing CronInterface and didn't test yet at all, but seems simple enough :)

> @Berdir yes, I mean option 2 looks more viable to me

I don't think it was ever more "viable", it would be far more complex and so much more work. It would be more flexible and open up more possibilities, but the current state of that component (zero documentation, experimental) makes this a simple choice for me. Note that we still have the option to look into replacing our queue system separately with the messenger component, then *afterwards* we could think about the scheduler.

Especially for the initial step of just converting to tagged services without extra features. That's very straight forward. Still haven't actually tested anything yet, but I've extend the proof of concept to fully replace system_cron().

Some notes on that:
* Yes I know there are some missing docblocks, documentation that needs to be updated, dependency injection to be done. That's all not very relevant right now, what we should discuss the overall API.
* As mentioned, I reused the existing CronInterface for it, but that's really a CronRunnerInterface (which I'm not sure really should exist at all). So we should probably introduce a new Interface that's very similar to that one.
* some system_cron() things are called on interfaces/official API's others only on specific implementations. I took some shortcuts with that for now, for example Flood has it on the interface but I'm just doing the cron thing on the database implementation. We should possibly deprecate some garbage collection interface methods and just let implementations handle it. For now, flood for example would need a separate service that calls that method on the flood service.
* Lazy loading those services would be nice, but that only really gets interesting when we support scheduling.
* BC: This will break installations using ultimate_cron completely, not easy to handle that. We could keep system_cron() and so on around but somehow flag it so that the implementation in core doesn't actually call it? Or we just live it, there are only a few alternative cron runner implementations.

Setting to needs review to get some feedback on that.

> Seems to have some test failures.

It's a proof of concept patch converting a single hook to the proposed new patch. It's far from complete for much bigger reason than that but I'm looking for architecture review, so IMHO needs review is an appropriate status.

> While we're modernizing Cron, could we also add a Console command for running Cron through the drupal utility?

The public API to run cron is unaffected with the currently proposed approach and there is no reason to add a console command in this issue as well, this is going to be more than enough on its own. If you want that, feel free to open a separate issue.

Wow, I just couldn't understand why there's an exception thrown when it's called directly as a service but not on 11.x I was debugging through the installer and that site directory protection code, and I just could not see a difference.

Turns out there is no difference. The very same exception is also thrown on 11.x in that test, the only difference is that all exceptions are caught and "logged" for cron hooks, but I didn't have that part yet for services.

The problem is a conflict between the test user agent stuff, which stores the info in a .htkey file inside the sites directly, and the site directory protection logic, which runs before in the installer which makes that directly read-only. Which then later on the test reverts again (I guess?). I really wish we could just remove all that strange protection code (#3091285: Composer scaffolding fails when permissions on default.settings.yml or default.settings.php is not writable.).

But it's not the problem of this issue, I just accidentally exposed it.

I do want to move this forward, but before I do the work of converting all those cron hooks to services, I'd like some feedback on the general idea and architecture. Specifically, as mentioned, I think this should be new interface and method, looking for suggestions on how to name that.

I converted a few more that were easy to move to an existing service. Looking at the others, I'm a bit unsure if we should really replace all of them in this issue and deprecate hook_cron() already. It's going to be a big issue and many of them are tied to existing issues to move functions into services, so we'd have to make up new services only to deprecate them again later.

Then again, actually getting all those issues in is likely going to take years if it ever happens. But we can always convert the leftovers to services any time we want.

that does look interesting and pretty much like I expected it would from reading the docs, but there are several things that sound like pretty hard blockers:

* having to run a consume process is a no-go and definitely not the same as a cron. Even on more advanced hosting like platform.sh, that implies a separate worker container, while a cron you just define and it runs within the environment. And regular webhosting won't be able to do that at all. That's why we ship with the ability to run cron through a URL (so you can do it externally and some webhostings support specifically that) and automated_cron. So unless we find a way to run consume on-cron or on the fly like automated_cron and the idea to run queues in destruct/shutdown as well, there is no way that we'll get that into core.
* I'm confused by the whole name thing. Having to explicitly specify the names really doesn't work IMHO, we can't require you to customize that command every time you install a module? Having the ability to do so seems like a powerful feature for very few sites that process a large amount of messages but I'd expect we need something like a consume --all and maybe then to be able to separate one of them a consume --except. The counterpart to that with the current queue system is to have queue definitions that do not run on cron.

> Catch-up would be the main concern.

Yes, and if we do need to do that ourselves then how much exactly do we benefit in the end and how interchangeable will the system really be?

For me, based on what I've seen so far, the answer is fairly clear, also as (not very active but sole) maintainer of ultimate_cron.

1. Convert hook_cron() to tagged services + an interface is a straight forward step that allows us to remove another hook. It also allows each module and component to provide an arbitrary amount of cron jobs, ultimate_cron can discover them as well instead of the current manual step and just like now, allow to customize their execution.

2. Add features as deemed useful, for example the ability to define a desired schedule/frequency. ultimate_cron can support that as the default configuration and replace the current hardcoded assumptions for core. Sure, that will need exactly the same scheduling and catch-up logic, but that works in both ways and we could still use that if we switch in later steps.

....

N. If/after core successfully replaces its queue system with the messenger component, then we can look into using the scheduler component as well, and we'd IMHO lose very little, those tagged services can be converted to message handlers, the scheduling component from step 2 can be reused and so on. And ultimate_cron can go to its ultimate destination, the trash bin :)

I fixed flood tests, although that's kinda weird. The testCleanup() method is only testing the memory backend aka the default in kernel tests. there is a database test as well, but not sure how useful it is to test the memory backend like that. It does point to a problem though, that it would break if someone were to replace the service with something that either doesn't add the tag or doesn't have the interface.

redis.module does that for example.

Also, Drupal\Tests\dblog\Kernel\DbLogTest::testDbLogCron is a problem too. that counts the hook implementations and then asserts the number of log entries that were created. First, seems wrong that the test coverage for that is in db log module, second, with services, it's a lot more and we don't have a way to know how many to expect.

Thanks for the feedback. Yeah, a separate Interface and method definitely makes sense, suggestions on the method name? Not sure what we'd call a service that has a has a cron method. I don't think CronRunner makes much sense that really seems like what the current CronInterface should be called (or CronScheduler or something). Looking at the symfony messenger/scheduler components I also don't really see an obvious concept/name we could use from there as the concepts are very different.

CronSubscriberInterface maybe, it's similar to the event system, instead of subscribing to an event, you subscribe to cron scheduling?

Do you also agree with this being an intermediate step where we introduce this as a parallel system to the hook and don't deprecate anything yet? As mentioned earlier, there are a lot of cron hooks related to things that are being converted to services in other issues, forcing just that part to a service now will just complicate that and then instead we can just update the relevant issues to directly use this system instead of going through hook_cron(). search/statistics stuff for example.

Then we can decide later if/when we want to deprecate the hook.

> The issue summary talks about stuff like more advanced scheduling etc (e.g. only running every N cron runs etc)

Pretty sure that should be a follow-up. We can easily expand this and introduce annotations or something to allow implementations to say that they want to be called once per hour or once per day or so. And for now we can let ultimate_cron handle that, it just needs to support this new interface as well and then it can continue to function as it does now.

Added CronSubscriberInterface and new title, as we don't replace hook_cron() just yet.

For more scheduling features, we could then go back to using #154043: Cron should not remain monolithic (Implement a scheduling hook for cron) for that.

I'll also add a follow-up to deprecate hook_cron() and will prepare a change record.

Ok, test are now passing, after fixing a lot of stupid bugs and typos ;)

Also created a change record: https://www.drupal.org/node/3414595.

To do:

* Follow-up issue to deprecate hook_cron() (or consider it, or at least convert more of core to services)
* documentation
* ultimate_cron issue to add support for this. I do want to test if discovery of this is working correctly there and how to handle that.

Also, I put the new Service directly into Drupal\Core as CronInterface and Cron are also there. Feels a bit weird, but also weird to create a new namespace just fro this new interface when the existing code is not there?

Ultimate Cron support: #3414863: Support CronSubscribers, it's not very pretty, but ultimate_cron already supports service methods, so I'm just converting them to use service:runCron as callback, with an assumption that the first part until the dot might be a module, so then assigning to that:

You could basically already do that with any service, but this allows to autodiscover those services.

Keep in mind that with ultimate_cron, or if we add support in core eventually for some kind of cron rules/scheduling, there isn't really a single/global cron run anymore. does onCron() still make sense then?

Looking at https://en.wikipedia.org/wiki/Cron, it could also be runCronJob() or so?

Happy to pick whatever name we decide on, also for the interface, just pointing that out.

> So another thought that has come to mind that might help decide here: in a way, right now, we are kinda just replicating the event dispatcher, although we want to extend this in the future so some events become dispatched only when a time condition is met. It's possible one service might want to respond to different time schedules with different methods. How do we envisage that working? Similar to the getSubscribedEvents() method of EventSubscriberInterface? Or with method attributes? Or something else?

For scheduling instructions, I was thinking about using attributes: #[CronSchedule(cron: '0 * * * *')]. I didn't think about the use case for multiple cron jobs on a single service, I think that's pretty rare, but once we have the attribute we could probably just check every method on the class for having such an attribute and support multiple if we need/want to?

IMHO a separate method would complicate this quite bit in its current state as you'd always need two methods.

Benefits compared to a regular event:
* There is no need for an event class, there is no input/output
* We don't want cron jobs to be able to stop execution, so we'd need to prevent that.
* Easier discovery and ability to call the subscribers directly. As shown in #34 this is useful for ultimate_cron now and it would also be quite feasible to add optional support for that in core too with that attribute.

In regards to messenger/scheduler component, my thoughts are above, for example #23. This is IMHO an improvement that benefits both core and ultimate_cron and it's pretty simple and doable now. Getting the whole messenger and scheduler components in core and replacing our queue and cron systems completely sounds like a huge undertaking and something that I won't really be to help with. Already in the middle of such an undertaking with the mailer component and that's more than enough for me.

We could for example decide that we will need deprecate hook_cron(), so we won't force anyone to convert just for the sake of it, and then if messenger/scheduler really gets into core and replaces the queue system, we could deprecate both in favor of that. But I feel like there are a lot of option questions to be resolved before that is possible.

Fixed some problems with the rebase.

I have no strong opinion on the name, if onCron() is acceptable for others, then fine by me. Renamed accordingly, also moved the interface, added autoconfig and removed all hardcoded tags. Not sure if the one in KernelTestBase is needed or not.

Auto configuration makes tests very unhappy:

    
    1) Drupal\KernelTests\Core\DrupalKernel\DrupalKernelTest::testCompileDIC
    Symfony\Component\DependencyInjection\Exception\RuntimeException:
    Constructing service "cache_factory" from a parent definition is not
    supported at build time.
    

Reverted that for now.

One thing that might work to avoid that error is that some cron tasks should be on their own service that just uses the respective factory, although that means we need to keep the respective garbage collection APIs, or we refactor it further and make garbage collection the responsibility of specific implementations, but that would likely need quite a few additional deprecations and changes (and might still need separate cron services, not sure).

Thoughts?

Whatever we do, autoconfig does seem to be a bit fragile...

Rebased. Still needs feedback on #56.

Rebased, removed statistics.module changes.

Evaluated this using #3442009: OOP hooks using attributes and event dispatcher hooks.

* Cron implementations can be now be in services and a module can have multiple, so system_cron() for example can be split up. One limitation specifically with that it is that hooks are currently only supported in modules, so we still need services in system module that call out to the respective component. Opened #3481903: Support hooks (Hook attribute) in any registered service for that.
* Discovery for alternative cron executions like ultimate_cron. I will have to investigate this, I guess it should be possible to use invokeAllWith() to find such services and then define a cron job for them.
* Additional instructions and features, like how frequently an implementation should be called. Not directly possible, but I suppose it would be possible to add additional attributes, on top of the hook or replace the hook and deprecate that, with very few other changes. So moving things to a service as a regular hook still helps us move toward a future with more cron functionality in core.