Jump to:
| Project: | Webform |
| Version: | 5.x-2.1.3 |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
I set up webform for a client to create surveys, and she seemed to take very well to the interface. She really liked the system, in fact, until we started getting data in. Invariably, the first few fields were blank, even though they were required. Sure enough, if I tried to create a new submission with the stored data, it wouldn't let me until values were submitted for the missing fields. Even more curious, the emails that were being sent included the missing data, but if I edited the submissions to put the missing data back in, it would go blank again.
Looking into it more, I realized that the problem goes back to the fact that my client ended her survey with a series of textareas for comments. Since they all resided within a fieldgroup that had an explanation, she just put a number as the label each one. At that point, webform defaulted to using the same number as the form key.
The real problem starts when webform prepares to store the data, in the function called _webform_client_form_submit_flatten:
function _webform_client_form_submit_flatten($node, $fieldset, &$form, $parent = 0) {
if (is_array($fieldset)) {
foreach ($fieldset as $form_key => $value) {
$cid = webform_get_cid($node, $form_key, $parent);
if (is_array($value) && $node->webform['components'][$cid]['type'] == 'fieldset') {
_webform_client_form_submit_flatten($node, $value, $form, $cid);
unset($form[$form_key]);
unset($form[$cid]);
}
else {
// The order here is significant!
unset($form[$form_key]);
$form[$cid] = $value;
}
}
}
}Because the function is adding items in based on component id and deleting items based form key, any time you have a component with a numerical form key you have the potential for collision between the form key of one component and the component id of another. In this particular case, as _webform_client_form_submit_flatten did its work it was wiping out (unsetting) data stored stored from the first few fields when it got to the textareas with numeric form keys at the end.
In the short term, the obvious solution is just not use numeric form keys. In fact, I changed the form keys and everything is working normally. I do think, though, that webform should be better equipped to handle user input. I would suggest that at a minimum, it default to adding some kind of a better identifier if the user only uses a number for the component label.
A better solution would be for this function to not delete from and write to the same array on the fly. I think it would be a better approach to write to a new array while deleting from the old, and then after all the old ones have been deleted, go back and add all the new ones back in, either through an array merge or just stepping through them and putting them in.
Comments
#1
Wow, thanks surge_martin for the excellent synopsis! I agree, creating a different result set is a better idea. I'm currently on a plane, which makes creating patches difficult, but here's my suggested version of the function:
<?php
function _webform_client_form_submit_flatten($node, $fieldset, $parent = 0) {
$values = array();
if (is_array($fieldset)) {
foreach ($fieldset as $form_key => $value) {
$cid = webform_get_cid($node, $form_key, $parent);
if (is_array($value) && $node->webform['components'][$cid]['type'] == 'fieldset') {
$values += _webform_client_form_submit_flatten($node, $value, $cid);
}
else {
$values[$cid] = $value;
}
}
}
return $values;
}
?>
#2
I think that's the right direction, but that code would change how the function operates, returning a flattened array of values instead of altering the original form.
Here's another approach:
<?php
function _webform_client_form_submit_flatten($node, $fieldset, &$form, $parent = 0) {
if (is_array($fieldset)) {
$new_vals = array();
foreach ($fieldset as $form_key => $value) {
$cid = webform_get_cid($node, $form_key, $parent);
if (is_array($value) && $node->webform['components'][$cid]['type'] == 'fieldset') {
_webform_client_form_submit_flatten($node, $value, $form, $cid);
unset($form[$form_key]);
unset($form[$cid]);
}
else {
// The order here is no longer significant!
unset($form[$form_key]);
$new_vals[$cid] = $value;
}
}
foreach ($new_vals as $cid => $value) {
$form[$cid] = $value;
}
}
}
?>
The only thing I'm not sure about is if an element in a fieldset with a numeric form key could still overwrite a top level form value. I'm having trouble figuring out a foolproof way to prevent that using a recursive function. You'd almost need to make a secondary function that wrote all the flattened values to a separate array and deleted the originals, and then have this as the parent function that would finish things off by writing the flattened values back to the original array.
I guess alternatively, a more dramatic change would be to just create an entirely new array for the flattened values (i.e. $form['flattened']), but changing over the rest of the code for this module to use the new array would definitely be non-trivial.
#3
I didn't have any problem with changing the functionality of the form, and I already committed my #1 change a while back. Sorry to not take in your suggestion in #2. :(
The change in functionality shouldn't make any difference to end-users because it's a private function. If they were using it before, well, this'll teach them to use private functions.
#4
Automatically closed -- issue fixed for two weeks with no activity.