Currently i am not able to wrap my batch operations in a class(which is very often the use case) because of this in batch.inc:
if (function_exists($batch_set['finished'])) {

If we change this to
if (is_callable($batch_set['finished'])) {

We can do this:
$batch['finished'] = array($this, 'method')

This already works with the actual batch operation callback, just not with the finished callback.

Now, i will submit patches for both 7.x and 8.x and foresee already war emerging on this, but i really hope it will be accepted, otherwise i'll have to resort to some really dirty solutions(move the function out from the class or something) and the client wont exactly buy me a champaign.

CommentFileSizeAuthor
#9 1924420_7.patch2.9 KBchx
#7 1924420_7.patch4.25 KBchx
#5 drupal-replace_function_exists_with_is_callable-1924429-5.patch2.16 KBtwistor
#2 drupal-replace_function_exists_with_is_callable-1924429-1.patch1 KBbfr

Comments

bfr’s picture

bfr commented

Here's the first one.

edit: The upload form is broken, will try again later.

bfr’s picture

bfr commented
StatusFileSize
new drupal-replace_function_exists_with_is_callable-1924429-1.patch1 KB

New try. Uploading seems to work with Firefox but not Chrome? Or is my Chrome broken?

bfr’s picture

bfr commented
Status: Active » Needs review
bfr’s picture

bfr commented

Backport here.

twistor’s picture

twistor commented
Title: Replace function_exists() with is_callable() in batch api finished callback. » Replace function_exists() with is_callable() in batch.inc.
Assigned: bfr » twistor
Category: feature » bug
Priority: Major » Normal
StatusFileSize
new drupal-replace_function_exists_with_is_callable-1924429-5.patch2.16 KB

This problem also exists with the new FormInterface.

I'm currently seeing this in _batch_next_set() while trying to spawn a batch process from EntityNGConfirmFormBase::submit().

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

#5: drupal-replace_function_exists_with_is_callable-1924429-5.patch queued for re-testing.

chx’s picture

chx commented
Issue summary: View changes
StatusFileSize
new 1924420_7.patch4.25 KB
xano’s picture

xano
he/they
Primary language English
Location Southampton
commented 
  1. +++ b/core/includes/batch.inc
@@ -237,7 +237,7 @@ function _batch_process() {
+      list($callable, $args) = $item->data;

    Could we name it $callback for the sake of clarity? callable sounds like a type hint. 

  2. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockSaveTest.php
@@ -49,13 +49,13 @@ public function testImport() {
-      'body' => array(Language::LANGCODE_NOT_SPECIFIED => array(array('value' => $this->randomName(32)))),

    Is this an accidental left-over from another patch?

chx’s picture

chx commented
StatusFileSize
new 1924420_7.patch2.9 KB

The last submitted patch, 7: 1924420_7.patch, failed testing.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented 
+++ b/core/includes/batch.inc
@@ -458,11 +458,11 @@ function _batch_finished() {
+      $callback($_batch['source_url'], array('query' => array('op' => 'finish', 'id' => $_batch['id'])));

This only works in PHP 5.4 and higher: http://3v4l.org/ERMcQ, so we can't commit this until our testing infra supports 5.4 all the way. The rest of the patch looks good. RTBC if the bot agrees.

chx’s picture

chx commented

Good then that Drupal 8 requires PHP 5.4 (even if this is not yet committed).

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Status: Needs review » Reviewed & tested by the community

RTBC, on condition that the tests pass.

chx’s picture

chx commented
Priority: Normal » Major

Blocks migrate, bumping up.

catch’s picture

catch
he/him
Primary language English
commented

I'm confused by #11 - is that passing because we're not testing this?

chx’s picture

chx commented

Well, sorta, it's completely legal as long as you don't actually use an array as your callable which core currently doesn't for any of these callbacks. Redirect callbacks are used extremely rarely, for example 0 times in core.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

OK I think we can live with that level of obscure incompatibility until the bots are actually upgraded.

Committed/pushed to 8.x, thanks!

twistor’s picture

twistor commented
Assigned: twistor » Unassigned

Status: Fixed » Closed (fixed)

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