Form cache delete on node save should remove all cache entries for that form

I think it would be a better solution to make a new parameter (or some special case for $sid) that makes it possible to delete all cache entries for a particular form, rather than taking an $sid argument that doesn't do anything at all.

Updated issue summary.

Dropping the $sid argument like in the patch would lead to a broken interface for simultaneous edits:

1. User A opens the form_builder interface.
2. User B opens the form_builder interface edits and saves.
3. User A now tries to edit however there is no more form_builder cache for this node so all edits fail.

I'd suggest another approach: Delete the form_builder cache whenever the form_builder interface is loaded. This would also solve other issues with stale form_builder caches.

Here is a patch that resets the form_builder cache whenever node/%/webform is loaded (or to be more precise: whenever form_builder_interface() is called).

To make things work I've also had to change two other things:

1. The form_builder_webform_save_form() needs to be built before calling form_builder_interface() -- so it's submit handler is called before the form_builder cache is reset.
2. I've changed form_builder_positions() to use drupal's AJAX framework. With ajaxSubmit() it has always triggered a complete rebuild of the page which lead to the cache being deleted (because form_builder_interface() was called). Now it only executes the submit-handler so it even has a (big) performance improvement when dragging fields

There was an issue with the previous patch: Triggering the ajax submission via .mousedown() had the side-effect of setting mousePressed=1. This patch uses a slightly more cautious approach of only calling triggerHandler('mousedown').

with the patch of comment #8 a new bug is introduced:

1. Create a new form in form_builder and add a fieldset
2. Add another field and place it inside the new fieldset
3. Save the form
4. When viewing the form the field is placed after the fieldset and not inside the fieldset
5. When loading the form into form_builder again the field is displayed below the fieldset
6. When now the field is draged into the fieldset and the form is saved the form is now correctly displayed with the field inside the fieldset

So it seems that already saved fields (with CID's ??) are treated differently than new fields.

I've tracked the problem down to the form_builder.js function

Drupal.formBuilder.updateElementPosition

When the last line

$('#form-builder-positions input[type="submit"]').triggerHandler('mousedown');

is called for an already existing field then in ajax.js the code

$(ajax.element).bind(element_settings.event, function (event) {
    return ajax.eventResponse(this, event);
});

is executed. This Ajax code is not called with newly created fieldsets/fields.

The reason for the problems in #9 was addElement replaces the position-form without calling Drupal.attachBehaviors. So event-handlers weren't registered anymore after that.

The newly attached patch fixes that too.

I'm not sure about this change @torotil. With Form Builder, it's common that users may assemble rather lengthy forms all in a single page load (similar to the amount of work done when assembling a View in Views module). Form Builder intentionally saves the users work and reloads it so that if they accidentally leave or reload the page, their work is not lost. I'd really prefer to keep the autosaving functionality rather than discarding any in-progress work when the page is loaded.

Yes that's a valid concern. Currently it's bought with inconsistencies and the danger of overriding changes made by other uses.

What about storing a hash of the unchanged form that the form_builder_cache entry is based on and then show a warning if it has changed. The user should then have 2 options:

1. override the changes made by someone elese (update the hash)
2. throw away his/her own changes.

For us users unintentionally overriding changes of others was a huge issue. The usual course of events would be:

1. User A "looks" at the form (thereby generating his/her form_builder_cache entry).
2. During the next months/weeks User B edits the form and saves it.
3. User A wants to make minor tweaks to the form (as displayed), opens up the form_builder interface and either finds that it has nothing in common with the form he just saw or unknowingly overrides changes made by user B (introducing regressions).

While users tend to blame themselves when they lose data due to leaving the site - they sure tend to blame the software / us if they don't know how they lost the data.

During the next months/weeks User B edits the form and saves it.

We should implement hook_cron() to clean up the entries in form_builder_cache. I'm rather surprised we don't clean up after an editing session has been abandoned.

I think we already have per-user caches, since the $sid column in the form_builder_cache table is tied to a user's session ID. The only way two users should be seeing each others work would be if they were using the same computer. If that's not the case already, we need to correct this to prevent user's sharing in-progress work.

I don't think we have any warning system for users editing the same form at the same time though, it'd be great to have the same options as Views for showing locked forms and allowing the user to "break" the lock and start a new editing session.

The only way two users should be seeing each others work would be if they were using the same computer.

… and that is the core of the problem described in #13. Users which don't save the form for a long time don't see changes made (and saved) by other users.

We should implement hook_cron() to clean up the entries in form_builder_cache. I'm rather surprised we don't clean up after an editing session has been abandoned.

Cleaning up the form_builder_cache during cron-runs is in general a good idea and should be easy enough to implement.

We could improve on that by sending an AJAX request whenever a user leaves/abandons the form_builder session. This would mean starting with a fresh session every time a user opens the form_bulder interface (which is exactly what my patches already implement).

Here is a patch that implements hook_cron to delete stale cache entries. By default it deletes them after 6h (which equals the hardcoded core cache_form limit).

I've now committed the migration to the core ajax functionality separately. Here is a patch for the remaining changes.

Update the path to the current 7.x-1.x version.

+++ b/modules/webform/form_builder_webform.module
@@ -35,8 +35,9 @@ function form_builder_webform_components_page($node) {
+  $form = drupal_get_form('form_builder_webform_save_form', $node->nid);
...
-  $build[] = drupal_get_form('form_builder_webform_save_form', $node->nid);
+  $build[] = $form;

What would this change?

What would this change?

Further up in the patch the call to form_builder_webform_save_form() is modified to reset the form-cache, while the call to form_builder_interface() (re)generates it. Therefore the order of the two calls is important.

EDIT: If it's that unclear it might be better to do clear the object-cache in another place, though.

Actually the patch might mitigate the issue a lot. So this might be an easy provisional solution.

joelpittet: You were right about your comment to the patch. What I remembered was about some previous version of it.

I've committed this to 7.x-1.x.