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.
Comment | File | Size | Author |
---|---|---|---|
#16 | drupal_cron_run_return_param_d7-1816008-15.patch | 519 bytes | Kuldip Gohil |
#11 | drupal_cron_run_return_param_d7-1816008-10.patch | 568 bytes | Kuldip Gohil |
#8 | drupal_cron_run_return_param_d7-1816008-4.patch | 581 bytes | Kuldip Gohil |
#5 | drupal_cron_run_return_param_d7-1816008-4.patch | 577 bytes | Kuldip Gohil |
| |||
#3 | 1816008-drupal_cron_run_return_docs.patch | 546 bytes | Cameron Tod |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedI'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?
Comment #2
peterx CreditAttribution: peterx commentedThis 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.
Comment #3
Cameron Tod CreditAttribution: Cameron Tod commentedI 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.
Comment #4
jhodgdonThanks 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".
Comment #5
Kuldip Gohil CreditAttribution: Kuldip Gohil as a volunteer and commentedPatch uploaded for D7 version, did not find drupal_cron_run() function in common.inc in D8 so please update here for D8.
Comment #6
Kuldip Gohil CreditAttribution: Kuldip Gohil as a volunteer and commentedComment #8
Kuldip Gohil CreditAttribution: Kuldip Gohil as a volunteer and commentedComment #9
Kuldip Gohil CreditAttribution: Kuldip Gohil as a volunteer and commentedComment #10
jhodgdonThanks for the patch!
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"?
Comment #11
Kuldip Gohil CreditAttribution: Kuldip Gohil as a volunteer and commentedHi Jennifer,
I've updated and recreated patch, please review.
Thanks
Comment #12
Kuldip Gohil CreditAttribution: Kuldip Gohil as a volunteer and commentedComment #13
jhodgdonThanks but...
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!
Comment #14
Kuldip Gohil CreditAttribution: Kuldip Gohil as a volunteer and commentedHi Jennifer,
I'm thinking to put simply below (single line), can you please review and suggest?
Comment #15
jhodgdonYes, 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. :)
Comment #16
Kuldip Gohil CreditAttribution: Kuldip Gohil as a volunteer and commentedPlease find updated patch
Comment #17
Kuldip Gohil CreditAttribution: Kuldip Gohil as a volunteer and commentedComment #18
jhodgdonThanks, looks good!
Comment #19
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedCommitted 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.