Config entity forms need to have access to an updated entity object at all times

Great patch! Mostly nits:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -50,6 +50,11 @@ public static function create(ContainerInterface $container) {
   +    // Content entity forms only need to rebuild the entity in the validation
   +    // and the submit handler.
   +    unset($form['#after_build']);
   

   Why? Can't some widget also be doing AJAXy stuff?

2. +++ b/core/lib/Drupal/Core/Entity/EntityForm.php
   @@ -131,13 +131,13 @@ protected function init(FormStateInterface $form_state) {
   +    // Add process and after_build callbacks.
   

   s/process/#process/
   s/after_build/#after_build/

3. +++ b/core/lib/Drupal/Core/Entity/EntityForm.php
   @@ -131,13 +131,13 @@ protected function init(FormStateInterface $form_state) {
   +    $form['#after_build'][] = '::afterBuild';
   

   Oh my! #after_build! Been a long time since we saw that! :D

4. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -801,8 +801,8 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
   -      foreach ($element['#after_build'] as $callable) {
   -        $element = call_user_func_array($callable, array($element, &$form_state));
   +      foreach ($element['#after_build'] as $callback) {
   +        $element = call_user_func_array($form_state->prepareCallback($callback), array($element, &$form_state));
   

   Woah, nice catch! This should also update FormBuilderTest to prevent this regression in the future.

5. +++ b/core/modules/config/src/Tests/ConfigEntityTest.php
   @@ -319,6 +319,44 @@ function testCRUDUI() {
   +    $edit = array(
   ...
   +    $edit += array('size_value' => 'medium');
   

   array() -> []

6. +++ b/core/modules/config/src/Tests/ConfigEntityTest.php
   @@ -319,6 +319,44 @@ function testCRUDUI() {
   +    // Test the same scenario but it in a non-js case by using a js-hidden
   

   s/non-js/non-JS/
   s/js-hidden/'js-hidden'/

   This makes it clear that in the first case, you're describing a situation/context, and in the second, you're describing an element class.

7. +++ b/core/modules/config/src/Tests/ConfigEntityTest.php
   @@ -319,6 +319,44 @@ function testCRUDUI() {
   +    $entity = entity_load('config_test', $id);
   

   ConfigTest::load()

8. +++ b/core/modules/config/tests/config_test/src/ConfigTestForm.php
   @@ -68,6 +112,20 @@ public function form(array $form, FormStateInterface $form_state) {
   +   * Element submit handler for non-js testing.
   

   s/non-js/non-JS/

9. +++ b/core/modules/editor/editor.module
   @@ -92,7 +92,8 @@ function editor_form_filter_admin_overview_alter(&$form, FormStateInterface $for
   -    $format_id = $form_state->getFormObject()->getEntity()->id();
   +    $format = $form_state->getFormObject()->getEntity();
   +    $format_id = $format->isNew() ? NULL : $format->id();
   
   @@ -175,8 +176,9 @@ function editor_form_filter_admin_format_editor_configure($form, FormStateInterf
   -        'format' => $form_state->getFormObject()->getEntity()->id(),
   +        'format' => $format->isNew() ? NULL : $format->id(),
   
   @@ -215,7 +217,8 @@ function editor_form_filter_admin_format_validate($form, FormStateInterface $for
   +  $format_id = $format->isNew() ? NULL : $format->id();
   

   Yay, better :) Thanks!

1. Ah! That really helps — thanks :)
4. Hm, fair — that's probably okay.
7. I wasn't sure, but I suspected that that was the reason why. I ran into that too a long time ago. Fine then :)

Patch looks great, just clarification nitpicks :

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -50,6 +50,12 @@ public static function create(ContainerInterface $container) {
        $form = parent::form($form, $form_state);
   +
   +    // Content entity forms only need to rebuild the entity in the validation
   +    // and the submit handler because Field API uses its own #after_build
   +    // callback for its widgets.
   +    unset($form['#after_build']);
   

   Looks a bit surprising when seen on its own. Would help seeing the big picture if the comment made it clearer that we're undoing part of what the parent did. Also maybe by removing the empty line and grouping the code more ?

2. +++ b/core/lib/Drupal/Core/Entity/EntityForm.php
   @@ -156,6 +156,22 @@ public function processForm($element, FormStateInterface $form_state, $form) {
      /**
   +   * Form element #after_build callback: Updates the entity with submitted data.
   +   *
   +   * This is the entity object builder function that allows rebuilt forms (e.g.
   +   * submitted via AJAX) to keep relying on entity values instead of form state.
   +   */
   +  public function afterBuild(array $element, FormStateInterface $form_state) {
   +    // If the form is processing user input, rebuild the entity with the current
   +    // form values.
   +    if ($form_state->isProcessingInput()) {
   +      $this->entity = $this->buildEntity($element, $form_state);
   +    }
   +
   +    return $element;
   +  }
   

   Comments could be enhanced a bit IMO. Proposals below are what I feel would have helped me grasp the logic faster :-)

   - Phpdoc : "This is the entity object builder function that allows..." : slightly confusing, an #after_build callback is not an entity builder function.
   Proposal :
   "Updates the internal $this->entity object with submitted values when the form is being rebuilt (e.g. submitted via AJAX), so that subsequent processing (e.g. AJAX callbacks) can rely on it." ?

   - Inner code comment : the logic around "detect if we're doing the initial build or a rebuild" could be more explicit.
   Proposal : "Rebuild the entity If #after_build" is being called as part of a form rebuild, i.e. if we are processing input.".

Needs work for #20

Thanks ! RTBC +1

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 1c72fda and pushed to 8.0.x. Thanks!

Trying to simplify the Ajax flow in Field UI "Manage Display" forms in #2497847: Simplify EntityDisplayEditFormBase ajax rebuild flow to work only with $this->entity, it does seem we call buildEntity() quite a lot.
Opened #2497981: EntityForms call buildEntity() a lot on AJAX calls to see what happens if we only rebuild when submitting values.