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
Comment #1
danchadwick commentedIn 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.
Comment #2
danchadwick commentedImplemented as described. The auto save option was reworded to include saving submissions that fail validation. Committed to 7.x-4.x and 8.x
Comment #4
danchadwick commentedAlas, 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).
Comment #5
danchadwick commentedCommitted #4 to 7.x-4.x and 8.x.
Comment #7
quicksketchThanks 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.
Comment #8
danchadwick commentedYes, 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.
Comment #9
danchadwick commentedComment #12
danchadwick commentedI 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:
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).
Comment #14
quicksketchI 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.
Comment #15
danchadwick commentedYeah, that would be nice, but no dice when there are errors. Here's the end of drupal_process_form:
Comment #16
danchadwick commentedArgh. 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.
Comment #17
danchadwick commentedSolution 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.
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.
Comment #18
danchadwick commentedD.o seemed to have difficulty uploading the patch. Re-uploading.