form_builder_set_element() may overwrite existing fields on key changes

I've noticed Form Builder getting confused about form cache early in the development. I haven't been able to reproduce it reliably though. I think it has to do with the creation and deletion of nested Fieldsets, but again, I haven't found the exact cause yet, maybe your problem is something different.

It shouldn't even be necessary to save, the cache itself is getting messed up. So we should be able to create the problem simply by dropping in a couple of fieldsets, then reloading the page.

I see this issue, and the issue with the cache itself getting messed up when I am dealing with a very large form. The form_builder_cache table is using a blob for the data column, which has a limit of 64k (65,535 bytes). My form, when $form is serialized, is larger than 64k, and so when the table is updated, the serialized string is truncated.

Updating the table to use a mediumblob (16,777,215 bytes) eliminates that problem for me.

As I haven't had any specific reports of this (other than the large cache size, there's a separate issue for that) and there isn't anything actionable in this issue, I'm closing it. As Acquia has been using Form Builder for months now, I don't think this problem still exists. Please open a new issue after I've made the new 1.0 beta releases.

I'm pretty sure this is our problem:

      elseif (isset($element['#key']) && $key != $element['#key']) {
        $new_key = $element['#key'];
        $index = array_search($key, $form);
        $before = array_slice($form, 0, $index + 1, TRUE);
        $after = array_slice($form, $index + 2, NULL, TRUE);
        $form = $before + array($new_key => $element) + $after;
        unset($form[$key]);
      }

I *FOUND* the problem! You were absolutely right! I'm guessing Acquia hasn't run into the problem because they don't allow you to change the form element keys in Gardens. You were spot-on with the problem, which originated in the call to array_search(). What I had intended array_search() to do was to return an integer value, indicating where in the array we should base our offset to do the splicing. However the problem was that we were calling array_search() on an associative array, so it was returning a string offset instead of a numeric one.

For example, let's take this array:

$form = array(
  '#tree' = TRUE,
  'field1' => array(...),
  'field2' => array(...),
);

We would call array_search('field1', $form). I had expected that it would return the integer "1", since field1 is in the 1 position ('#tree' is in 0). But instead, this call would return the string '#tree'! When you added the string '#tree' + 2, you would get the integer '2' (oh... PHP...), essentially the offset was entirely wrong (or in fact, always '2'), causing one field or entire fieldset to get eaten every time a field key was changed.

To fix all these problems, I've changed it to the following code:

      elseif (isset($element['#key']) && $key != $element['#key']) {
        $new_key = $element['#key'];
        $index = array_search($key, array_keys($form));
        $before = array_slice($form, 0, $index, TRUE);
        $after = array_slice($form, $index + 1, NULL, TRUE);
        $form = $before + array($new_key => $element) + $after;
        unset($form[$key]);
      }

The real key being calling array_keys() inside of array_search(), to ensure that we get a numeric offset back.

This became apparent in my testing for #1404806: Use #type = 'machine_name' for key fields, in case you're curious why it finally came back up again. I feel so much better finding a blatant programming error, I'd always been suspicious that this problem had gone unfixed. I've committed this fix to both 6.x and 7.x branches.

Updating the title, since this was a problem with form_builder_set_element() rather than form_builder_cache_load().