Need a way to say it's time to run cron

I may be overreacting, but my gut reaction is as follows. I don't have an expectation that new cron jobs should jump the queue, although I understand that others may feel differently. However, I object to something that resets the last cron execution time to accomplish it on the grounds that altering the last execution time could have weird side effects, for example, on jobs that a site owner could reasonably expect to occur at certain times.

Looks it needs basic test to rtbc

Great, just not clear why sleep(1); is used for...
As a bug it could use test-only patch

Here's patch with removal of sleep() and exact testing of that hooks are fired

I thought to put it around \Drupal\Tests\system\Kernel\System\InfoAlterTest but that just increase code fragmentation (automated cron tests living in system module)

I still think it should be part of system module, so here's
- hooks moved to system module where the state is maintained
- test converted to kernel test

no interdiff as it bigger then patch

+++ b/core/modules/system/system.module
@@ -1304,3 +1304,21 @@ function system_theme_registry_alter(array &$theme_registry) {
+function system_modules_installed() {
+  // Reset last cron run to allow new modules to execute scheduled tasks.
+  \Drupal::state()->delete('system.cron_last');
+}
...
+function system_themes_installed() {
...
+  system_modules_installed();

probably here better to not call install hook implementation

Fix #12

lib/Drupal/Core/Cron.php: $this->state->set('system.cron_last', $request_time);
is where the state is changed, so any contrib/custom code could use cron service and use the state, see http://grep.xnddx.ru/search?text=system.cron_last

If this is only affecting automated cron, wouldn't it be better to have a state flag that tells automated cron to run anyway?

I agree with @jhodgdon that the system report should still repot the last time cron ran so having a flag that automated cron sets when modules/themes are installed, and then it'd run cron if the flag is set, or enough time has elapsed since the last cron ran.

Slight nit pick with the patch in #7 in that I wouldn't have the hook_themes_installed call the automated_cron_modules_installed() function directly in case (for some reason that I can't think of!) more code is added to automated_cron_modules_installed() that shouldn't be executed if a theme is installed.

Awesome 👍 That needs few words about new flag I guess

++To approach!

$this->state->set('system.cron_needed', FALSE);
Please do not store false in database, that's why I used to delete this "semaphore"

Exactly that, thank you!

One mire nit It is reset to FALSE at the start of a cron run. needs to paraphrase to missing state instead of false

Just a minor clean-up and nit

+++ b/core/lib/Drupal/Core/Cron.php
@@ -133,6 +142,9 @@ public function run() {
+      // Reset the cron needed state variable, so it can be reset during
+      // cron run if another run is needed.
+      $this->state->delete('system.cron_needed');
       $this->invokeCronHandlers();

I guess `can be reset` should be `can be set during run`

I think it's ok to RTBC by me as approach by mjpa who needs to be added to commit credit

I have a concern about the approach. Since search_cron sets the cron_needed flag nearly every request could end up running cron if there is a lot of content to index. I'm not sure that's a desired behaviour.

++ to get PM POV, meantime linked the issue to commerce recurring issue, it also wins if this flag will appear in core. Ref #2931290: Multiple recurring orders get created if cron is badly configured

As a bag-o-feature it may also help contrib that trying to clean-up "orphan files/media" but it brings division cron becomes more like queue

It seems to me it is more desirable to get the content indexed, than to wait for days potentially while it is not indexed. If someone is actually using core Node Search, wouldn't they want it to work?

Yes but they also need the rest of their site to work, which is why we set the default automated cron to three hours and encourage people to set up a regular cron job. Some sites have both a regular cron job and automated cron - which will never run if cron runs every 5 minutes, but it's a fallback in case the regular cron stops for some reason. With this change, you could end up with automated cron running all the time, which at the least will potentially tied up an apache process.

I do think it's a good idea for module install/uninstall though.

I recall a "throttle" core module in early days, as it gone every module forced to throttle execution (aggregator, update, locale,...)

All modules share cron hook so extra api to balance throttling would be great to adopt for help topics in blocked issue

Patch #35 applied cleanly. Moving to RTBC +1

Patch in #35 looks fine to me.

On the topic of the search module setting the flag each cron run if it's not fully indexed, only 1 request can run the automated cron, and once it's running, no others will run the automated cron. So at worst, you lose a single process that can serve the site. That shouldn't stop a site working, unless the server only serves 1 request at a time. Yes, it could stop people browsing the site if the site is busy as it's 1 less useable process, but then the site is already slow anyway.

The only issue with the patch from #26 I can think of, would be if indexing some content consistently fails, so you end up running cron all the time...

+++ b/core/modules/system/system.module
@@ -1304,3 +1304,19 @@ function system_theme_registry_alter(array &$theme_registry) {
+/**
+ * Implements hook_modules_installed().
+ */
+function system_modules_installed() {
+  // Set flag saying cron needs to be run.
+  \Drupal::state()->set('system.cron_needed', TRUE);
+}

Do we need this flag in system module?

The reason we use this flag is for automated cron only. Imo we could implement this better if we listened to these events in the automated cron module and set something on the event. To run the automated cron. This would have the additional benefit that you know for sure that it is the request that has installed the module that has run cron - and therefore has the correct container. With the patch as it currently stands it is theoretically possible that a slow request could end up running cron with a container built before the module / theme was installed.

@jhodgdon the system module does not need to be involved at all and we should not use state because of the container reason I gave in #44

Here's what needs to be done:

• Move the hooks to automated_cron
• Remove the use of state
• Add a new public method to \Drupal\automated_cron\EventSubscriber\AutomatedCron - ::forceCronRun() - which sets a protected property $this->forceCronRun to TRUE.
• In \Drupal\automated_cron\EventSubscriber\AutomatedCron::onTerminate() run cron if $this->forceCronRun === TRUE.

Alternatively, we could consider running cron when a module or theme is installed. We'd need to determine if this should be via the API (ModuleInstaller / ThemeInstaller) or only via the UI.

I do partially feel that this is the wrong solution for the problem. I feel like if an admin does a search and any of the search indexes involved in the search are not at 100% they should get a message about the search indexes not being complete. I think trying to automatically run cron in these situations is bound to end up triggering edge cases that are hard to debug - for example, some static cache that is not tied to the container and is incorrect after module installation but is correct on the next request.

protected property $this->forceCronRun to TRUE.

It will not work because this state is not stored between requests and as #45 states it orthogonal to automated cron - it's about ability to react on any event and request cron time for something that any extension may need - content re-index is just one of cases.

Moreover cron is core service so only possible namespace for cron state is system module

@alexpott if automated cron is not enabled help_topics's search or any other module needs ability to say "run cron to make me working after install"

@andypost well solution in #35 only works via automated cron... and it will work when a module is enabled via the UI. Maybe not drush because the request terminate will not fire but then #35 wouldn't work for drush because it opens up the possibility of cron being run with the wrong container...

Also "it's about ability to react on any event" - this is highly suspicious. I don't think we should have the ability to react on any event and force a cron run. I think cron should either be run by cron (or a cron like thing - hence automated cron) or by user interaction saying run cron. There might be a case for on module/theme install but I think that that is quite fraught for the reasons already outlined.

Re #49 - I think a message for privileged users on search when a search index is incomplete would be a great addition. I think this is one of the things that @catch asked for in https://www.drupal.org/project/drupal/issues/3087218#comment-13972153

Given the above I would close this as won't fix.