Reference to $form_state['values'] is altering Views Bulk Operations modify action form submit

Nope, we need to be able to alter the submitted values in the submit callback, see title_field_term_description_submit(). We need a different solution.

I've just spent several hours trying to debug exactly this bug, and after many backtraces I worked my way to implementing exactly the same 'solution' as in comment 1 here. Nice to know that that 'solution' isn't correct, but I'd be very interested in what will work.

For anyone else like me trying to google this issue, the original VBO error I was getting was:

Recoverable fatal error: Argument 1 passed to drupal_array_get_nested_value() must be an array, null given, called in modules/field/field.default.inc on line 38 and defined in drupal_array_get_nested_value() (line 6490 of includes/common.inc).

..which means that the callback title_field_attach_submit() is deleting array items in the $form_state['values'] which, since this is passed by reference, is affecting everything else afterwards that needs to also operate on this array.

Please see attached patch.

I've removed the variable assignment by reference, or else the call to drupal_array_get_nested_value() was stripping most of the values in $form_state['values']. However, to ensure that changes to the $values variable are incorporated into the $form_state array, I've also included a call to drupal_array_set_nested_value() at the end.

This should ensure the functionality of assigning by reference is retained, without its destructiveness though as I'm not entirely sure the purpose of this callback function this definitely needs review.

Patch path reformatted for testing.

Ugh... this should be compatible with SimpleTest.

Please see attached patch with corrected variable name for $values.

I'd really appreciate feedback on this patch, as we'd like to push this across to production soon but I need to know whether this ensures existing functionality is retained.

I'd appreciate a comment explaining why #11 actually solves the issue and a brief update to the issue summary with the same information. Aside from this the change does not look harmful to me. The proper way to test it should be changing the input format of a replaced term description and check that everything keeps working. Ideally a test addition would be the perfect way to show this patch does work.

Please see my comment in #5 which explains why this patch fixes the issue.

To restate though, the code was performing the variable assignment by reference of $values = &$form_state['values'] and then was immediately making a call to drupal_array_get_nested_value($values, $form['#parents']) that accepts $values by reference and makes changes directly to that $values array, whereby it strips most items and leaves just those that are children of $form['#parents']. Therefore, by the time the function title_field_attach_submit() exits, it has potentially deleted elements from the $form_state['values'] array.

In most instances, it just so happens that drupal_array_get_nested_values() returns all the original values unaltered and therefore this code was, by luck, not affecting subsequent functions that also needed to pull values from $form_state['values']. But when VBO is involved, the $form_state['values'] array is very much larger, including data from multiple nodes and the call to drupal_array_get_nested_values() is indeed destructive.

So, by making the assignment $values = $form_state['values'] by value, and then by reinjecting the (potentially) changed values back into this array, we avoid destructively altering other values that are not the concern of title module.

Unfortunately, I am not in a position to write a test for this function (and I don't understand "the proper way to test it should be changing the input format of a replaced term description" — can you elaborate?).

Thanks for clarifying but I asked also a comment in code with this explanation.

I don't understand "the proper way to test it should be changing the input format of a replaced term description" — can you elaborate?

Sorry, I got confused with the submit callback (see http://drupalcode.org/project/title.git/blob/628406b0bf0f67eb8a62979eed0...).

I guess we just need to confirm that things don't break for field widgets belonging to field groups (and obviously for widgets in plain forms): submitted values are synchronized back into the legacy properties and taken from there at storage time (they are also stored as fields). We must check that the legacy storage and the field storage have consistent values.

I've done no dedicated testing for this, but we've had this patch running on a production system without issue for over a month now. I'd very much appreciate this to be commit, or else we are going to have to maintain this patch against future releases.

Thanks.

Is it that hard to provide a new patch with a comment explaining why we need this fix?

Please see attach patch with comments.

Committed and pushed the attached patch, thanks!

(I made some minor comment cleanup before comitting it)

This still needs tests