Allow 'Cron run completed.' log message to be skipped

By default Drupal keeps the last 1000 log messages. While this can be increased, it comes with increased database size and processing time.

Providing the option to not log "Cron run completed." events where there are no errors would free up a lot of space and any errors won't disappear from the database log so quickly.

For example, we're running cron every minute on some sites. We use Ultimate Cron → to send any queued email every minute, while everything else is hourly or 3 hourly.

The reason for this is to improve the user experience. Users fill out a form which is submitted instantly when they click "Submit", with an email sent to them within a minute. Without queueing email it would take 3 or more seconds to submit the form.

I agree with @longwave, in this case there is no need to use null logger.

I agree with @longwave in this case, there is no need to use null logger.

+++ b/core/lib/Drupal/Core/Cron.php
@@ -311,8 +322,7 @@ protected function invokeCronHandlers() {
-    $logger = $time_logging_enabled ? $this->logger : new NullLogger();
+    $logger = $this->isDetailedLoggingEnabled() ? $this->logger : new NullLogger();
 
     // Iterate through the modules calling their cron handlers (if any):

Can the tests be expanded to capture when the setting is off and on?

Also IS still needs to be completed.

Plus #13

@imclean, @smustgrave, it would be nice if you read the full version of the code, and then write comments.
Let me clarify for you to understand.

If you check the patch from comment 2 and comment 6 from @longwave, you will see that in the proposed solution (patch 2), null logger is redundant and it is enough to use a common if condition.

Comment 7 mentions "pattern similar to Cron::invokeCronHandlers()" and if you look at the code it's appropriate to use a null logger to avoid adding a bunch of if conditions inside closure.

So my comment regarding the null logger applies exclusively to the setCronLastTime method.

@smustgrave,

Can the tests be expanded to capture when the setting is off and on?

\Drupal\Tests\dblog\Kernel\DbLogTest::testDbLogCron (starts on line 55) - this code covers on/off setting.

Updated patch included code style fixes

Reroll for D10.0.

Reroll for D10.0.

Had to update my phpcs version. Though, now I have more errors than the one used by Drupal pipeline. I hope it'll make it, otherwise I'll have to call it a day.

I agree it would be nice addition, but I don't know if we can introduce the change without a new config, the current behavior has been there for 6 years (since Drupal 8.3). At minimum it needs a change to form element descriptions for "Detailed cron logging" checkbox.

New patch to Drupal 10.1.x.

I used the same approach that in Cron::invokeCronHandlers().

Same path to Drupal 11.x

The last submitted patch, 20: D10.1-3292849-20.patch, failed testing. View results →

The last submitted patch, 21: D11-3292849-21.patch, failed testing. View results →

Require fix tests.

Reroll of #16 for 10.3.6 introducing this commit

This needs to be changed to use a branch instead of just a patch.

Looking at the implementation, I'm not sure we can add the config factory at that place in the argument list, since it will break implementations that have the other arguments already? It is a required argument, so maybe it is in the right place. I'm not sure about this.

We don't need to make this so complicated. Given that we already do

    $time_logging_enabled = \Drupal::config('system.cron')->get('logging');

then I think it's fine to copy-paste that to run() and be done here; we can handle injecting the config factory as a dependency in another issue.

I think we shouldn't be too far from a green pipeline.

It could require more tests though, to make sure when "logging" returns true or false.

For now, I've been working my way around to get it green. Dunno why it failed on the last one, will check tomorrow.

As #26 says, you can't just add a new argument to the constructor, especially in the middle of the argument list; contrib or custom code may extend this class - for example I'm fairly sure Ultimate Cron in contrib extends this service. If we are going to do that we need to add it to the end of the list and provide backward compatibility.

Yeah, it's kind of right.

Anyone who extended the cron might struggle with this new constructor.

So what's your stand? You think we should go for a static call?

As per discussion the null logger is actually needed, because it prevents code from failing, in case the logger is needed.

But I'm fine with any of the working solutions. True dat, if we implement a new constructor, anyone who extended the cron will have to make changes ....

aporie → changed the visibility of the branch 3292849-allow-cron-run-without-dependency-injection to hidden.

aporie → changed the visibility of the branch 3292849-allow-cron-run-without-dependency-injection to active.

Hi @longwave,

Sorry I overlooked your message in #30, it contained all the answers to my questions.

So ... I've been investigating, and I didn't manage to set @trigger_error('...', E_USER_DEPRECATED), at least not without breaking ultimate_cron. I actually need this patch for a project on which I have ultimate_cron installed, so it was easy to test.

I think the issue is that, whereas extending Cron.php constructor in ultimate_cron/src/UltimateCron.php, they alter the service provider in ultimate_cron/src/UltimateCronServiceProvider.php and add the config.factory using addMethodCall:

/**
   * {@inheritdoc}
   */
  public function alter(ContainerBuilder $container) {
    // Overrides cron class to use our own cron manager.
    $container->getDefinition('cron')
      ->setClass('Drupal\ultimate_cron\UltimateCron')
      ->addMethodCall('setConfigFactory', [new Reference('config.factory')]);
  }

I didn't really know which action to take, so for now I just implemented it using Drupal static call and without implementing a null constructor variable and adding the deprecation notice.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.