The current behavior is that if a form fails validation when Submit (or next page) is clicked, the form is not saved. If the user doesn't notice that the form wasn't actually submitted and leaves the page, their work is lost.

Proposed behavior. If Drafts are enabled, then when a form fails validation and has not yet been submitted, it is saved as a draft. I believe this behavior is entirely beneficial, so I would proposed that no option be added to control this behavior.

Comments are welcome, especially from quicksketch.

Comments

danchadwick’s picture

In implementing this, I now think that there should be an option controlling this behavior, much as there is one for saving drafts between pages of a multi-page form. The main reason is that it can cause data which fails validation (including other than missing required components). Absent this issue, any data that has been saved has passed validation for the page it is one. Because drafts can (and usually do) have validation errors, I decided that an extra option for this is not necessary. If auto_save (save between pages of multi-page form) is enabled, then saving validation errors is enabled.

I'm unsure whether there should be a message indicating that a draft has been created (or updated). Absent a good reason, I plan to do whatever the inter-page auto-save does (which I think is nothing, but I haven't checked). Like auto_save for multi-page forms, there is no message to the user.

This does introduce a small API change in that there are now three validation handlers for the form. Prevalidation checks to see if the form can be submitted at all (limits, still open, etc). Then regular component validation. Then the post validation handlers saves any form that passes prevalidation but fails component validation as a draft, updating the form with the new draft submission, if needed. If another module want to add a validation handler, it should be inserted after the prevalidation handler and before the post validation handler. This should be documented.

danchadwick’s picture

Version: 7.x-4.x-dev » 8.x-4.x-dev
Status: Active » Fixed
StatusFileSize
new3.98 KB

Implemented as described. The auto save option was reworded to include saving submissions that fail validation. Committed to 7.x-4.x and 8.x

  • Commit 6b4ce38 on 7.x-4.x by DanChadwick:
    Issue #2284595 by DanChadwick: Added auto-saving forms with submission...
  • Commit 44df6d1 on 8.x-4.x by DanChadwick:
    Issue #2284595 by DanChadwick: Added auto-saving forms with submission...
danchadwick’s picture

Status: Fixed » Active
StatusFileSize
new2.25 KB

Alas, I found that the implementation didn't work when forms were cached. The following patch is in addition to #2.

It also prevents validation errors from being saved as a draft during ajax operation (such as auto-complete).

danchadwick’s picture

Status: Active » Fixed

Committed #4 to 7.x-4.x and 8.x.

  • Commit d12f232 on 7.x-4.x by DanChadwick:
    Issue #2284595 by Dan Chadwick: Fixes saving validation errors as draft...
  • Commit faed4d2 on 8.x-4.x by DanChadwick:
    Issue #2284595 by Dan Chadwick: Fixes saving validation errors as draft...
quicksketch’s picture

Status: Fixed » Needs work

Thanks Dan! Sorry I didn't get to reviewing this and I think you were right to push forward with it anyway.

This idea is a good one but the final implementation sure seems like it's awfully fragile. Webform already does some pretty deep FormAPI tricks, but manually setting the form cache and injecting things into $_POST is stretching the boundaries of safe implementation. Executing the submit handlers with a NULL button name also seems like it could have unexpected side-effects for contrib modules and possibly Webform itself in updates to our own submit handlers.

I'm going to mark this as "needs work" but we can leave the current implementation in the project. If possible, I'd like to see if we could create a cleaner implementation (possibly just calling webform_submission_insert/update directly instead of passing to the submit handlers), but more than anything else this should get additional tests to ensure the validation/draft-saving functionality works together.

danchadwick’s picture

Version: 8.x-4.x-dev » 7.x-4.x-dev

Yes, the implementation is fragile. It depends upon the Drupal 7 form API not changing. With the impending release of Drupal 8 and the difficulty that I've had in getting anything into Drupal 7 core, I very much doubt that the essential form handling of Drupal 7 will change, save for a security issue.

The root of the problem is that validation handlers really can't change the form. It looks like you can, because it is passed by reference, but the changes are only kept if the form passes validation is passed in memory to the submission handlers. By definition our situation is that the form has failed validation, but we want to change it and present the changed form to the user for correction, with a draft saved.

I can think of only two possible ways to handle this:

1) prevent the errors, saving them in the form_state, then allow the form to submit, but redirect to the same page and do the form API's job of restoring the errors (messages and error styling of the element)
or
2) allow the form to submit and modifying the form and form_state so that when the form is presented with errors, it is as if it had been saved as a draft.

