Problem/Motivation

drupal_cron_run() invokes hook_cron_queue_info() hooks and processes items registered there by passing queue items data to a worker (worker callback). But the invoking code (drupal_cron_run()) it only allows passing of procedural functions not callables in the general meaning of http://php.net/manual/language.types.callable.php.

This is bug that prevent using of class methods as worker callbacks.

Proposed resolution

Change drupal_cron_run() to allow REAL worker callbacks.

Remaining tasks

  1. Patch
  2. RTBC
  3. Commit

User interface changes

None.

API changes

Developers are able now to pass a callable as worker callback in hook_cron_queue_info().

CommentFileSizeAuthor
#18 2342667-cron-and-batch-callables.patch3.83 KBDave Reid
#15 cron_and_batch-2342667-15.patch3.95 KBclaudiu.cristea
#11 cron_and_batch-2342667-11.patch4.55 KBclaudiu.cristea
#11 interdiff.txt398 bytesclaudiu.cristea
#9 cron_and_batch-2342667-9.patch4.94 KBclaudiu.cristea
#5 cron_processing_of-2342667-5.patch5.01 KBclaudiu.cristea
#5 interdiff.txt3.63 KBclaudiu.cristea
#1 cron_processing_of-2342667-1.patch2.02 KBclaudiu.cristea
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea commented
Status: Active » Needs review
FileSize
cron_processing_of-2342667-1.patch2.02 KB

Patch

dcam’s picture

dcam CreditAttribution: dcam commented
Related issues: +#2309539: Inconsistency in callback processing

There is duplication of work between this issue and #2309539: Inconsistency in callback processing. The patches should be merged and one of the issues should be closed. Normally, the newer issue (this one) is the one that should be closed.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea commented

Each occurence needs to ne researched and treated separately because it depends on specific context. I wouldn't mix them in a single issue. Moreover, I don't have time resouces to tackle that ticket. This just pop up because of a specific project where I needed this. So, from my point of view it's a "take it or leave it".

MiroslavBanov’s picture

MiroslavBanov CreditAttribution: MiroslavBanov commented

@claudiu
If there is interest in the community, someone will tackle the ticket.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea commented
FileSize
interdiff.txt3.63 KB
cron_processing_of-2342667-5.patch5.01 KB

OK. Here's a patch including the batch processing and I added also a test. I will close the other one because my last patch is continuing this one.

@dcam, can you please review this? Thank you!

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea commented
Title: Cron processing of queues doesn't accept callables » Cron and batch processing of queues are not accepting callables

claudiu.cristea queued 5: cron_processing_of-2342667-5.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 5: cron_processing_of-2342667-5.patch, failed testing.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea commented
Status: Needs work » Needs review
FileSize
cron_and_batch-2342667-9.patch4.94 KB

Rerolled.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented 
+files[] = cron_queue_test.module

Not sure the above is needed, although otherwise the patch looks good.

However, is this patch even necessary? At least on PHP 5.4, this seems to work fine:

$function = array('ClassName', 'methodname');
$function();

And if I run the tests on PHP 5.4 they pass even without the rest of the patch...

I can't seem to find clear documentation of this behavior on php.net though. Will the tests fail (without the rest of the patch) in earlier versions of PHP or something?

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea commented
FileSize
interdiff.txt398 bytes
cron_and_batch-2342667-11.patch4.55 KB

Here are some manual tests:

PHP 5.4

$ php -v
PHP 5.4.28 (cli) (built: May  5 2014 15:10:56)

$ php -r "
class foo {
  static function bar() {
    echo 'baz';
  }
}
\$function = array('foo', 'bar');
\$function();
"
baz

PHP 5.3

$ php -v
PHP 5.3.28 (cli) (built: May  5 2014 13:57:34)

$ php -r "
class foo {
  static function bar() {
    echo 'baz';
  }
}
\$function = array('foo', 'bar');
\$function();
"
PHP Fatal error:  Function name must be a string in Command line code on line 8

The lazy loader, yes, we don't need that.

EDIT: Removed version copyright info for readability reasons.

claudiu.cristea queued 11: cron_and_batch-2342667-11.patch for re-testing.

ndobromirov’s picture

ndobromirov CreditAttribution: ndobromirov commented

In the part of the patch: 

...
diff --git a/includes/batch.inc b/includes/batch.inc
index 061acd4..c628900 100644
--- a/includes/batch.inc
+++ b/includes/batch.inc
@@ -463,7 +463,7 @@ function _batch_finished() {
       if (function_exists($batch_set['finished'])) {
...

Needs to check is_callable instead of function_exists, as the later is invalid for lambda functions and class methods.

ndobromirov’s picture

ndobromirov CreditAttribution: ndobromirov commented
Status: Needs review » Needs work
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Status: Needs work » Needs review
FileSize
cron_and_batch-2342667-15.patch3.95 KB

Patch.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Issue tags: +callables
Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
FileSize
2342667-cron-and-batch-callables.patch3.83 KB

Re-rolled for CHANGELOG.txt.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented

@Dave Reid, as you only made a reroll without providing a patch it would be nice if you can provide a review or... RTBC. This issue is here for a long time :)

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

I didn't believe I can RTBC my own patch, even if it is a reroll.

ndobromirov’s picture

ndobromirov CreditAttribution: ndobromirov commented
Status: Needs review » Reviewed & tested by the community

Seems good.
Tests are passing, the code makes no regressions and provides only enhancements.
Marking it as RTBC.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +7.40 release notes

Committed to 7.x - thanks!

Not sure if we should expand some of the documentation, e.g. hook_cron_queue_info(), to highlight this too. Although since this is only something core supports right now (no guarantee any already-written code elsewhere that invokes the same hook supports it) maybe it's better to leave this as is and just have it as a semi-hidden feature for the time being.

  • David_Rothstein committed 3d1be9b on 7.x 
    Issue #2342667 by claudiu.cristea, Dave Reid, ndobromirov: Cron and...
ndobromirov’s picture

ndobromirov CreditAttribution: ndobromirov at FFW commented
Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid at Lullabot commented

Status: Fixed » Closed (fixed)

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