Move form validation logic out of FormBuilder into a new class

Here's how I envisioned this.
I was also able to add more comprehensive unit tests, which is a major plus.

Removed left over code I meant to refactor out.

More test coverage.

+++ b/core/includes/form.inc
@@ -345,12 +346,18 @@ function drupal_redirect_form($form_state) {
-  \Drupal::formBuilder()->executeHandlers($type, $form, $form_state);
+  if ($type == 'submit') {
+    \Drupal::formBuilder()->executeSubmitHandlers($form, $form_state);
+  }
+  elseif ($type == 'validate') {
+    \Drupal::service('form_validator')->executeValidateHandlers($form, $form_state);
+  }

@@ -844,12 +851,12 @@ function form_set_value($element, $value, &$form_state) {
- *   Use \Drupal::formBuilder()->flattenOptions().
+ *   Use \Drupal\Core\Form\FormHelper::flattenOptions().

Splitting executeHandlers() into two methods, and moving flattenOptions into a static helper class are the only API changes here.

For BC purposes, FormBuilder still implements all of the original methods, and just passes the calls through to the new FormValidator class.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -32,8 +29,7 @@
-  use StringTranslationTrait;

@@ -1750,22 +1381,6 @@ protected function drupalInstallationAttempted() {
   /**
-   * Wraps watchdog().
-   */
-  protected function watchdog($type, $message, array $variables = NULL, $severity = WATCHDOG_NOTICE, $link = NULL) {
-    watchdog($type, $message, $variables, $severity, $link);
-  }
-
-  /**
-   * Wraps drupal_set_message().
-   *
-   * @return array|null
-   */
-  protected function drupalSetMessage($message = NULL, $type = 'status', $repeat = FALSE) {
-    return drupal_set_message($message, $type, $repeat);
-  }

A side benefit is that FormBuilder no longer cares about string translation, drupal_set_message, or watchdog.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -92,24 +88,15 @@ class FormBuilder implements FormBuilderInterface {
-  protected $forms;
...
-  protected $flattenedOptions = array();

$forms was leftover, and $flattenedOptions is now part of FormHelper, so now all of the properties of FormBuilder are other services, no more state!

Nice refactoring!!

1. +++ b/core/lib/Drupal/Core/Form/FormHelper.php
   @@ -0,0 +1,58 @@
   +/**
   + * Provides utility methods used by form processing.
   + */
   +class FormHelper {
   

   Could we just go with FormUtility or something better?

2. +++ b/core/lib/Drupal/Core/Form/FormValidator.php
   @@ -0,0 +1,532 @@
   +
   +  /**
   +   * The current request.
   +   *
   +   * @var \Symfony\Component\HttpFoundation\Request
   +   */
   +  protected $request;
   ...
   +  public function __construct(UrlGeneratorInterface $url_generator, CsrfTokenGenerator $csrf_token = NULL) {
   

   The injection seems wrong. We should also use the stack

3. +++ b/core/lib/Drupal/Core/Form/FormValidatorInterface.php
   @@ -0,0 +1,67 @@
   +
   +  /**
   +   * Sets the request object to use.
   +   *
   +   * @param \Symfony\Component\HttpFoundation\Request $request
   +   *   The request object.
   +   */
   +  public function setRequest(Request $request);
   +
   

   Let's really use the stack.

Rerolled this on top of #2225605: Use request stack in form builder, it now includes that patch.

Per a discussion on IRC with sun, opened #2257835: Move form submission logic out of FormBuilder into a new class to do the same for submit.
Also when rewriting the services, I mistakenly did not inject string translation. Thanks @dawehner and @sun for catching that.
Fixed that and removed the optional-ness of CSRF generator.

Rerolled, that's in.

The only remaining remark I had/have is whether we can rename the FormValidator::executeValidateHandlers() method into FormValidator::executeHandlers()?

The repetition of "validate" is unnecessarily verbose on a separate class/object. Especially given the follow-up issue, having a consistent executeHandlers() method on both the FormValidatorInterface and the FormSubmitterInterface would be logical.

This means that FormBuilder cannot implement FormValidatorInterface, but I'm interpreting that just as a temporary legacy/BC layer anyway, so we can easily resolve that by leaving the current methods on FormBuilderInterface + just replace their phpDoc to state that they are @deprecated proxies for the corresponding FormValidator methods.

Aside from that, I really like the idea of exposing the main 3 concepts of Form API as separate classes (build/validate/submit). That will certainly help many developers to understand the fundamental architectural design of our form processing.

I think I'd like to hold off on renaming that until #2257835: Move form submission logic out of FormBuilder into a new class, because I'm honestly not as confident about that issue as this one. And if that one doesn't pan out, changing executeValidateHandlers to executeHandlers will make much less sense.

Hm. That would mean a second API change though...

I think it would be weird/confusing to only split out validation. I'm marking this RTBC on the assumption that we'll also split out submission.

Yes, that's less much less code and it might be harder (because some parts are intermixed into [re]building) — however, we wouldn't have to move the input processing, just the executeHandlers() + submitForm() logic that's contained in processForm()... at least that's what I thought of regarding parity for clarity.

Let's get a followup to move OptGroup tests out of FormBuilderTest.

Plus before this can be done we need a change notice or a list of change notices that need updating.

Committed f710a6c and pushed to 8.x. Thanks!

Let's update the change notices https://drupal.org/node/2121003 and https://drupal.org/node/2241767

Created #2259301: Move OptGroup tests out of FormBuilderTest

Added https://drupal.org/node/2259297 as a change notice for OptGroup, and updated those two.
Thanks, see you in #2257835: Move form submission logic out of FormBuilder into a new class

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -801,97 +786,7 @@ public function prepareForm($form_id, &$form, &$form_state) {
    * {@inheritdoc}

@@ -973,209 +868,23 @@ public function redirectForm($form_state) {
+   * {@inheritdoc}

@@ -1202,93 +911,42 @@ public function executeHandlers($type, &$form, &$form_state) {
    * {@inheritdoc}
...
    * {@inheritdoc}
...
    * {@inheritdoc}
...
    * {@inheritdoc}
...
    * {@inheritdoc}
...
    * {@inheritdoc}

I think @deprecated can be added here.