(The third way would be to get the Drupal 7 FAPI fixed, but that's not happening.)

Neither of these approaches is clean and both are somewhat fragile. I felt that allowing the form to actually submit would possibly have more unintended consequences. This is why I submit the form on a copy of the form and form_state, so that anything done to them will not affect the actual form.

The reason I am calling the actual submit handlers is two-fold:
1) Other modules may have added additional submit handlers via a hook_form_alter. If these are needed for consistency, then they should be called too.
2) There is significant work being done to save the draft, including flattening the component tree, creating submission time tracking cookies, and so forth.

It seems safer to call the actual submit handler.

And last, even if we directly called the needed webform functions instead of the submit handler, the form, form_state, and FAPI need to be manipulated to make it as if the form had always had the draft (rather than just got one).

I need to pass a button name to the submit handlers that won't trigger undesirable consequences. As the code is written, an empty string name ('') is passed. This shouldn't cause any PHP errors in webform or other webform-related modules (as NULL might). That said, if you prefer, I could pass a mnemonic name that won't collide with any other button that might be added by someone else, such as '__Auto_Save__'.

I happy to try any other implementations that you think might be better, but I can't think of any. Or if you desire, we can reverse the patches and remove the feature.

danchadwick’s picture

Status: Needs work » Needs review

The last submitted patch, 2: webform-auto_save_validation_errors-2284595.2.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 4: webform-auto_save_validation_errors-2284595-4.patch, failed testing.

danchadwick’s picture

Status: Needs work » Fixed
StatusFileSize
new3.39 KB

I found one bug and made a number of improvements with this patch:

1) Once the form is submitted (i.e. exists in the database and is not a draft), validation errors should never be saved. The issue was that if you edited a completed submission and introduced a validation error, the submission would incorrectly revert to a draft. This was fixed by checking:

($form['#is_draft'] || !$form_state['values']['details']['sid'])

2) It turns out that $form['#is_draft'] was not always correctly initialized. I fixed that in the form definition function, webform_client_form, and I update it after the draft is auto-saved.

3) I set the sid of the newly-created autosave draft in $form_state['values']['details']['sid']. I don't believe this is used anywhere, but increasing the fidelity of the auto-save seems wise.

4) Following quicksketch's suggestion, I change the name of the faux button used when calling the submit handlers from '' to '__AUTOSAVE__'. It seems like it might have fewer side effects.

I'm going to again mark this as fixed, but I let's see if others turn up any issues. If this implementation turns out to be too fragile, I can reverse the patches (and maybe try the other implementation).

  • DanChadwick committed 2a1b7e4 on 7.x-4.x
    Issue #2284595 by DanChadwick: Fixed auto-save validation errors when...
  • DanChadwick committed 7c6515b on 8.x-4.x
    Issue #2284595 by DanChadwick: Fixed auto-save validation errors when...
quicksketch’s picture

The root of the problem is that validation handlers really can't change the form. It looks like you can, because it is passed by reference, but the changes are only kept if the form passes validation is passed in memory to the submission handlers.

I think you can actually modify the $form if you set $form_state['rebuild'] = TRUE, even from the validation handler. Then the form will be entirely rebuilt based on the values in $form_state. Pretty sure that's the case but I haven't confirmed.

danchadwick’s picture

I think you can actually modify the $form if you set $form_state['rebuild'] = TRUE, even from the validation handler. Then the form will be entirely rebuilt based on the values in $form_state. Pretty sure that's the case but I haven't confirmed.

Yeah, that would be nice, but no dice when there are errors. Here's the end of drupal_process_form:

    if (($form_state['rebuild'] || !$form_state['executed']) && !form_get_errors()) {
      // Form building functions (e.g., _form_builder_handle_input_element())
      // may use $form_state['rebuild'] to determine if they are running in the
      // context of a rebuild, so ensure it is set.
      $form_state['rebuild'] = TRUE;
      $form = drupal_rebuild_form($form_id, $form_state, $form);
    }
danchadwick’s picture

Status: Fixed » Needs review
StatusFileSize
new1.79 KB

Argh. Well, saving the form to the cache cause problems with compound form elements, like dates, that are expanded with a #process function. The un-processed form must be cached, and that is no longer available. However, it appears that setting the submission in the $form_state['build_info']['arg'][1], which is passed to the form definition function, plus setting $form_state['no_cache'] to prevent caching works. I need to do more testing, but here's a patch.

danchadwick’s picture

Status: Needs review » Fixed
StatusFileSize
new4.87 KB

Solution 16 was flawed. Webform's single draft code obscured the flaws.

I went back to the beginning and studied in depth the flow through the Form API. This revealed what would and what wouldn't work.

