scenario: i want my site to create a profile node automatically for users when they're registering, so i implement the function hook_user, here's the sample code,

function q_user($op, &$edit, &$account, $category = NULL) {
switch ($op) {
case 'insert':
//create a profile node for user;
global $profile_uid;

$form_state = array();
$node = array('type' => 'profile');
$form_state['values']['name'] = $account->name;
//pass the uid to nodeapi when presave;
$profile_uid = $account->uid;
$form_state['values']['op'] = t('Save');
drupal_execute('profile_node_form', $form_state, (object)$node);
break;
}
}

it works fine but i can't test it using simpletest. here's the test code.

function setUp() {
parent::setUp('content_profile', 'mymodule');
}

function testAutoCreateProfile() {
$account = $this->drupalCreateUser();
$node = node_load(array('uid'=>$account->uid));
$this->assertEqual($account->name, $node->name, "Auto create profile node when creating new user.");
}

i tracked the source, here's the code execute steps.
1.drupal_execute()
2.drupal_process_form()
3.form_execute_handlers('submit', $form, $form_state)

in the function 3,

foreach ($handlers as $function) {
if (function_exists($function)) {
if ($type == 'submit' && ($batch =& batch_get())) {
// Some previous _submit handler has set a batch. We store the call
// in a special 'control' batch set, for execution at the correct
// time during the batch processing workflow.
$batch['sets'][] = array('form_submit' => $function);
}
else {
$function($form, $form_state);
}
$return = TRUE;
}
}

because simpletest sets batch, so it can't excute the form submit handler. and it also doesn't get the test fail, but just loop for test.

anyone can help? thanks.

Files: 
CommentFileSizeAuthor
#50 297972_drupal_execute_batch_api.patch1.02 KBheyrocker
#46 297972_drupal_execute.patch5.34 KBheyrocker
Passed: 10645 passes, 0 fails, 0 exceptions View
#43 297972_drupal_execute_7.patch5.17 KBSteven Jones
Passed: 10645 passes, 0 fails, 0 exceptions View
#33 297972_drupal_execute_6.patch4.04 KBSteven Jones
Failed: Failed to apply patch. View
#31 297972_drupal_execute_5.patch3.24 KBSteven Jones
Failed: 9663 passes, 1 fail, 7 exceptions View
#21 297972_drupal_execute_4.patch4.63 KBSteven Jones
Failed: Failed to apply patch. View
#19 297972_drupal_execute.patch4.39 KBSteven Jones
Failed: Failed to apply patch. View
#17 297972_drupal_execute.patch4.06 KBSteven Jones
Failed: Failed to apply patch. View
#13 297972_drupal_execute.patch2.92 KBSteven Jones
Failed: Failed to apply patch. View
#8 297972drupal_execute_2.patch823 bytesscor
Failed: Failed to apply patch. View
#6 297972drupal_execute.patch815 bytesscor
Failed: Failed to apply patch. View

Comments

boombatower’s picture

Project: SimpleTest » Drupal core
Version: 6.x-2.2 » 7.x-dev
Component: Code » simpletest.module

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

wilson98’s picture

Version: 7.x-dev » 6.4

sorry, i'm using drupal 6.4 not drupal 7.x.

wilson98’s picture

sorry, i'm using drupal 6.4 not drupal 7.x.

boombatower’s picture

Version: 6.4 » 7.x-dev

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.

scor’s picture

Title: It seems simpletest can't test programmed form » drupal_execute incompatible with the batch API
Version: 7.x-dev » 6.x-dev
Component: simpletest.module » forms system

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).

scor’s picture

FileSize
815 bytes
Failed: Failed to apply patch. View

the patch allows programmed forms to be executed directly

yched’s picture

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.

scor’s picture

FileSize
823 bytes
Failed: Failed to apply patch. View

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...

Steven Jones’s picture

Status: Active » Needs review

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.

boombatower’s picture

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

0xAFFE’s picture

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 :-).

Steven Jones’s picture

Version: 6.x-dev » 7.x-dev
Status: Needs review » Needs work

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!

Steven Jones’s picture

Status: Needs work » Needs review
FileSize
2.92 KB
Failed: Failed to apply patch. View

Patch attached for Steps 1 and 2.

yched’s picture

+  /**
+   * 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 :-)

Steven Jones’s picture

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.

yched’s picture

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.

Steven Jones’s picture

FileSize
4.06 KB
Failed: Failed to apply patch. View

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

Status: Needs review » Needs work

The last submitted patch failed testing.

Steven Jones’s picture

Status: Needs work » Needs review
FileSize
4.39 KB
Failed: Failed to apply patch. View

Try again :-)

Status: Needs review » Needs work

The last submitted patch failed testing.

Steven Jones’s picture

Status: Needs work » Needs review
FileSize
4.63 KB
Failed: Failed to apply patch. View

Update for HEAD.

kiamlaluno’s picture

Title: drupal_execute incompatible with the batch API » drupal_execute is incompatible with batch API
Status: Needs review » Needs work
catch’s picture

Status: Needs work » Needs review
deviantintegral’s picture

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

Steven Jones’s picture

Status: Needs work » Needs review
FileSize
3.24 KB
Failed: 9663 passes, 1 fail, 7 exceptions View

Reroll for HEAD.

Status: Needs review » Needs work

The last submitted patch failed testing.

Steven Jones’s picture

Status: Needs work » Needs review
FileSize
4.04 KB
Failed: Failed to apply patch. View

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

Status: Needs review » Needs work

The last submitted patch failed testing.

Steven Jones’s picture

Status: Needs work » Needs review

Tests pass on my local machine. Hmmm...

kiamlaluno’s picture

And #33 shows it passed the test, now.

Status: Needs review » Needs work

The last submitted patch failed testing.

yfreeman’s picture

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

Steven Jones’s picture

Status: Needs work » Needs review

@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!

yfreeman’s picture

@steve

I agree. Keep up the good work.

heyrocker’s picture

Status: Needs review » Needs 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.

webchick’s picture

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?

Steven Jones’s picture

Status: Needs work » Needs review
FileSize
5.17 KB
Passed: 10645 passes, 0 fails, 0 exceptions View

Hopefully the attached patch will address those issues.

heyrocker’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

Status: Reviewed & tested by the community » Needs work

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!

heyrocker’s picture

FileSize
5.34 KB
Passed: 10645 passes, 0 fails, 0 exceptions View

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.

heyrocker’s picture

Status: Needs work » Needs review
Chris Johnson’s picture

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

webchick’s picture

Version: 7.x-dev » 6.x-dev
Status: Needs review » Patch (to be ported)

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. :)

heyrocker’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.02 KB

Here it is

yched’s picture

Status: Needs review » Reviewed & tested by the community

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

heyrocker’s picture

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

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, committed to Drupal 6.

Status: Fixed » Closed (fixed)

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