Posted by Damien Tournoud on October 12, 2008 at 10:47pm
4 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | simpletest.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
... because the batch API stores the whole calling form (ie. the whole test selection form) along with the batch parameters by default. Here is a patch that bypass this behavior.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| simpletest-max-packet-fail.patch | 657 bytes | Ignored: Check issue status. | None | None |
Comments
#1
Improved version, with more comments.
#2
pwolanin suggested a better wording.
#3
tested. Prevents the WSOD from adding more test classes. Tests still run as expected.
#4
As one of the people who experienced this problem I'm glad for this patch. Like pwolanain I tested and verified it works as expected.
@Damien Tournoud thanks for the fix.
#5
I confirm that the full suite ran with max_allowed_packet = 1M, flawlessly.
#6
I think we should make the code comments better still. We don't explain why we call batch_process().
#7
@Dries - did you look at the comments in #2?
#8
Yes, I don't think they are sufficiently explanatory. In fact, I think the comments in #1 were more specific. Let's take another pass at the comments! :)
#9
ok, here's another try.
#10
I'm still not sure I understand this patch, and the code comments still don't help me. Sorry. I'll have to start digging the code first so I fully understand this before I can review this properly. Until then, this is a bit fishy.
#12
One last try.
<?php+ // Normally, the forms portion of the batch API takes care of calling
+ // batch_process(), but in the process it saves the whole $form into the
+ // database (which is huge for the test selection form).
+ // By calling batch_process() directly, we skip that behavior and ensure
+ // that we don't exceeding the size of data that can be sent to the database
+ // (max_allowed_packet on MySQL).
?>
#13
@Dries - MAMP, xampp, and other setups come with mysql configured to max_allowed_packet = 1M. The simpletest form ($form) has reached a point where it is really really large. Normally, when batch_set() is called in a form submit handler (in this case simpletest_test_form_submit() through simpletest_run_tests()) drupal_process_form() makes sure batch_process() is called. But, before drupal_process_form() executes batch_process() it adds the entire form array to the batch that is stored in the database. In this case the form that is stored in the database causes the $batch to be larger then 1M so an exception is thrown and execution is stopped.
By adding batch_process() in simpletest_run_tests() we set the batch processing into motion without the full simpletest form being added the batch so the form is not stored in the database.
It is a special case for the simpletest form because it is so large.
Does this explanation help?
#14
Heine spotted a grammar error.
#15
We should avoid mysql specific references. This problem could come up in a different database engine. Can this be rephrased in more general terms.
#16
@mfer: please read the patch. It *is* phrased in general terms.
#17
+ // Normally, the forms portion of the batch API takes care of calling+ // batch_process(), but in the process it saves the whole $form into the
+ // database (which is huge for the test selection form).
+ // By calling batch_process() directly, we skip that behavior and ensure
+ // that we don't exceed the size of data that can be sent to the database
+ // (max_allowed_packet on MySQL).
The last line refers to mysql and a mysql setting. That's not general.
#18
@mfer, thanks for your review... but (1) MySQL is mentioned only in the last line, after a general explanation has been made; and (2) that issue only occurs on MySQL as far as we know.
#19
I did some checking. Of the major databases (mysql, postgresql, oracle, mssql) my understanding is that this will only affect mysql.
#20
Thanks for clarifying. That helps. I committed the patch to CVS HEAD. Thanks.
#21
Automatically closed -- issue fixed for two weeks with no activity.