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.
Comments
Comment #1
apadernoComment #2
yched CreditAttribution: yched commentedInteresting approach.
I'd recommend using NULL as a default instead, and using 'drupal_goto' if NULL within the code.
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.
Comment #3
gordon CreditAttribution: gordon commented1. 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.
Comment #4
yched CreditAttribution: yched commentedNot 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 ;-)
Comment #5
gordon CreditAttribution: gordon commentedYes 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.
Comment #6
gordon CreditAttribution: gordon commentedI 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.
Comment #7
moshe weitzman CreditAttribution: moshe weitzman commenteddrupal_function_exists() just got removed.
Comment #8
yched CreditAttribution: yched commented"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.
Comment #9
gordon CreditAttribution: gordon commentedI 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.
Comment #10
yched CreditAttribution: yched commentedCan'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()
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.
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.
Comment #11
gordon CreditAttribution: gordon commentedI have made more changes as above. I have removed the _batch_do_shell as I felt it could not be made generic enough.
Comment #13
lilou CreditAttribution: lilou commentedHEAD is broken.
Comment #14
zzolo CreditAttribution: zzolo commentedReviewed:
* 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.
Comment #15
gordon CreditAttribution: gordon commentedI 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.
Comment #16
gordon CreditAttribution: gordon commentedComment #17
moshe weitzman CreditAttribution: moshe weitzman commentedThanks for sticking with this, gordon. This refactor is gonna be very useful for aegir, drush, field migration, data migration, and custom scripts everywhere.
Comment #18
hass CreditAttribution: hass commentedCommand line batches - *great* :-)
Comment #19
sunWatch out for PHPWTF.
The coding standard is "elseif".
This review is powered by Dreditor.
Comment #21
gordon CreditAttribution: gordon commentedthanks @sun I have made those couple of changes.
Comment #22
sunOn 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.
Comment #23
gordon CreditAttribution: gordon commentedI 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()
Comment #24
sunIn the elseif, 'redirect_callback' will always be !empty() now, so you can remove the first condition before the &&.
This review is powered by Dreditor.
Comment #25
gordon CreditAttribution: gordon commentedWhat 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.
Comment #26
sunyep, 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.
Comment #27
gordon CreditAttribution: gordon commentedYes I get you now. I was going to fix this, but you bet me to it.
Comment #28
sunTagging.
Comment #29
mattyoung CreditAttribution: mattyoung commentedsub
Comment #30
Dries CreditAttribution: Dries commentedTests? This seems like the kind of new feature that could really use some tests.
Comment #31
gordon CreditAttribution: gordon commentedActually 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.
Comment #32
sunI 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.
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.
Comment #33
gordon CreditAttribution: gordon commentedI have removed this since this is a private function and it should be set in batch_progress().
Make sure this has the drupal_goto set.
Take this information from $batch
I'm on crack. Are you, too?
Comment #35
gordon CreditAttribution: gordon commentedFixed name of incorrect function.
This review is powered by Dreditor.
Comment #36
sunAwesome! This really looks ready to fly now! :)
Comment #37
chx CreditAttribution: chx commentedErm. 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,
makes me run screaming. May I get
please?
Comment #38
gordon CreditAttribution: gordon commentedI have changed it as per your request. Much better readability.
Comment #39
gordon CreditAttribution: gordon commentedComment #40
chx CreditAttribution: chx commentedI 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..?
Comment #41
gordon CreditAttribution: gordon commentedI 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.
Comment #42
yched CreditAttribution: yched commentedDoesn'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 ?
Comment #43
gordon CreditAttribution: gordon commentedI have fixed up the $batch -> $function
Without the batch identifier you can still make use of it, but it is just harder.
Comment #44
chx CreditAttribution: chx commentedYeah 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.
Comment #45
sunWhy don't we define the default value in the function argument already?
The same seems to apply to the $url argument...
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?
Comment #46
gordon CreditAttribution: gordon commentedI have made the changes as above.
I think this is there now. I think it is starting to go round in circles a bit.
Comment #47
sun$url argument now needs to default to 'batch'.
Comments should always form proper sentences with proper capitalization.
I'm on crack. Are you, too?
Comment #48
gordon CreditAttribution: gordon commentedI have made the changes as above.
Comment #49
sunErm. Great, #47 or #48 it is then - both are almost identical ;)
Comment #51
gordon CreditAttribution: gordon commentedHere is a fixed version of the patch which now applies.
Comment #52
gordon CreditAttribution: gordon commentedComment #53
moshe weitzman CreditAttribution: moshe weitzman commentedSeems like we have consensus here. Code looks good.
Comment #54
Dries CreditAttribution: Dries commentedAlthough this introduces a new feature, I decided to go ahead and committed this change.
Comment #56
yched CreditAttribution: yched commentedFollowup fix: #617420: Batch system error on redirection