Problem/Motivation

The main premise of entity forms is that we get to work with an entity object at all times instead of checking submitted values from the form state. However, this premise is currently broken in the main form() method when the form is rebuilt via AJAX.

Proposed resolution

Add an #after_build callback that updates the entity object with user-submitted values from the form state.

Remaining tasks

Review the patch.

User interface changes

Nope.

API changes

Nope.

Files: 
CommentFileSizeAuthor
#22 interdiff.txt2.21 KBamateescu
#22 2448357-22.patch12.21 KBamateescu
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,793 pass(es). View
#18 interdiff.txt3.31 KBamateescu
#18 2448357-18.patch12.08 KBamateescu
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,767 pass(es). View
#15 interdiff.txt1.87 KBamateescu
#15 2448357-15.patch12.01 KBamateescu
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,741 pass(es). View
#13 2448357-13.patch10.14 KBamateescu
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,014 pass(es), 5 fail(s), and 4 exception(s). View
#10 2448357-10-test-only.patch6.74 KBamateescu
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,009 pass(es), 6 fail(s), and 0 exception(s). View

Comments

amateescu’s picture

Status: Active » Needs review
FileSize
4.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,994 pass(es), 11 fail(s), and 0 exception(s). View
9.99 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,936 pass(es), 1,250 fail(s), and 535 exception(s). View

Here's the patch.

The last submitted patch, 1: 2448357-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 1: 2448357.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
6.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,006 pass(es), 5 fail(s), and 0 exception(s). View
1.76 KB

This fixes the test-only patch so it shows only the relevant failures. Now let's see why I broke all of core :/

Status: Needs review » Needs work

The last submitted patch, 4: 2448357-4-test-only.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
12.69 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,616 pass(es), 1,150 fail(s), and 505 exception(s). View
963 bytes

I seem to be finding all kinds of subtle broken things this weekend.

Test-only patch from #4 is still accurate for review.

Status: Needs review » Needs work

The last submitted patch, 6: 2448357-6.patch, failed testing.

amateescu’s picture

Title: Entity forms need to have access to an updated entity object at all times » Config entity forms need to have access to an updated entity object at all times
Status: Needs work » Needs review
FileSize
12.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,553 pass(es), 658 fail(s), and 146 exception(s). View
2.76 KB

It seems that content entity forms really need the current flow, probably because field api has its own #after_build callback for widgets, so let's make it specific to them. This allows us to fix config entity forms which use only raw Form API code.

Status: Needs review » Needs work

The last submitted patch, 8: 2448357-8.patch, failed testing.

amateescu’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
6.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,009 pass(es), 6 fail(s), and 0 exception(s). View
9.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,751 pass(es), 194 fail(s), and 85 exception(s). View

I need to stop thinking that I solved all the problems in the world every time I do small change accompanied by a victorious comment :)

Let's just fix what this issue was about: we need to be able to access updated $entity values in the form() method when it is rebuilt after an AJAX submit.

No interdiff because I'm just reverting the crap from #6 and #8.

Status: Needs review » Needs work

The last submitted patch, 10: 2448357-10.patch, failed testing.

The last submitted patch, 10: 2448357-10-test-only.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
10.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,014 pass(es), 5 fail(s), and 4 exception(s). View
861 bytes

#sigh

Status: Needs review » Needs work

The last submitted patch, 13: 2448357-13.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
12.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,741 pass(es). View
1.87 KB

CKEditor was relying on the id() method to return NULL for config entities that are not yet saved, but that assumption is not correct.

Wim Leers’s picture

Wim Leers’s picture

Status: Needs review » Needs work

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!

amateescu’s picture

Status: Needs work » Needs review
FileSize
12.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,767 pass(es). View
3.31 KB

Thanks for the review :)

  1. I expanded the comment to better explain why this is not needed for content entity forms.
  2. Fixed.
  3. Yeah, took me a while to figure out that's the best insertion point :)
  4. That's a unit test and it would be quite painful to mock everything needed, but this is now tested by ConfigEntityTest through ConfigTestForm.
  5. Fixed.
  6. Fixed.
  7. I did that initially but the magic entity class loader doesn't work for the ConfigTest entity type, probably because it's extended by other test entity classes.
  8. Fixed.
  9. Agreed :)
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

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 :)

yched’s picture

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.".

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs work for #20

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
12.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,793 pass(es). View
2.21 KB

Updated the comments according to the suggestions from #20, I hope it's ok to put it back to RTBC after these documentation changes :)

yched’s picture

Thanks ! RTBC +1

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • alexpott committed 1c72fda on 8.0.x
    Issue #2448357 by amateescu: Config entity forms need to have access to...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

yched’s picture

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.