I have tested the attached patch with multiple drafts (as implemented by an external modules), as well as with multi-page forms, editing existing drafts, and forced form caching and without.

I also went through the exercise of reverting the other 3 committed patches and examining a single diff to make sure that no code crumbs were left behind from the failed attempts.

This solution relies on calling the submit handler (as before) and adjusting the sid in the form and form_state to the new sid. It then prevents the form from being in the cache so that a new form will be built with the sid. As part of this, it ensures that a submission which is being edited is always used as the draft in preference to any draft sid's in the database.

/**
 * THEORY OF OPERATION:
 * The Drupal 7 Form API lacks an easy way to rebuild the form in the event of
 * validation errors. The opertions is thus:
 * 
 * 1) The form is first displayed. If it is an existing draft, 
 *    webform_client_form will generated a form to edit the draft submission.
 *    Otherwise it creates a form for a new, empty submission. As usual.
 * 2) The submit button is pressed. The form is retrieved from cache or is
 *    recreated by webform_client_form. The values from the $_POST are merged in
 *    and the validation routines are called. As usual.
 * 3) The postvalidation routine, below, detects that validation errors should
 *    be autosaved and calls the submit handlers on a copy of the form and
 *    form_state. This creates the submission, or saves to the existing
 *    submission. The original form and form_state are not modified (yet).
 * 4) If a new submission was created, the form and form_state are updated with
 *    the newly-created sid of the submission, which is returned to the
 *    browser in the hidden field [details][sid]. The form is set to not be
 *    cached, and any existing cached copy is cleared to force step 5. The form
 *    is presented with validation errors as usual.
 * 5) When the form is submitted again, the form must be rebuilt because it is
 *    not in the cache. The existing draft detection in _webform_fetch_draft_sid
 *    detects that a webform draft is being submitted, and uses its sid in
 *    preference to any other stored draft sid in the database. In the event
 *    that multiple drafts are being implemented by another module, this ensures
 *    that the correct draft is edited.
 * 6) Repeat from step 2 until the form is abandoned (leaving the draft) or
 *    successfully submitted.
 */
function webform_client_form_postvalidate(&$form, &$form_state) {
  $errors = form_get_errors();
  $nid = $form_state['values']['details']['nid'];
  $node = node_load($nid);
  if (user_is_logged_in() &&
      $errors && !isset($errors['']) &&
      $node->webform['auto_save'] &&
      !$form_state['values']['details']['finished'] &&
      !empty($form_state['values']['op'])) {
    // Validation errors are present, prevalidation succeeded (e.g. submission
    // limits are ok), auto-save is enabled, this form isn't finished (i.e. is
    // or soon will be a draft) and a button was pushed (not ajax).

    // Process submission on a copy of the form and form_state to prevent the
    // submission handlers from making unintended changes. Use a button that
    // isn't Save Draft, Next Page, Submit, etc to avoid triggering any
    // unwanted side effects.
    $submit_form = $form;
    $submit_form_state = $form_state;
    $submit_form_state['values']['op'] = '__AUTOSAVE__';
    form_execute_handlers('submit', $submit_form, $submit_form_state);
    $sid = $submit_form_state['values']['details']['sid'];
    if ($sid != $form_state['values']['details']['sid']) {
      // A new submission was created. Update the form and form_state as if it
      // has been submitted with the new sid. This causes the Form API to
      // render the form with new sid.
      $form_state['values']['details']['sid'] = $sid;
      $form_state['input']['details']['sid'] = $sid;
      $form['details']['sid']['#value'] = $sid;
      
      // Prevent the form from being cached, forcing it to be rebuilt from the
      // form definition function, which will honor the new sid.
      $form_state['no_cache'] = TRUE;
      if (!variable_get('cache', 0) && !empty($form_state['values']['form_build_id'])) {
        cache_clear_all('form_' . $form_state['values']['form_build_id'], 'cache_form');
        cache_clear_all('form_state_' . $form_state['values']['form_build_id'], 'cache_form');
      }
    }
  }
}

Committed to 7.x-4.x and 8.x because the existing release candidate is flawed and a prompt fix is appropriate.

If there are further issues with this feature, my inclination is to abandon it and revert all the commits.

danchadwick’s picture

D.o seemed to have difficulty uploading the patch. Re-uploading.

  • DanChadwick committed e3dd315 on 7.x-4.x
    Issue #2284595 by Dan Chadwick: Fixed rebuilding new auto-saved draft...
  • DanChadwick committed 3b2c563 on 8.x-4.x
    Issue #2284595 by Dan Chadwick: Fixed rebuilding new auto-saved draft...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.