Move the form builder functions in form.inc to a form service

1. +++ b/core/includes/form.inc
   @@ -4866,28 +3954,6 @@ function _form_set_attributes(&$element, $class = array()) {
   -function _drupal_form_send_response(Response $response) {
   
   +++ b/core/lib/Drupal/Core/Form/Form.php
   @@ -0,0 +1,1520 @@
   +  protected function sendResponse(Response $response) {
   

   hell yeah! now one cant use that, unless sub-classing:)

2. +++ b/core/lib/Drupal/Core/Form/Form.php
   @@ -0,0 +1,1520 @@
   +class Form {
   

   how about adding an interface in the mix, making possible to do all the kinds of stuff in contrib?

Make an off handed comment in IRC, Tim writes a 126 KB patch. Life is good. :-)

1. +++ b/core/core.services.yml
   @@ -106,6 +106,9 @@ services:
   +  form:
   +    class: Drupal\Core\Form\Form
   +    arguments: ['@module_handler', '@event_dispatcher']
   

   This should probably be called form_renderer, or something like that. "form" implies it is "THE form for the site", which it is not.

2. +++ b/core/lib/Drupal/Core/Form/Form.php
   @@ -0,0 +1,1520 @@
   +/**
   + * @todo.
   + */
   +class Form {
   +
   

   I agree with Paris that this needs an interface. Nearly all services need an interface. :-)

3. +++ b/core/lib/Drupal/Core/Form/Form.php
   @@ -0,0 +1,1520 @@
   +  /**
   +   * Triggers kernel.response and sends a form response.
   +   *
   +   * @param \Symfony\Component\HttpFoundation\Response $response
   +   *   A response object.
   +   */
   +  protected function sendResponse(Response $response) {
   

   We should probably add a note in the docblock that this is horrible but FAPI can't handle returning the response object at this time. Acknowledge the flaws so you don't get blamed for them later. :-)

4. +++ b/core/lib/Drupal/Core/Form/Form.php
   @@ -0,0 +1,1520 @@
   +  /**
   +   * Returns the CSRF token manager service.
   +   *
   +   * @return \Drupal\Core\Access\CsrfTokenGenerator
   +   */
   +  protected function csrfToken() {
   +    return \Drupal::csrfToken();
   +  }
   +
   +  /**
   +   * Returns the HTTP kernel.
   +   *
   +   * @return \Drupal\Core\HttpKernel
   +   */
   +  protected function httpKernel() {
   +    return \Drupal::service('http_kernel');
   +  }
   

   This is a service. We can just have this as normal dependencies. No need for \Drupal.

Also, a 1500 line class means something is wrong. I suspect there's a lot of refactoring that could be done to break up the class into more discrete parts. Whether that's something worth doing here, or even worth doing at all if we may be replacing it in Drupal 9, is a debatable point, I grant. If we decide not to, we should document/@todo why it wasn't done.

Ha! That's competing with the database then on lines of comment vs. lines of code. :-)

Tagging.

Impressive work!

1. +++ b/core/core.services.yml
   @@ -106,6 +106,9 @@ services:
        arguments: [default]
   +  form_builder:
   +    class: Drupal\Core\Form\FormBuilder
   +    arguments: ['@module_handler', '@keyvalue.expirable', '@event_dispatcher', '@url_generator', '@request', '@?csrf_token', '@?http_kernel']
   

   It would be good to have the request added via setter injection so synching the request later would actually work.

2. +++ b/core/includes/form.inc
   @@ -111,515 +104,57 @@
   + * @deprecated Use \Drupal::formBuilder()->buildForm()
     */
   ...
   + *
   + * @deprecated Use \Drupal::formBuilder()->getFormStateDefaults()
   

   Don't we always add since which version the particular function is considered as deprecated?

3. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -0,0 +1,1354 @@
   +   *   The module handler.
   ...
   +   *   The keyvalue expirable factory.
   ...
   +   *   The event dispatcher.
   ...
   +   *   The URL generator.
   ...
   +   *   The current request.
   ...
   +   *   The CSRF token generator.
   ...
   +   *   The HTTP kernel.
   

   I know this is out of scope of the patch though I would like to add the "need" or usage of the different dependencies. For example:
   The CSRF token generator to validate the form token.

4. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -0,0 +1,1354 @@
   +      $form_state['input'] = $form_state['method'] == 'get' ? $_GET : $_POST;
   

   I know this is c&p though can't we just use the new request object now?

5. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -0,0 +1,1354 @@
   +      $form['#build_id'] = 'form-' . Utility\Crypt::hashBase64(uniqid(mt_rand(), TRUE) . mt_rand());
   

   Is there a reason why we don't use the full use statement?

6. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -0,0 +1,1354 @@
   +        $url = url($path, array('query' => $query));
   

   url() to url generator?

7. +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
   @@ -0,0 +1,606 @@
   +  /**
   +   * Retrieves default values for the $form_state array.
   +   */
   +  public function getFormStateDefaults();
   

   Is there a reason why this has to be public?

8. +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
   @@ -0,0 +1,404 @@
   +   * @var \PHPUnit_Framework_MockObject_MockObject
   ...
   +   * @var \PHPUnit_Framework_MockObject_MockObject
   

   I really like if we also typehint the actual interface, so we have autocompletion for both

9. +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
   @@ -0,0 +1,404 @@
   +  public function testRedirectForm($form_state, $result = NULL, $status = 302) {
   +    if ($result) {
   +      $this->urlGenerator->expects($this->once())
   +        ->method('generateFromPath')
   +        ->will($this->returnValueMap(array(
   +          array(NULL, array('query' => array(), 'absolute' => TRUE), '<front>'),
   +          array('foo', array('absolute' => TRUE), 'foo'),
   +          array('bar', array('query' => array('foo' => 'baz'), 'absolute' => TRUE), 'bar'),
   +          array('baz', array('absolute' => TRUE), 'baz'),
   +        ))
   +      );
   +    }
   +    else {
   +      $this->urlGenerator->expects($this->never())
   +        ->method('generateFromPath');
   +    }
   +
   +    $form_state += $this->formBuilder->getFormStateDefaults();
   +    $redirect = $this->formBuilder->redirectForm($form_state);
   +    if ($result) {
   +      $this->assertSame($result, $redirect->getTargetUrl());
   +      $this->assertSame($status, $redirect->getStatusCode());
   +    }
   +    else {
   +      $this->assertNull($redirect);
   +    }
   +  }
   +
   +  /**
   

   We could also just split up the test into two test functions: testRedirectWithResult testRedirectWithoutResult.

10. +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
    @@ -0,0 +1,404 @@
    +  function test_form_id() {
    +    $form['test'] = array(
    +      '#type' => 'textfield',
    +      '#title' => 'Test',
    +    );
    +    return $form;
    +  }
    

    We don't necessarily have to put this into the global namespace

It seems to be the wrong approach to move the static variables in all the different form error functions to the service as the service should be stateless.

Is it possible to keep the form errors out for now and think about a better way later?

This looks like a nice step forward. The FormBuilder class is doing quite a bit, but that can be factored out later. The cache and validation seem like good candidates for extraction in a followup.

I wouldnt worry too much about the statelessness of this service at the moment. A bigger problem is all the implicit dependencies it has. I'd be nice if more of them were wrapped in local methods like getElementInfo to make them more explicit, keep things testable, and make it easier to refactor later.

+++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
@@ -62,8 +62,12 @@ public function setUp() {
+    $translation_manager = $this->getMock('Drupal\Core\StringTranslation\TranslationInterface');
+    $translation_manager->expects($this->any())
+      ->method('translate')
+      ->will($this->returnArgument(0));

there is a getStringTranslationStub method in your base class which does exactly this

1. +++ b/core/includes/form.inc
   @@ -111,515 +104,57 @@
   -  return form_builder($form_id, $form, $form_state);
   +  \Drupal::formBuilder()->rebuildForm($form_id, $form_state, $old_form);
   

   we should return it, shouldnt we? i mean the old code used to

2. +++ b/core/includes/form.inc
   @@ -674,1560 +209,118 @@ function form_load_include(&$form_state, $type, $module, $name = NULL) {
   +  \Drupal::formBuilder()->retrieveForm($form_id, $form_state);
   ...
   -  return $form;
   

   same here

Sorry, but i didnt do the review with dreditor, it is very laggy and CPU is killing me

1)

+   * @dataProvider providerTestRedirectWithResult
..
+  public function providerTestRedirectWIthResult() {

Case insensitivity made this hide. lets make the I lowercase

2)

+  public function testBuildFormWithObject() {
..
+  public function testBuildForm() {

Those test methods are exactly the same?

3)

+
+  /**
+   * {@inheritdoc}
+   */
+  public function prepareForm($form_id, &$form, &$form_state) {
....
+      $form['#build_id'] = 'form-' . Utility\Crypt::hashBase64(uniqid(mt_rand(), TRUE) . mt_rand());
....
+
+  /**
+   * {@inheritdoc}
+   */
+  public function rebuildForm($form_id, &$form_state, $old_form = NULL) {
....
+      $form['#build_id'] = 'form-' . Utility\Crypt::hashBase64(uniqid(mt_rand(), TRUE) . mt_rand());

Utility\Crypt is weird :S why not the full namespace or, even better no namespace and use statement?
It also happens for other Utitlity classes eg Url and NestedArrray in validateForm()
for Unicode in doValidateForm()

4)

+  /**
+   * {@inheritdoc}
+   */
+  public function getCache($form_build_id, &$form_state) {
....
+              module_load_include($file['type'], $file['module'], $file['name']);

Can we use the module handler here?

5)

+  /**
+   * {@inheritdoc}
+   */
+  public function processForm($form_id, &$form, &$form_state) {
..
+        // Redirect the form based on values in $form_state.
+        $redirect = self::redirectForm($form_state);

Should be $this->redirectForm

6)

+  /**
+   * {@inheritdoc}
+   */
+  public function validateForm($form_id, &$form, &$form_state) {
....
+      if (!$this->csrfToken->validate($form_state['values']['form_token'], $form['#token'])) {
+        $path = current_path();

lets retrieve $path from $this->request

7) Finally if you have set php to show strict errors phpunit testsuite fails now because of the barch_get madness:/

Wohooo great work @tim.plunkett. A lot of doc issues. If you think these things can move to follow up issues please create one or add suggestion I'll create those.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -0,0 +1,1770 @@
   +   * An array of known forms.
   

   Can we add @see hook_forms and some more explanation?

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -0,0 +1,1770 @@
   +        $form_arg = $form_arg::create(\Drupal::getContainer());
   

   Why not $this->getContainer() or simply $this->container?

3. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -0,0 +1,1770 @@
   +      $form_state['input'] = $form_state['method'] == 'get' ? $_GET : $_POST;
   

   Can we use $request here? Instead of $_GET or $_POST.

4. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -0,0 +1,1770 @@
   +    // If the incoming input contains a form_build_id, we'll check the cache for a
   

   80 chars.

5. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -0,0 +1,1770 @@
   +      $form = $this->getCache($form_state['input']['form_build_id'], $form_state);
   ...
   +    if (!isset($form)) {
   

   I think we should declare $form and use empty/is_array for check here.

6. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -0,0 +1,1770 @@
   +    // If this was a successful submission of a single-step form or the last step
   

   80 chars

7. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -0,0 +1,1770 @@
   +    // cached is the $form structure before it passes through self::doBuildForm(),
   

   80 chars

8. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -0,0 +1,1770 @@
   +      array_shift($args);
   +      array_shift($args);
   

   A simple doc line would be great here.

9. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -0,0 +1,1770 @@
   +    // menu_get_item() is not available.
   

   Is this correct? I don't see any call to menu_get_item(). Yeah $this->menuGetItem() is here so should we $this->menuGetItem() add here.

10. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -0,0 +1,1770 @@
    +      // In cases where many form_ids need to share a central constructor function,
    

    80 chars.

11. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -0,0 +1,1770 @@
    +    // since call_user_func_array() requires that referenced variables are passed
    ...
    +    // builder function to pre-populate the $form array with form elements, which
    

    80 chars

12. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -0,0 +1,1770 @@
    +    // self::doBuildForm() finishes building the form by calling element #process
    

    80 chars

13. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -0,0 +1,1770 @@
    +      // drupal_html_id() maintains a cache of element IDs it has seen,
    +      // so it can prevent duplicates. We want to be sure we reset that
    +      // cache when a form is processed, so scenarios that result in
    +      // the form being built behind the scenes and again for the
    +      // browser don't increment all the element IDs needlessly.
    

    expand it to 80 chars.

14. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -0,0 +1,1770 @@
    +      // made to $form_state during this request. Normally, a submit handler sets
    ...
    +      // submissions triggered by non-buttons), there is no submit handler to set
    ...
    +      // @todo D8: Simplify this logic; considering Ajax and non-HTML front-ends,
    +      //   along with element-level #submit properties, it makes no sense to have
    ...
    +        // may use $form_state['rebuild'] to determine if they are running in the
    ...
    +    // shall be cached. But the form may only be cached if the 'no_cache' property
    +    // is not set to TRUE. Only cache $form as it was prior to self::doBuildForm(),
    +    // because self::doBuildForm() must run for each request to accommodate new user
    ...
    +    // multi-step forms, this allows the user to go back to an earlier build, make
    ...
    +    // This does not apply to programmatically submitted forms. Furthermore, since
    

    80 chars.

15. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -0,0 +1,1770 @@
    +    // Invoke hook_form_alter(), hook_form_BASE_FORM_ID_alter(), and
    +    // hook_form_FORM_ID_alter() implementations.
    

    Oh this is the place where magic happens :D

16. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -0,0 +1,1770 @@
    +    // If validation errors are limited then remove any non validated form values,
    ...
    +      // $form_state['values'] locations, even if these locations are not included
    ...
    +        // dictated by #parents. If it is, copy it to $values, but do not override
    

    80 chars.

17. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -0,0 +1,1770 @@
    +   * First ensures required fields are completed, #maxlength is not exceeded, and
    

    80 chars

18. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -0,0 +1,1770 @@
    +            $options = form_options_flatten($elements['#options']);
    

    Hello OO?

19. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -0,0 +1,1770 @@
    +          // browser behavior, required select fields without a #default_value get
    ...
    +      // to self::setErrorByName() be suppressed and not result in a form error, so
    +      // that a button that implements low-risk functionality (such as "Previous"
    +      // or "Add more") that doesn't require all user input to be valid can still
    ...
    +      // #limit_validation_errors property is ignored if submit handlers will run,
    +      // but the element doesn't have a #submit property, because it's too large a
    ...
    +      // If submit handlers won't run (due to the submission having been triggered
    +      // by an element whose #executes_submit_callback property isn't TRUE), then
    ...
    +      // end of this function, doing it here is only to handle the rare edge case
    ...
    +        // An unchecked checkbox has a #value of integer 0, different than string
    ...
    +        // set a form error message. So when a visible title is undesirable, form
    ...
    +    // self::doValidateForm() turns it on again when starting on the next element, if
    ...
    +        // errors are suppressed. For example, a "Previous" button might want its
    +        // submit action to be triggered even if none of the submitted values are
    

    80 chars.

20. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -0,0 +1,1770 @@
    +        !url_is_external($element['#action'])) {
    

    Url::isExternal($path);

21. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -0,0 +1,1770 @@
    +      // Set a flag if we have a correct form submission. This is always TRUE for
    +      // programmed forms coming from self::submitForm(), or if the form_id coming
    ...
    +      // Prior to checking properties of child elements, their default properties
    ...
    +    // Final tasks for the form element after self::doBuildForm() has run for all other
    ...
    +      // identifying any submit button. Other browsers submit POST data as though
    ...
    +      // If the triggering element specifies "button-level" validation and submit
    +      // handlers to run instead of the default form-level ones, then add those to
    ...
    +        // have been determined (including from input variables set by JavaScript
    ...
    +        // have their #name property not derived from their #parents property, we
    ...
    +    // control's attributes. However, it's good UI to let the user know that input
    ...
    +    // consider whether a multi-step form would be more appropriate (#disabled can
    ...
    +    // affect when a control is enabled, then it is best for accessibility for the
    ...
    +        // Get the input for the current element. NULL values in the input need to
    ...
    +        // For browser-submitted forms, the submitted values do not contain values
    ...
    +        // distinguish elements having NULL input from elements that were not part
    ...
    +        // Call #type_value without a second argument to request default_value handling.
    ...
    +        // Final catch. If we haven't set a value yet, use the explicit default value.
    +        // Avoid image buttons (which come with garbage value), so we only get value
    ...
    +      // can only have been triggered by a button, and we need to determine which
    ...
    +        // form_state_values_clean() and for the self::doBuildForm() code that handles
    ...
    +   * This detects button or non-button controls that trigger a form submission via
    ...
    +   * key '_triggering_element_value' may also be set to require a match on element
    ...
    +   * This detects button controls that trigger a form submission by being clicked
    

    80 chars.

22. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -0,0 +1,1770 @@
    +   * Wraps element_info().
    ...
    +   * Wraps drupal_installation_attempted().
    ...
    +   * Wraps menu_get_item().
    ...
    +   * Wraps watchdog().
    ...
    +   * Wraps drupal_set_message().
    ...
    +   * Wraps element_children().
    ...
    +   * Wraps drupal_html_class().
    ...
    +   * Wraps drupal_html_id().
    ...
    +   * Wraps drupal_static_reset().
    

    @see to the function or @todo to remove it.

23. +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
    @@ -0,0 +1,763 @@
    +   * This function should be used instead of drupal_build_form() when $form_state
    ...
    +   *   reference so that the caller can use it to examine what in the form changed
    ...
    +   *   to store information related to the processed data in the form, which will
    ...
    +   *   - build_info: Internal. An associative array of information stored by Form
    ...
    +   *     - callback: The actual callback to be used to retrieve the form array. If
    ...
    +   *     - files: An optional array defining include files that need to be loaded
    ...
    +   *       another array containing values for the parameters 'type', 'module' and
    ...
    +   *     immediately built and sent to the browser, instead of a redirect. This is
    ...
    +   *     Normally, $form_state['rebuild'] is set by a submit handler, since it is
    ...
    +   *     done or requires another step. However, a validation handler may already
    +   *     set $form_state['rebuild'] to cause the form processing to bypass submit
    ...
    +   *   - method: The HTTP form method to use for finding the input for this form.
    ...
    +   *     forms do not use form ids so are always considered to be submitted, which
    ...
    +   *     cached, which allows the entire form to be rebuilt from cache. A typical
    ...
    +   *     page request to the one that processes the submission. 'cache' can be set
    +   *     to TRUE to do this. A prominent example is an Ajax-enabled form, in which
    ...
    +   *     (multi-step) forms having the 'rebuild' flag set, regardless of the value
    ...
    +   *   - no_cache: If set to TRUE the form will NOT be cached, even if 'cache' is
    ...
    +   *   - input: The array of values as they were submitted by the user. These are
    ...
    +   *     understanding of security implications. In almost all cases, code should
    ...
    +   *     (see 'programmed' key), or if the form_id coming from the $_POST data is
    ...
    +   *     submission, which may or may not be a button (in the case of Ajax forms).
    +   *     This key is often used to distinguish between various buttons in a submit
    ...
    +   *   - has_file_element: Internal. If TRUE, there is a file element and Form API
    ...
    +   *     the location where application-specific data was stored for communication
    ...
    +   *     recommended way to ensure that the chosen key doesn't conflict with ones
    ...
    +   *     available across successive clicks of the "Preview" button (if available)
    ...
    +   *   - complete_form: A reference to the $form variable containing the complete
    ...
    +   *     handlers being invoked on a form element may use this reference to access
    ...
    +   *   - temporary: An array holding temporary data accessible during the current
    +   *     page request only. All $form_state properties that are not reserved keys
    ...
    +   *     not be cached during the whole form workflow; e.g., data that needs to be
    +   *     accessed during the current form build process only. There is no use-case
    ...
    +   *   The rendered form. This function may also perform a redirect and hence may
    ...
    +   * finished. If a validate or submit handler set $form_state['rebuild'] to TRUE,
    ...
    +   * Ajax form submissions are almost always multi-step workflows, so that is one
    ...
    +   *   different $form_id values to the proper form constructor function. Examples
    ...
    +   *   #action properties in Ajax callbacks and similar partial form rebuilds. The
    ...
    +   *   incoming $_POST information from a user's form submission. If a key is not
    ...
    +   *   browsers submit by not having a $_POST entry, include the key, but set the
    ...
    +   *   should not be included here, but rather placed directly in the $form_state
    ...
    +   * This function is the heart of form API. The form gets built, validated and in
    ...
    +   *   An associative array containing the structure of the form, which is passed
    +   *   by reference. Form validation handlers are able to alter the form structure
    +   *   (like #process and #after_build callbacks during form building) in case of
    ...
    +   * Usually (for exceptions, see below) $form_state['redirect'] determines where
    ...
    +   * redirect to), or an array of arguments for url(). If $form_state['redirect']
    +   * is missing, the user is usually (again, see below for exceptions) redirected
    ...
    +   *   never be set or altered by form builder functions or form validation/submit
    ...
    +   *   value of $form_state['redirect'] if it is set, or the current path if it is
    +   *   not. RedirectResponse preferentially uses the value of $_GET['destination']
    ...
    +   * indicate which element needs to be changed and provide an error message. This
    ...
    +   * re-display the form to the user with the corresponding elements rendered with
    ...
    +   * "Previous" button in a multistep form should not fire validation errors just
    ...
    +   * (may be set to an empty array). Any #submit handlers will be executed even if
    ...
    +   * Example 1: Allow the "Previous" button to function, regardless of whether any
    ...
    +   * Form errors higher up in the form structure override deeper errors as well as
    ...
    +   * two are self::doValidateForm() (invoked via self::validateForm() and used to
    +   * invoke validation logic for each element) and drupal_render() (for rendering
    ...
    +   *   type, one of the functions in this array is form_process_datetime(), which adds
    ...
    +   *   adds the attributes and JavaScript needed to make the details work in older
    +   *   browsers. The #process functions are called in preorder traversal, meaning
    ...
    +   * self::doValidateForm() and drupal_render(), appropriate for those operations.
    ...
    +   * logic based on the current value. However, all of self::doBuildForm() runs before
    ...
    +   * As a security measure, user input is used for an element's #value only if the
    +   * element exists within $form, is not disabled (as per the #disabled property),
    +   * and can be accessed (as per the #access property, except that forms submitted
    ...
    +   * Because of the preorder traversal, where #process functions of an element run
    ...
    +   * - Where user input from the current submission must affect the structure of a
    ...
    +   *   This is most commonly implemented with a submit handler setting persistent
    ...
    +   *   $form_state['values'] and setting $form_state['rebuild']. The form building
    ...
    +   *   within functions that run during the rendering phase (#pre_render, #theme,
    ...
    +   *   The form element that should have its value updated; in most cases you can
    +   *   just pass in the element from the $form array, although the only component
    ...
    +   *   if you want to update the value of $form['elem1']['elem2'], which should be
    

    80 chars.

Thanks for the awesome work. For fixing all the doc issues. I don't think there are anymore doc issues. The patch is green with a lot of unit test coverage and I think it is good to go. I haven't reviewed unit test part of the patch. RTBC for me but let others decide it. I think @tim.plunkett should be author of this commit. :). I will add the followups in the morning.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -1491,19 +1497,19 @@ protected function handleInputElement($form_id, &$element, &$form_state) {
+        // self::submitForm(), so we do not modify $form_state['input'] for them.

80 chars. If you have to ever reroll the patch.

I've reviewed the manual interdiff from #31 and the incidental parts of #33. Since the rest should be just copy/paste I didn't look into it too deeply. This looks like a big step forward, as we can now inject the form builder cleanly, unit test more things, save some code weight on non-form-using requests... All good stuff.

timplunkett++

I couldn't find anything functionally wrong with the patch. Good work, Tim, and thanks! Below are some minor comment issues and some code style nitpicking.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -0,0 +1,1778 @@
   +      //   appended via += $this->getFormStateDefaults().
   

   This reads a little awkwardly, plus the assumption makes me doubt it.

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -0,0 +1,1778 @@
   +        // An unchecked checkbox has a #value of integer 0, different than string
   

   80 characters.

3. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -0,0 +1,1778 @@
   +        // errors are suppressed. For example, a "Previous" button might want its
   +        // submit action to be triggered even if none of the submitted values are
   

   80 characters.

4. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -0,0 +1,1778 @@
   +        // self::submitForm(), so we do not modify $form_state['input'] for them.
   

   80 characters.

5. +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
   @@ -0,0 +1,763 @@
   +   * This function should be used instead of drupal_build_form() when $form_state
   

   80 characters.

6. +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
   @@ -0,0 +1,763 @@
   +   *   reference so that the caller can use it to examine what in the form changed
   

   80 characters, and many more for FormBuilderInterface::buildForm().

7. +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
   @@ -0,0 +1,763 @@
   +   * finished. If a validate or submit handler set $form_state['rebuild'] to TRUE,
   

   80 characters, and many more for FormBuilderInterface::rebuildForm().

8. +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
   @@ -0,0 +1,763 @@
   +   * This function is the heart of form API. The form gets built, validated and in
   

   80 characters.

9. +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
   @@ -0,0 +1,763 @@
   +   *   An associative array containing the structure of the form, which is passed
   

   80 characters, and many more for FormBuilderInterface::validateForm().

10. +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
    @@ -0,0 +1,763 @@
    +   * Usually (for exceptions, see below) $form_state['redirect'] determines where
    

    80 characters, and many more for FormBuilderInterface::redirectForm().

11. +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
    @@ -0,0 +1,763 @@
    +   * When a validation error is detected, the validator calls form_set_error() to
    

    80 characters, and many more for FormBuilderInterface::setErrorByName().

12. +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
    @@ -0,0 +1,763 @@
    +   * The standard form_set_error() behavior can be changed if a button provides
    

    form_set_error() should be \Drupal::FormBuilder()->setErrorByName().

13. +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
    @@ -0,0 +1,763 @@
    +   * Form errors higher up in the form structure override deeper errors as well as
    

    80 characters.

14. +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
    @@ -0,0 +1,763 @@
    +   * two are self::doValidateForm() (invoked via self::validateForm() and used to
    

    80 characters, and many more for FormBuilderInterface::doBuildForm().

15. +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
    @@ -0,0 +1,763 @@
    +   *   The form element that should have its value updated; in most cases you can
    

    80 characters, and many more for FormBuilderInterface::setValue().

16. +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
    @@ -0,0 +1,651 @@
    +      array(array('redirect' => array('bar', array('query' => array('foo' => 'baz')))), 'bar'),
    

    Not very readable with all these associative arrays on one line.

Storm should really have a feature to rewrap the comments as we like them to be.

@catch and I have discussed this and decided that it makes sense to do this refactoring as it is consistent with the direction of Drupal core and the procedural wrappers have not been removed.

Updated issue summary.

Updated issue summary.

Committed 82eb292 and pushed to 8.x. Thanks!

If you have php configured for strict errors you need the following fix - applied on commit.

--- a/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
+++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
@@ -644,8 +644,9 @@ function test_form_id() {
     define('WATCHDOG_ERROR', 3);
   }
   if (!function_exists('batch_get')) {
-    function batch_get() {
-      return array();
+    function &batch_get() {
+      $batch = array();
+      return $batch;
     }
   }
 }

Updated issue summary.

Created #2120863: Add docs to core/lib/Drupal/Core/Form/FormBuilder.php. Didn't add #5 in #32 because it would change code flow.

Added some basic one: https://drupal.org/node/2121003

My Dreditor died on the huge patch, so not providing the actual code lines here, but inside FormBuilder $this->csrfToken and $this->httpKernel get called unconditionally even though they are optional in the constructor. Can someone explain that, it seems wrong? Thanks!

Updated issue summary.

Change notice makes sense to me.

added issue

I checked, the follow-ups are open. so removing the needs follow up tag. I updated the issue summary.
and https://drupal.org/contributor-tasks/draft-change-notification says after removing the needs change notification, to also change status to fixed. doing that.

If this needs to stay open for something else, please list the next steps.

Ahh, thanks for #48, that makes sense. A code comment would've been nice, but yeah...

matching followups and noting they are all opened.