To prevent conflict and performance problems with other code that runs during cron, decorate the cron service instead of implementing hook_cron

Tagging as we may be able to address with future core modulehandler reworks.

If this was the approach we were going to take in the final (core) version of this module, I'd probably want to kick the tires more, and would likely have pushback. There are some definite underlying concerns here.

But we're just trying to get an alpha out, before DrupalCon, after discovering several serious gotchas. This, therefore, seems like a reasonable temporary solution for running unattended updates by piggybacking on top of the existing cron infrastructure.

But there's a question here: we know that our update system currently doesn't work with Automated Cron. How does decorating the cron service address that? Or is it something we're just not addressing here, and instead waiting for 📌 Disallow unattended updates if performed by automated_cron Fixed to land?

Decorating the service will not make the problem of Automated Cron worse or better. Automated Cron will still fail to update because it uses the service in kernel terminate event. so I don't think it needs to wait on 📌 Disallow unattended updates if performed by automated_cron Fixed

In 📌 Disallow unattended updates if performed by automated_cron Fixed we could only disallow update if it is currently being run by automated cron but I am not sure it is worth the effort. Maybe but it shouldn't affect this issue

Re-titling for clarity based on the issue summary.

As I said, this is a temporary solution. The real answer, in my view, is to run the update in phases that are handled by a queue that gets worked on during cron runs, and only decorate the cron service for the apply phase, which really does need to be as unencumbered by other code as possible.

So, with that in mind, this is RTBC.

Looks great!

But:

1. don't we want to set a decoration priority to ensure ours is always first? Otherwise, it may still fight with certain contrib modules… See https://symfony.com/doc/current/service_container/service_decoration.htm....
2. why do we not generate a ProxyClass for it? This is required, otherwise it won't be instantiated as a lazy service. Which means that right now, this issue is causing every single request to be responded too more slowly! 🙈 See #1973618: DIC: Lazy instantiation of service dependencies (ProxyManager for "proxy services") → , which introduced this last line:
   

     cron:
       class: Drupal\Core\Cron
       arguments: ['@module_handler', '@lock', '@queue', '@state', '@account_switcher', '@logger.channel.cron', '@plugin.manager.queue_worker', '@datetime.time', '%queue.config%']
       lazy: true
   

3. Did we test that this still works when e.g. https://www.drupal.org/project/ultimate_cron → is installed, which has 40K D8 installs? See https://git.drupalcode.org/project/ultimate_cron/-/blob/8.x-2.x/src/Ulti... — I don't see how it'd still work with that installed. We need a follow-up that adds a validator for checking if $this->container->get('cron') returns our service, and if it doesn't, shows an error on the status report.

I don't see how it'd still work with that installed.

Why wouldn't it work?

They're overriding the cron service, and we're decorating it. Those are two different approaches. From what I'm seeing here, if you have Ultimate Cron, our decorator should wrap seamlessly around it.

#11: I think you're right! 👍

That leaves only #10.1 & 2 😊

About #10.2: does the decorator actually need the proxy class? Or will it wrap around the existing proxy class? I'm unclear on how it works. In discussion, @tedbow thought it would not actually be an issue.

I don't think 10.2 is accurate but I am busy unblocking other issues. Will comment later