drupal_execute is incompatible with batch API

This looks like an issue for core to be back-ported once fixed.

Yes, but SimpleTest 6.x-2.x is a back-port of core 7.x SimpleTest. So if this is a problem it need to be solved in core and back-ported.

this is caused by the batch API which provides an empty $form_state to the form submit function. I run into the same problem when using drupal_execute in hook_enable: it works when I enable the module in the modules page, but breaks when I include the module in a install profile and run the installer (which uses the batch API).

the patch allows programmed forms to be executed directly

drupal_process_form() uses a slightly different logic to determine if the form should initiate a batch process, or if it is being drupal_execute()'ed inside an already running batch:

  // If batches were set in the submit handlers, we process them now,
  // possibly ending execution. We make sure we do not react to the batch
  // that is already being processed (if a batch operation performs a
  // drupal_execute).
  if ($batch =& batch_get() && !isset($batch['current_set'])) {
    // ...
    batch_process();

It seems this precaution was not enough since form_execute_handlers() has to consider this case too, but I'd suggest using the same test as above, to stay consistent.

updated the patch with yched's remarks. it works but I don't know if it covers all the edge cases of drupal_execute/batch API...

Ran into this bug, and this patch worked nicely.

What's best to do, fix and test it on Drupal 7 using testing etc. then backport? Or fix it in D6, then D7.

Current workflow: it gets into D7 core, I backport it to D6.

This patch works nice on Drupal 6.8.

I ran into this issue while adding a field to a cck-content-type via the 'content_copy_import' form using drupal_execute.

Saved my evening :-).

From looking at form.inc, this bug is still in D7.
Step 1. Make a test that runs a drupal_execute within a batch.
Step 2. Patch core.
Step 3. Backport!

Patch attached for Steps 1 and 2.

+  /**
+   * Check that we can run drupal_execute during a batch.
+   */
+  function testRequiredFields() {
(...)
+    // We get run in a batch, so can just call drupal execute:
+    drupal_execute('form_test_mock_form', $state);
(...)
+}

Is that enough ? When launched through the UI, tests are indeed run in a batch, but
1) that's only incidental and we could imagine it might change at some point
2) I'm not sure it's the case when tests are launched from the command-line, or by the test bot
in which cases the test only tests drupal_execute :-)

Well, okay, is this even testable then? I thought that when you set a batch from within a batch, the first would complete and then the second would run. If this is the case then we really can't test this?

Can we visit a page in our test, that has a batched drupal_execute? Is simpletest capable of doing that?

This is my first D7 test related patch btw.

I thought that when you set a batch from within a batch, the first would complete and then the second would run
You got that right, except there's a cool subtlety here : both batches do not run on the same instance of drupal :-)
The enclosing 'simpletest' batch runs on your usual instance, and the batch you execute in a test function runs on the 'fake' drupal instance created by the test engine.
So, yes, this sould be testable. In fact, #316344: Add meta refresh support to SimpleTest got committed precisely so that the simpletest internal browser could handle the batch progress page.

Okay, here's a better test, we visit a page that sets a batch, which calls drupal_execute.

Try again :-)

Update for HEAD.

The patch in #21 works for me and allows my test over in #360377: book_get_books() cache becomes stale when batch-inserting book pages to function properly. +1 for RTBC.

Reroll for HEAD.

Okay, the last patch included the tests, but not the patch for form.inc, whoops!

Tests pass on my local machine. Hmmm...

And #33 shows it passed the test, now.

I came across this bug in mid July. I found a workaround without patching core.
pausing a batch process for form submission

@kaiserjozy Good work, but it is still a workaround, the bug in core needs to be fixed to be able to use install profiles properly.

The patch does work, if you look at the the pifr logs it passes on slave #5, but not on #7, argh!

@steve

I agree. Keep up the good work.

Unfortunately it appears to need a reroll. I just tried to apply it and got Hunk #1 failed in form.test. Everything else applies with varying degrees of offset. I've had zero exposure to the simpletest stuff, otherwise I'd reroll myself, it looks like it's probably pretty straightforward. I'm happy to retest after a reroll.

Got bit by this today. Would be nice to have it fixed.

+      if ($type == 'submit' && ($batch =& batch_get()) && !isset($batch['current_set'])) {

Please comment this so that the discussion in this issue is reflected and someone doesn't break it again.

Code-style on the test:

+    variable_set('form_test_mock_submit', 0); // This variable gets updated in the call below

Comment above the variable, ending with a period. But while we're making a comment, how about explain what this variable is used for and why we set it to 0?

+    $this->assertEqual(2, variable_get('form_test_mock_submit', 0), t('Check drupal_execute called submit handlers when running in a batch'));

Why 2? Can we make that something more obvious?

+  // Set up the batch:}

Remove :} and end with period.

+  //$bacth['title'] = t('drupal_execute Test');
...
+  //$batch['finished'] = 'form_test_batch_finished';

Remove debugging code.

And just in general, could we please better comment form_test_drupal_execute_batch_api() so it's clear what is going on?

+function form_test_drupal_execute_batch_api($arg = '') {
+function form_test_batch_callback($value) {
+function form_test_mock_form($form_state) {
+function form_test_mock_form_submit($form, &$form_state) {

No PHPDoc on these. Please add a line to explain what they do.

+function form_test_mock_form_validate($form, &$form_state) {

Nothing in this function. Please remove it.

+  $form['submit'] = array(
+    '#type' => 'submit',
+    '#value' => t('Submit'),
+  );
+
+  $form['test_value'] = array(
+    '#type' => 'textfield',
+    '#default_value' => 0,
+  );
+

Why the submit button first and then the textfield? Can we please flip them so it's more natural?

Hopefully the attached patch will address those issues.

Patch applies cleanly, passes all tests (albeit with Locale turned off right now due to #353883: Convert locale.inc to dbtng.). Code cleanup per webchick looks good, use of new strings vs mystery numbers makes code way more understandable. Does what it is supposed to. +1

Wow, nice clean-up! A couple more very minor nits and then this is ready to go:

+      // We test here to see if the form being submitted is being done so as
+      // part of a batch, and thus we need to handle the submit handler differently.
+      if ($type == 'submit' && ($batch =& batch_get()) && !isset($batch['current_set'])) {

Right, I can see that from the line below. :) What I want to know is 'why' this special check is required. A summary of the discussion on this issue would be good here, so that the next person who comes across it realizes why all three of those need to be checked for.

In the PHPDoc above +function form_test_drupal_execute_batch_api($arg = '') { could you mention something about the order of steps should be "initial_state", then "form_submitted", then "done"? Great work naming these so they're much more clear.

+  /**
+   * Implementation of setUp().
+   *
+   * Our form_test module provides a page and form that we use in the test.
+   */

We actually don't need to PHPDoc the setUp function because it's inherited. Funny, but true. So you can remove these lines and we're good.

Thanks so much!

OK, I gave this a shot. I used the comment from drupal_process_form() as a base to explain what is going on in form_execute_handlers(), which I think is good. Also integrated the rest of your notes.

Note that it is impossible to create CCK content types from update_N() hooks because of this bug. :-(

Great work, folks! Committed to HEAD!

Needs someone to create a patch that's #46 without the test stuff, and some other folks to test that patch and ensure it fixes the problem. If so, please mark it "reviewed & tested by the community" and we will hopefully see this go into the next version of Drupal 6. :)

Here it is

Code is correct and early comments in the thread report that it fixes the bug in D6

Yes it works well, I've actually been running it for about a week now on D6.

Thanks, committed to Drupal 6.