Move form submission logic out of FormBuilder into a new class

Awesome! That's exactly the parity I thought of :-)

1. +++ b/core/core.services.yml
   @@ -122,10 +122,13 @@ services:
   +  form_submission:
   +    class: Drupal\Core\Form\FormSubmission
   

   For consistency, I think the service ID should be form_submitter and the class name should be FormSubmitter.

   Alternatively, we might want to go back to the other issue and quickly rename the validator into validation prior to commit?

2. +++ b/core/lib/Drupal/Core/Form/FormSubmission.php
   @@ -0,0 +1,232 @@
   +  public function handleFormSubmission(&$form, &$form_state) {
   ...
   +  public function redirectForm($form_state) {
   ...
   +  public function executeSubmitHandlers(&$form, &$form_state) {
   

   Minor: The order of methods looks a bit off/reversed in the class and interface...

   1. submitForm
   2. executeSubmitHandlers
   3. redirectForm

3. +++ b/core/lib/Drupal/Core/Form/FormSubmissionInterface.php
   @@ -0,0 +1,30 @@
   +  public function handleFormSubmission(&$form, &$form_state);
   

   The validator interface calls the corresponding method validateForm() — any particular reason for not renaming this submitForm()?

I'm not convinced that FormBuilder::submitForm() belongs to this class. It lives/operates at the opposite end; i.e., it's an initial entry point to build a form + futz with $form_state in a way to auto-submit it, but doesn't interfere with the actual form submission logic.

In turn, IMO this patch already covers pretty much everything the class should cover. Just some polishing and I'd consider it done :-)

Apologies, I don't know how, but somehow I completely missed the following moved methods in the earlier patch:

+++ b/core/lib/Drupal/Core/Form/FormSubmissionProcessor.php
@@ -0,0 +1,319 @@
+  public function determineTriggeringElement($element, $form_state) {
...
+  protected function elementTriggeredScriptedSubmission($element, $form_state) {
...
+  protected function buttonWasClicked($element, $form_state) {

+++ b/core/lib/Drupal/Core/Form/FormSubmissionProcessorInterface.php
@@ -0,0 +1,119 @@
+  public function determineTriggeringElement($element, $form_state);

These methods are not part of the form submission/processing logic at all.

They belong to the user input processing, which is (currently) performed during form building. Their abstract meaning is:

Determine if some values in $_GET or $_POST are mapping to some form elements, so we can *pretend* that those (button) elements saw user input and were "clicked", even if they were actually not.

A positive result of that user input processing is stored in $form_state - as if the user input had contained the actually required values to trigger that state natively (even though it did not).

This information must be set and is consumed before the form validation + submission workflow begins. The effective triggering element can have additional form validation handlers assigned to it.

That is why it is not part of the submission workflow.

Thanks. Last remaining step from my perspective is: Naming.

Can we rename FormSubmissionProcessor into just simply FormSubmitter, please?

(ibid on the service ID: form_submission_processor → form_submitter)

FormSubmissionProcessor is completely out of line compared to everything else, which defeats the whole point of splitting up the main Form API concepts. FormProcessor cannot be used as name, because major parts of the actual form processing logic still live in FormBuilder::processForm() (which is fine). The FormValidator isn't a FormValidationProcessor either. Despite the offer in #5, you objected to rename FormValidator. This is about the submission workflow of a form only; hence, FormSubmitter.

Second, you wanted to re-order the methods in the class + interface to follow #5.2.

FormSubmitter in analogy to FormValidator makes sense to me. Also, it's much shorter without really being less informative.

Thanks for your prompt feedback, @pwolanin.

@tim.plunkett: Also, if you'd explain the underlying reasoning for naming it FormSubmissionProcessor, then others might be able to relate to that (whichever) idea. Right now, I'm not able to see any reasoning that would make sense of that naming, so I assume you're interpreting it as "bikeshed" whereas I'm clearly not — the parity of FormBuilder + FormValidator + FormSubmitter covers the main 3 Form API concepts in distinct classes whose names are fully in line.

handleFormSubmission() diverges from FormValidator::validateForm(). Sadly we cannot use submitForm(), because FormBuilder::submitForm() already exists. Therefore:

1. Renamed handleFormSubmission() to doSubmitForm().
2. Throw exception when FormBuilder::doSubmitForm() is called directly.

In light of that, I think we should create a follow-up issue to remove the additional interfaces from FormBuilder, but I'm happy to move forward with this for now.

That was my only remaining remark, so if we're fine with that, then I think this is RTBC.

I only renamed that method name, so I'm comfortable to RTBC this patch. Thanks!

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -793,78 +744,7 @@ public function validateForm($form_id, &$form, &$form_state) {
    * {@inheritdoc}

@@ -878,33 +758,14 @@ public function executeValidateHandlers(&$form, &$form_state) {
   public function executeSubmitHandlers(&$form, &$form_state) {

I think @deprecated can be added here.

20: form.submitter.20.patch queued for re-testing.

Here is the Rerolled patch.

Back to RTBC.

Committed/pushed to 8.x, thanks!