Form caches should be deleted after submission

Looks good to me, but should probably be reviewed by somebody with deeper Form API knowledge.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -490,8 +497,10 @@ public function processForm($form_id, &$form, FormStateInterface &$form_state) {
       if (!$form_state->isRebuilding() && !FormState::hasAnyErrors()) {
         if ($submit_response = $this->formSubmitter->doSubmitForm($form, $form_state)) {
+          $this->formCache->deleteCache($form_state->getValue('form_build_id'));
           return $submit_response;
         }
+        $this->formCache->deleteCache($form_state->getValue('form_build_id'));

Perhaps I'm missing something, but it looks like this line could be added in between the first and the second if statement?

I think it would make sense to also test that we really delete form-state on form-submission. Not sure whether FormTestBase::simulateFormSubmission() will trigger the removal.

#8 Yes, we absolutely can use that (we need to flag it as a non-programmed submission.

Addressed #7 as well. We want to do it after doSubmitForm() runs, but before returning the response.

Ugh, wrong patch, right interdiff.

EDIT: The new tests I added pass in isolation, but not as part of the larger suite. Debugging now.

We have a stray static to clear.

+1 to this patch from me. It makes 8.0.x consistent with 7.x and earlier versions. I also left a comment in #512026-180: Move $form_state storage from cache to new state/key-value system and suggest holding off on RTBC until people following that issue have had a chance to comment here.

Inspired by #2465053-21: Drupal 8 only allows one user every 6 hours to register when page caching is enabled — caused by entity UUID in form state, I tried the following:

1. Create a node that you can comment on, then visit that node page.
2. Type in a comment, click "Preview". This creates a multistep workflow (i.e., CommentForm::preview() calls $form_state->setRebuild()), which causes $form_state to be persisted.
3. On the Preview step, click "Save". In HEAD, this does not delete the $form_state stored in step 2. With this issue, it should.
4. Use the back button. The browser is nice to show you the prior "Preview" step.
5. Click "Save" again.

In HEAD, this results in a fatal error, because the undeleted $form_state includes the serialized comment, which includes a UUID, and so step 5's Save attempts to create another comment with the same UUID. #2465053: Drupal 8 only allows one user every 6 hours to register when page caching is enabled — caused by entity UUID in form state might change it so that UUIDs don't get persisted in $form_state, but this still demonstrates a general problem of a form state becoming invalid after submission, prior to the 6 hour expiration.

With #12, I expected this to get fixed, but it doesn't, because alexpott was partially correct in #512026-146: Move $form_state storage from cache to new state/key-value system:

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -488,8 +495,15 @@ public function processForm($form_id, &$form, FormStateInterface &$form_state) {
-        if ($submit_response = $this->formSubmitter->doSubmitForm($form, $form_state)) {
+        $submit_response = $this->formSubmitter->doSubmitForm($form, $form_state);
+        // If this form was cached, delete it from the cache after submission.
+        if ($form_state->isCached()) {
+          $this->deleteCache($form_state->getValue('form_build_id'));
+        }

The 'form_build_id' is no longer in $form_state, because for entity forms, the doSubmitForm() invokes EntityForm::submitForm(), which calls $form_state->cleanValues(). Changing the above to:

$this->deleteCache($form['#build_id']);

fixes that problem.

With that change applied, the use case above works (a new comment gets saved), because $form_state gets deleted from storage in step 3, and so step 5 happens with a brand new, reconstructed $form_state. Note that a different use case might not work the same way. For example, a complex multistep workflow where there's critical information in the last step's $form_state that can't be newly reconstructed. In such a case, clicking back and resubmitting might result in some other error. But, it would be the same error as would happen in HEAD anyway if you waited for 6 hours and then did it. In other words, contrib modules building such workflows need to account for deleted form states regardless since they can expire.

NW for the change above plus we should probably expand the tests to cover entity forms.

Adjusted the unit tests to not test get/set, just delete, and mimicked the ->cleanValues() call in the unit test.

Thanks to @Wim Leers for the clever suggestion on how to mimic hitting the back button in a web test!

Bumping this up.

RTBC + added a beta evaluation to the IS.

The table of patch files at the top of the issue is ordered wrong. Trying to at least move the latest patch to the top.

ahem, trying again.

Assigning to alexpott, because of #15 and because he originally authored the change to not delete after submission in #512026-146: Move $form_state storage from cache to new state/key-value system.

RTBC + 1

Leaving for Alex, but patch looks right to me.

Looks good to me too. Committed 189be72 and pushed to 8.0.x. Thanks!