API page: http://api.drupal.org/api/drupal/includes!common.inc/function/drupal_cro...

Enter a descriptive title (above) relating to drupal_cron_run, then describe the problem you have found:

The function returns TRUE when the cron run runs and FALSE if it cannot acquire the cron lock.

You might also want to note that the called functions can fail and leave the user logged in as anonymous with a fake session. This is relevant when calling the function from user code.

CommentFileSizeAuthor
#16 drupal_cron_run_return_param_d7-1816008-15.patch519 bytesKuldip Gohil
#11 drupal_cron_run_return_param_d7-1816008-10.patch568 bytesKuldip Gohil
#8 drupal_cron_run_return_param_d7-1816008-4.patch581 bytesKuldip Gohil
#5 drupal_cron_run_return_param_d7-1816008-4.patch577 bytesKuldip Gohil
#3 1816008-drupal_cron_run_return_docs.patch546 bytescameron tod

Comments

Anonymous’s picture

Anonymous (not verified) commented
Status: Active » Postponed (maintainer needs more info)

I'm not seeing that in the code. The user session is restored before returning even if a lock isn't acquired. Perhaps something else is going wrong for you?

peterx’s picture

peterx commented
Status: Postponed (maintainer needs more info) » Active

This issue is the code returning FALSE if the cron lock is not acquired, something that does happen when cron is run by hand at the same time as cron is running automatically. It can occur if you use drupal_cron_run() direct in your code.

The other potential problem, which can be ignored for this issue, is the problem where user code, a module, calls this function then this function calls a third function, one of the cron functions in a module. and the third module fails. You get a white screen of death. When the user presses the back button, they are logged out because the failure was in the middle of the drupal_cron_run() function.

An example is the submission of a cron run on node_save. In my code I copied drupal_cron_run to create cron_module($module) and will add try{} around the cron process function to ensure the cron queue submission always completes without WSOD.

The WSOD problem should not distract from documenting the FALSE return code.

cameron tod’s picture

cameron tod commented
Title: returns TRUE or FALSE » drupal_cron_run() @return parameter documentation incorrect, should specify returns TRUE or FALSE
Version: 7.x-dev » 8.x-dev
Status: Active » Needs review
StatusFileSize
new 1816008-drupal_cron_run_return_docs.patch546 bytes

I don't see there's a big issue with changing the docs here. My reading of the code suggests the function definitely does return an explicit FALSE if no lock is acquired.

There will be overlap with #1326456: Add missing @param in includes A-C, plus other corrections to some docblocks, so I have posted a comment there linking this issue. This change will probably be incorporated into that issue.

All fixes go against 8.x first, then are backported to 7.x and 6.x, so if this change gets marked RTBC, we can look at backporting it to Drupal 7.

Regarding the second issue you mention in #2, if you feel it is worthy of a bug report, please search through the core queue, and if you can't find a discussion of it, post a new issue.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work
Issue tags: +Needs backport to D7

Thanks for all the back and forth here and the patch!

I agree, the function definitely returns true or false. But I think the documentation if it is going to mention "a lock" should explain what that means. Also, after the comma there should be "and".

Kuldip Gohil’s picture

Kuldip Gohil commented
Issue summary: View changes
StatusFileSize
new drupal_cron_run_return_param_d7-1816008-4.patch577 bytes

Patch uploaded for D7 version, did not find drupal_cron_run() function in common.inc in D8 so please update here for D8.

Kuldip Gohil’s picture

Kuldip Gohil commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: drupal_cron_run_return_param_d7-1816008-4.patch, failed testing.

Kuldip Gohil’s picture

Kuldip Gohil commented
Version: 8.0.x-dev » 7.39
StatusFileSize
new drupal_cron_run_return_param_d7-1816008-4.patch581 bytes
Kuldip Gohil’s picture

Kuldip Gohil commented
Version: 7.39 » 7.x-dev
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Thanks for the patch!

+++ b/includes/common.inc
@@ -5308,8 +5308,8 @@ function drupal_page_set_cache() {
+ *   TRUE if cron ran successfully, and FALSE if a lock (it does not allow to re-run while its already running) could not be acquired.

This line needs to be wrapped into several lines. We do not want API documentation lines to exceed 80 characters.

Also the wording seems a bit awkward... maybe the part in parentheses could be made into a separate sentence at the end rather than inserting it in the middle of "if a lock could not be acquired"?

Kuldip Gohil’s picture

Kuldip Gohil commented
StatusFileSize
new drupal_cron_run_return_param_d7-1816008-10.patch568 bytes

Hi Jennifer,

I've updated and recreated patch, please review.

Thanks

Kuldip Gohil’s picture

Kuldip Gohil commented
Status: Needs work » Needs review
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Thanks but...

+++ b/includes/common.inc
@@ -5309,7 +5309,8 @@ function drupal_page_set_cache() {
+ *   TRUE if cron ran successfully, or FALSE if a lock could not be acquired
+ *   i.e. it does not allow to re-run while its already running.

Can we make this into two sentences, and not use "i.e."? Also "it" in the second sentence has no antecedent so is confusing. And the grammar/punctuation are not correct... but if you rewrite it into two complete sentences that should take care of it. Thanks!

Kuldip Gohil’s picture

Kuldip Gohil commented

Hi Jennifer,

I'm thinking to put simply below (single line), can you please review and suggest?

* @return
*   TRUE if cron ran successfully and FALSE if cron is already running
*/
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Yes, that looks fine to me. Make sure to end in a . though, and on the @return line, add a type:
@return bool
for extra credit. :)

Kuldip Gohil’s picture

Kuldip Gohil commented
StatusFileSize
new drupal_cron_run_return_param_d7-1816008-15.patch519 bytes

Please find updated patch

Kuldip Gohil’s picture

Kuldip Gohil commented
Status: Needs work » Needs review
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Thanks, looks good!

David_Rothstein’s picture

David_Rothstein commented
Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks!

This is fixed a little differently in Drupal 8 (see CronInterface::run()) but it might be intentional that the documentation there is a little more vague since in theory you can swap out the cron service with something else.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.