The way the progressive batch processing is handled make it extremely difficult to run the batch api outside the drupal http environment. Being able to use a different progress bar or in the case of shell no progress bar is impossible to use in the progressive mode.

In the case of the update.sh (Yet to be submitted) because of the use of the drupal_goto() using progressive mode is impossible as everything is needed to be run inside a php execution. Or if you are to embed the progress bar inside another update system it is not possible to replace the progressive implementation.

Basically what this patch does it allow the passing of a different callback instead of drupal_goto() to use so a different progressive system can be used.

So in the case of my update.sh I can pass a different callback to process the progressive batch API. This means that update.sh can progressively display the progress on the shell without having to do a whole heap and crap stuff.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

apaderno’s picture

Title: Change to Batch API. » Changes to batch API
yched’s picture

Interesting approach.

+++ includes/form.inc
@@ -2854,8 +2854,11 @@ function batch_set($batch_definition) {
-function batch_process($redirect = NULL, $url = NULL) {
+function batch_process($redirect = NULL, $url = NULL, $redirect_callback = 'drupal_goto') {

I'd recommend using NULL as a default instead, and using 'drupal_goto' if NULL within the code.

+++ includes/form.inc
@@ -2905,7 +2908,9 @@ function batch_process($redirect = NULL, $url = NULL) {
+      if ($redirect_callback) {
+        call_user_func_array('drupal_goto', array($batch['url'], 'op=start&id=' . $batch['id']));
+      }

I guess you meant if (drupal_function_exists($redirect_callback)), and $redirect_callback instead of 'drupal_goto' ;-)
Also, in the case of a callback different than drupal_goto, you might want to provide arguments in a nicer form than 'a url' and 'op=start&id=' . $batch['id'], which are really massaged for drupal_goto and a http execution. Sounds like the batch id should be enough, right ?

Beer-o-mania starts in 10 days! Don't drink and patch.

gordon’s picture

1. I did consider this, but I being able to set the $redirect_callback to NULL is a good thing it you want to skip the processing all together and go back to the calling function. I actually would have liked a callback that had different parameters to drupal_goto() but I felt that this would cause too much of an impact.

2. Yes I agree, and I have made the changes that were listed.

yched’s picture

being able to set the $redirect_callback to NULL is a good thing it you want to skip the processing all together and go back to the calling function

Not sure what you mean. In which cases would you want to call batch_process() just to skip the processing and return to the caller, leaving a stale, non-executed batch in the {batch} table ?

From my #1: "Also, in the case of a callback different than drupal_goto, you might want to provide arguments in a nicer form than 'a url' and 'op=start&id=' . $batch['id'], which are really massaged for drupal_goto and a http execution. Sounds like the batch id should be enough, right ?"
I still think that makes sense ;-)

gordon’s picture

Not sure what you mean. In which cases would you want to call batch_process() just to skip the processing and return to the caller, leaving a stale, non-executed batch in the {batch} table ?

Yes and no. It just means that it will be returned to the calling function and then the calling function would need to call _batch_process() itself.

I have now changed the default to NULL, and if you want to do this you can just pass FALSE and it will return without the batch being processed.

I have also added in the different callbacks for when using drupal_goto() as opposed to a custom callback. I don't like it as it makes the API ugly, but there is precedence in the menu system with the 'title callback' working different for t() as opposed to everything else.

gordon’s picture

I was just thinking about this and I was wondering if a function should be added to allow the running progressive batch processes from the shell?

Let me know what you think and I will extend this patch.

moshe weitzman’s picture

Status: Needs review » Needs work

drupal_function_exists() just got removed.

yched’s picture

"I was just thinking about this and I was wondering if a function should be added to allow the running progressive batch processes from the shell?"

Not sure what you mean - if that's code showing an actual use case for the feature, then it would possibly be a good idea, yes.

gordon’s picture

Status: Needs work » Needs review
FileSize
4.74 KB

I have fixed up the issue with the drupal_function_exists() and I have added the new _batch_do_shell() which can be used to process batches from a shell.

yched’s picture

+++ includes/batch.inc
@@ -466,3 +471,23 @@ function _batch_shutdown() {
+/**
+ * Batch do shell
+ */
+function _batch_do_shell($batch_id, $op) {

Can't go in as-is. At least needs a better PHPdoc. And the function name is awkward, since this is not a 'shell' equivalent of _batch_do()

+++ includes/form.inc
@@ -2846,8 +2846,11 @@ function batch_set($batch_definition) {
+ * @param $redirect_callback
+ *   (optional) Specify a function to be called to redirect to the progressive
+ *   processing page.

The comment could be better IMO. The whole point is to not redirect to *the* progressive processing *page*. This is about non-HTML progress handlers.

+++ includes/batch.inc
@@ -466,3 +471,23 @@ function _batch_shutdown() {
+  else if ($op == 'finish') {
+    return array(TRUE, '');
+  }

array (TRUE, '') ? Why ?

Other than that, I think its a really interesting addition. Hate to be a pain, but I think it should be taken a step further and have batch API's own drupal_goto()s be handled in a callback similar to your _batch_do_shell(). I don't think this should hold the patch so close to freeze, but it would be a needed cleaning followup, IMO.

gordon’s picture

I have made more changes as above. I have removed the _batch_do_shell as I felt it could not be made generic enough.

Status: Needs review » Needs work

The last submitted patch failed testing.

lilou’s picture

Status: Needs work » Needs review
zzolo’s picture

Status: Needs review » Needs work

Reviewed:

      if (is_null($redirect_callback) || $redirect_callback == 'drupal_goto') {
        drupal_goto($batch['url'], 'op=start&id=' . $batch['id']);
        
      }

* Extra blank line.
* Also, is there a test for this? Is it possible with the testing framework, since it uses this functionality?
* Also requested a re-test which passed.

gordon’s picture

I have fixed up the extra line.

Also there is a test for the batchapi in there, which is why I have not created any tests.

Gordon.

gordon’s picture

Status: Needs work » Needs review
moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for sticking with this, gordon. This refactor is gonna be very useful for aegir, drush, field migration, data migration, and custom scripts everywhere.

hass’s picture

Command line batches - *great* :-)

sun’s picture

+++ includes/batch.inc
@@ -450,7 +450,12 @@ function _batch_finished() {
-    drupal_goto($_batch['source_page']);
+    if (is_null($_batch['redirect_callback']) || $_batch['redirect_callback'] == 'drupal_goto') {
+      drupal_goto($_batch['source_page']);
+    }
+++ includes/form.inc
@@ -2929,7 +2935,12 @@ function batch_process($redirect = NULL, $url = NULL) {
-      drupal_goto($batch['url'], 'op=start&id=' . $batch['id']);
+      if (is_null($redirect_callback) || $redirect_callback == 'drupal_goto') {
+        drupal_goto($batch['url'], 'op=start&id=' . $batch['id']);
+      }

Watch out for PHPWTF.

# php -r "$foo['bar'] = FALSE; var_dump( ($foo['bar'] == 'drupal_goto') );"
bool(false)

# php -r "$foo['bar'] = TRUE; var_dump( ($foo['bar'] == 'drupal_goto') );"
bool(true)

# php -r "$foo['bar'] = TRUE; var_dump( ($foo['bar'] === 'drupal_goto') );"
bool(false)
+++ includes/batch.inc
@@ -450,7 +450,12 @@ function _batch_finished() {
+    else if ($_batch['redirect_callback'] && function_exists($_batch['redirect_callback'])) {
+++ includes/form.inc
@@ -2929,7 +2935,12 @@ function batch_process($redirect = NULL, $url = NULL) {
+      else if ($redirect_callback && function_exists($redirect_callback)) {

The coding standard is "elseif".

This review is powered by Dreditor.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

gordon’s picture

Status: Needs work » Needs review
FileSize
4.51 KB

thanks @sun I have made those couple of changes.

sun’s picture

+++ includes/batch.inc
@@ -450,7 +450,12 @@ function _batch_finished() {
+    if (is_null($_batch['redirect_callback']) || $_batch['redirect_callback'] === 'drupal_goto') {
...
+    elseif ($_batch['redirect_callback'] && function_exists($_batch['redirect_callback'])) {
+++ includes/form.inc
@@ -2940,7 +2946,12 @@ function batch_process($redirect = NULL, $url = NULL) {
+      if (is_null($redirect_callback) || $redirect_callback === 'drupal_goto') {
...
+      elseif ($redirect_callback && function_exists($redirect_callback)) {

On a second view - why do we test for is_null() instead of empty() here?

If it's empty or 'drupal_goto', do this. Otherwise, it must be non-empty (first condition in elseif is superfluous), and if the function exists, execute it.

This review is powered by Dreditor.

gordon’s picture

I have made the changes to use empty() instead of is_null() I actually have a feeling that empty() is a bit faster than is_null()

sun’s picture

+++ includes/batch.inc
@@ -450,7 +450,12 @@ function _batch_finished() {
+    if (empty($_batch['redirect_callback']) || $_batch['redirect_callback'] === 'drupal_goto') {
+      drupal_goto($_batch['source_page']);
+    }
+    elseif ($_batch['redirect_callback'] && function_exists($_batch['redirect_callback'])) {
+      call_user_func_array($_batch['redirect_callback'], array($_batch['id']), 'finish');
+    }
+++ includes/form.inc
@@ -2940,7 +2946,12 @@ function batch_process($redirect = NULL, $url = NULL) {
+      if (empty($redirect_callback) || $redirect_callback === 'drupal_goto') {
+        drupal_goto($batch['url'], 'op=start&id=' . $batch['id']);
+      }
+      elseif ($redirect_callback && function_exists($redirect_callback)) {
+        call_user_func_array($redirect_callback, array($batch['id']), 'start');
+      }

In the elseif, 'redirect_callback' will always be !empty() now, so you can remove the first condition before the &&.

This review is powered by Dreditor.

gordon’s picture

What do you mean, empty(NULL) === TRUE so if a $redirect_callback is left blank then it is set to NULL. And thus will go to the first if

Gordon.

sun’s picture

FileSize
3.99 KB

yep, that's what I meant -- the first condition will evaluate to TRUE if redirect_callback is NULL, '', FALSE, or 0, or if it's explicitly 'drupal_goto'.

In the elseif, the first condition tested the same again, which will always evaluate to TRUE, so it's superfluous to test it.

gordon’s picture

Status: Needs review » Reviewed & tested by the community

Yes I get you now. I was going to fix this, but you bet me to it.

sun’s picture

Issue tags: +API clean-up

Tagging.

mattyoung’s picture

sub

Dries’s picture

Tests? This seems like the kind of new feature that could really use some tests.

gordon’s picture

Actually I thought about this, and after looking at what tests were available currently it is really being tested completely, because the entire test system is a massive implementation of the batch API.

There are currently no implementation of this, as I didn't get time to submit my shell update.php implementation which does implement this.

The current implementation is heavily tested. other than the benefit of showing an implementation I am not sure it will accomplish much.

If you disagree I will look at implementing a test for it.

sun’s picture

I don't think it's easy to test this, since testing is running in a batch already... no? If you have an idea about how this could be tested, please do.

+++ includes/form.inc	18 Sep 2009 14:58:52 -0000
@@ -2956,7 +2962,12 @@ function batch_process($redirect = NULL,
+      if (empty($redirect_callback) || $redirect_callback === 'drupal_goto') {
+        drupal_goto($batch['url'], 'op=start&id=' . $batch['id']);
+      }
+      elseif (function_exists($redirect_callback)) {
+        call_user_func_array($redirect_callback, array($batch['id']), 'start');
+      }

Why do we call this with completely different arguments?

Let's use the array syntax for $query instead and let any alternative redirect callbacks figure out how to handle the 'op' instead of passing another argument.

Additionally, since we already check whether the function exists, we can simply invoke $redirect_callback as function.

This review is powered by Dreditor.

gordon’s picture

+++ includes/batch.inc	21 Sep 2009 20:20:02 -0000
@@ -445,7 +444,15 @@ function _batch_finished() {
+    if (empty($_batch['redirect_callback'])) {
+      $redirect_callback = 'drupal_goto';
+    }
+    else {
+      $redirect_callback = $_batch['redirect_callback'];
+    }

I have removed this since this is a private function and it should be set in batch_progress().

+++ includes/form.inc	21 Sep 2009 20:14:39 -0000
@@ -2947,6 +2952,7 @@ function batch_process($redirect = NULL,
       'redirect' => $redirect,
       'theme' => $GLOBALS['theme_key'],
+      'redirect_callback' => $redirect_callback,
     );

Make sure this has the drupal_goto set.

+++ includes/form.inc	21 Sep 2009 20:14:39 -0000
@@ -2984,7 +2990,12 @@ function batch_process($redirect = NULL,
+      if (empty($redirect_callback)) {
+        $redirect_callback = 'drupal_goto';
+      }

Take this information from $batch

I'm on crack. Are you, too?

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

gordon’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
4.46 KB
+++ includes/form.inc
@@ -2947,6 +2952,7 @@ function batch_process($redirect = NULL, $url = NULL) {
+      'redirect_callback' => empty($redirect_callback) ? 'drupal_callback' : $redirect_callback,

Fixed name of incorrect function.

This review is powered by Dreditor.

sun’s picture

Awesome! This really looks ready to fly now! :)

chx’s picture

Status: Reviewed & tested by the community » Needs work

Erm. Some comments earlier we had the possibility to 'flat out' the batch by changing the callback to NULL or FALSE. Do we have a batch alter? Please add if we dont. Also,

 $_batch['redirect_callback']($_batch['source_url'], array('op' => 'finish', 'id' => $_batch['id']));

makes me run screaming. May I get

$function = $_batch['redirect_callback'];
$function($_batch['source_url'], array('op' => 'finish', 'id' => $_batch['id']));

please?

gordon’s picture

I have changed it as per your request. Much better readability.

gordon’s picture

Status: Needs work » Needs review
chx’s picture

I thought we were discussing a batch alter so a command line script can call and change an existing function which in turn calls the batch API..?

gordon’s picture

I have added the drupal_alter() as it would be a great thing as you could do things like change the run method to something else which can fork the process and run it in the background, and this could be done by a contrib module.

Also I found some duplicate code which did "isset($url) ? $url : 'batch'" twice so I have fixed that as well.

It is also a pity that the batch doesn't have some form of identifier like the formapi has in $form_id so that it would make it easier to target certain batch processes. This would nice but it is outside the current scope of the issue.

yched’s picture

+    if (function_exists($function)) {
+      $batch($_batch['source_url'], array('op' => 'finish', 'id' => $_batch['id']));
+    }

Doesn't look right.

As gordon points out, batch alter is not really worth without a batch id to 'recognize' the batch you wnat to alter. The thing is that 2 separate form submit handlers, in possibly 2 different modules that don't even know each other, can set up a batch on a given form. This results in one batch with two independant 'sets' of operations and 'finished' callbacks. Then, which one decides of the batch id ?

gordon’s picture

I have fixed up the $batch -> $function

Without the batch identifier you can still make use of it, but it is just harder.

chx’s picture

Yeah you need to have some knowledge of what batch will be triggered by you calling some other functions with certain inputs and pass this knowledge to the batch alter. Not trivial but doable. It's not like this needs to be done often anyways. Doable is enough. Easy is not a priority here.

sun’s picture

+++ includes/form.inc
@@ -2931,8 +2931,13 @@ function batch_set($batch_definition) {
-function batch_process($redirect = NULL, $url = NULL) {
+function batch_process($redirect = NULL, $url = NULL, $redirect_callback = NULL) {
@@ -2943,13 +2948,16 @@ function batch_process($redirect = NULL, $url = NULL) {
-      'url' => isset($url) ? $url : 'batch',
+      'url' => $url,
...
+      'redirect_callback' => empty($redirect_callback) ? 'drupal_goto' : $redirect_callback,
+++ includes/update.inc
@@ -368,8 +368,11 @@ class DrupalUpdateException extends Exception { }
-function update_batch($start, $redirect = NULL, $url = NULL, $batch = array()) {
+function update_batch($start, $redirect = NULL, $url = NULL, $batch = array(), $redirect_callback = NULL) {

Why don't we define the default value in the function argument already?

The same seems to apply to the $url argument...

+++ includes/form.inc
@@ -2943,13 +2948,16 @@ function batch_process($redirect = NULL, $url = NULL) {
+    drupal_alter('batch', $batch);

Can we squeeze a short comment above this line explaining how this hook could be useful? As yched pointed out, it seems like it's only useful to unconditionally alter all batches.

I'm on crack. Are you, too?

gordon’s picture

I have made the changes as above.

I think this is there now. I think it is starting to go round in circles a bit.

sun’s picture

+++ includes/form.inc
@@ -2931,8 +2931,13 @@ function batch_set($batch_definition) {
-function batch_process($redirect = NULL, $url = NULL) {
+function batch_process($redirect = NULL, $url = NULL, $redirect_callback = 'drupal_goto') {
@@ -2943,13 +2948,17 @@ function batch_process($redirect = NULL, $url = NULL) {
-      'url' => isset($url) ? $url : 'batch',
+      'url' => $url,

$url argument now needs to default to 'batch'.

+++ includes/form.inc
@@ -2943,13 +2948,17 @@ function batch_process($redirect = NULL, $url = NULL) {
+    // allow hook_batch_alter() to make changes to the batch.

Comments should always form proper sentences with proper capitalization.

I'm on crack. Are you, too?

gordon’s picture

I have made the changes as above.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Erm. Great, #47 or #48 it is then - both are almost identical ;)

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

gordon’s picture

Here is a fixed version of the patch which now applies.

gordon’s picture

Status: Needs work » Needs review
moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Seems like we have consensus here. Code looks good.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Although this introduces a new feature, I decided to go ahead and committed this change.

Status: Fixed » Closed (fixed)

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

yched’s picture