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.
Comment | File | Size | Author |
---|---|---|---|
#50 | 297972_drupal_execute_batch_api.patch | 1.02 KB | gdd |
#46 | 297972_drupal_execute.patch | 5.34 KB | gdd |
#43 | 297972_drupal_execute_7.patch | 5.17 KB | Steven Jones |
#33 | 297972_drupal_execute_6.patch | 4.04 KB | Steven Jones |
#31 | 297972_drupal_execute_5.patch | 3.24 KB | Steven Jones |
Comments
Comment #1
boombatower CreditAttribution: boombatower commentedThis looks like an issue for core to be back-ported once fixed.
Comment #2
wilson98 CreditAttribution: wilson98 commentedsorry, i'm using drupal 6.4 not drupal 7.x.
Comment #3
wilson98 CreditAttribution: wilson98 commentedsorry, i'm using drupal 6.4 not drupal 7.x.
Comment #4
boombatower CreditAttribution: boombatower commentedYes, 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.
Comment #5
scor CreditAttribution: scor commentedthis 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).
Comment #6
scor CreditAttribution: scor commentedthe patch allows programmed forms to be executed directly
Comment #7
yched CreditAttribution: yched commenteddrupal_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:
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.
Comment #8
scor CreditAttribution: scor commentedupdated 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...
Comment #9
Steven Jones CreditAttribution: Steven Jones commentedRan 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.
Comment #10
boombatower CreditAttribution: boombatower commentedCurrent workflow: it gets into D7 core, I backport it to D6.
Comment #11
0xAFFE CreditAttribution: 0xAFFE commentedThis 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 :-).
Comment #12
Steven Jones CreditAttribution: Steven Jones commentedFrom 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!
Comment #13
Steven Jones CreditAttribution: Steven Jones commentedPatch attached for Steps 1 and 2.
Comment #14
yched CreditAttribution: yched commentedIs 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 :-)
Comment #15
Steven Jones CreditAttribution: Steven Jones commentedWell, 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.
Comment #16
yched CreditAttribution: yched commentedI 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.
Comment #17
Steven Jones CreditAttribution: Steven Jones commentedOkay, here's a better test, we visit a page that sets a batch, which calls drupal_execute.
Comment #19
Steven Jones CreditAttribution: Steven Jones commentedTry again :-)
Comment #21
Steven Jones CreditAttribution: Steven Jones commentedUpdate for HEAD.
Comment #23
apadernoComment #26
catchComment #27
deviantintegral CreditAttribution: deviantintegral commentedThe 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.
Comment #31
Steven Jones CreditAttribution: Steven Jones commentedReroll for HEAD.
Comment #33
Steven Jones CreditAttribution: Steven Jones commentedOkay, the last patch included the tests, but not the patch for form.inc, whoops!
Comment #35
Steven Jones CreditAttribution: Steven Jones commentedTests pass on my local machine. Hmmm...
Comment #36
apadernoAnd #33 shows it passed the test, now.
Comment #38
yfreeman CreditAttribution: yfreeman commentedI came across this bug in mid July. I found a workaround without patching core.
pausing a batch process for form submission
Comment #39
Steven Jones CreditAttribution: Steven Jones commented@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!
Comment #40
yfreeman CreditAttribution: yfreeman commented@steve
I agree. Keep up the good work.
Comment #41
gddUnfortunately 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.
Comment #42
webchickGot bit by this today. Would be nice to have it fixed.
Please comment this so that the discussion in this issue is reflected and someone doesn't break it again.
Code-style on the test:
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?
Why 2? Can we make that something more obvious?
Remove :} and end with period.
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?
No PHPDoc on these. Please add a line to explain what they do.
Nothing in this function. Please remove it.
Why the submit button first and then the textfield? Can we please flip them so it's more natural?
Comment #43
Steven Jones CreditAttribution: Steven Jones commentedHopefully the attached patch will address those issues.
Comment #44
gddPatch 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
Comment #45
webchickWow, nice clean-up! A couple more very minor nits and then this is ready to go:
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.
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!
Comment #46
gddOK, 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.
Comment #47
gddComment #48
Chris Johnson CreditAttribution: Chris Johnson commentedNote that it is impossible to create CCK content types from update_N() hooks because of this bug. :-(
Comment #49
webchickGreat 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. :)
Comment #50
gddHere it is
Comment #51
yched CreditAttribution: yched commentedCode is correct and early comments in the thread report that it fixes the bug in D6
Comment #52
gddYes it works well, I've actually been running it for about a week now on D6.
Comment #53
Gábor HojtsyThanks, committed to Drupal 6.