Immediately after install with the current 8.x from Git, you see this on the "Comment from display" form (path = admin/structure/types/manage/article/comment/form-display):

Selection_110.png

But this is how the form looks when rendered on a node page:

Selection_109.png

The order is the opposite of what it should be!

I couldn't find another issue in the queue for this already, but if it's a duplicate, please let me know.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plopesc’s picture

Hello,

I've been seeking this issue and I found that the problem comes when the Comment form Display form is not edited/saved.

Once the form has been saved, form is displayed OK.

I've been debugging the form mode process, but with no success. If somebody could point us how we can debug it, it would be appreciated.

Regards

plopesc’s picture

Title: comment fields shown in the wrong order (reversed) on form » Entity form display entities does not store base fields by default
FileSize
32.5 KB
34 KB

After come research, I found that the problem is that the entity form display does not include by default the Basefields included in the entity.

This is an example for a comment form display:

id: comment.comment_node_test.default
uuid: c9202370-7632-4a72-9100-fb8701c8825e
targetEntityType: comment
bundle: comment_node_test
mode: default
content:
  comment_body:
    type: text_textarea
    weight: '0'
    settings:
      rows: '5'
      placeholder: ''

And the comment form is displayed as below:
comment-before.png

Once the form is saved, this config entity stores the Base fields successfully:

id: comment.comment_node_test.default
uuid: c9202370-7632-4a72-9100-fb8701c8825e
targetEntityType: comment
bundle: comment_node_test
mode: default
content:
  comment_body:
    type: text_textarea
    weight: '0'
    settings:
      rows: '5'
      placeholder: ''
  author:
    weight: '-2'
    visible: '1'
  subject:
    weight: '-1'
    visible: '1'

And the comment form is displayed as below:
comment-after.png

This behavior exists also in other entity types, as nodes, but in that case, the title field is displayed in first place in the form.

Regards.

pcambra’s picture

Title: Entity form display entities does not store base fields by default » Entity form display entities doesn't store fields on creation in the yaml files

Digging further in this issue, it seems that both form modes and view modes (displayes) are not storing the fields automatically, for example the body field is created manually with set component on nodes:

    // Assign widget settings for the 'default' form mode.
    entity_get_form_display('node', $type->type, 'default')
      ->setComponent($field->id(), array(
        'type' => 'text_textarea_with_summary',
      ))
      ->save();

But the "base fields" are not, causing the issues described above, somehow, the display order is working but the form modes are not.

amateescu’s picture

I just want to note that the "base fields" that @pcambra refers to above, are actually treated as "extra fields" by the EntityFormDisplay entity. So just like in D7, for now.

I can take a look at this bug this weekend or sometime next week if no one gets to it before that :)

plopesc’s picture

Status: Active » Needs review
FileSize
1.14 KB

Hello

I think the bug is due to extra fields are not included in entity form config entity until the form is saved manually.
Then I used hook_entity_bundle_create() to include all the extra fields in the config entity.

Now, the config entity includes always the extra fields and the weights are taken into account.

First approach patch, just proof of concept.

Regards

Status: Needs review » Needs work

The last submitted patch, bundle_create_extra_fields-2044683-5.patch, failed testing.

plopesc’s picture

Status: Needs work » Needs review
FileSize
1.62 KB
1.33 KB

Ups, I didn't manage the visible option in hook_field_extra_fields() implementations.

Second round.

amateescu’s picture

Status: Needs review » Needs work

I don't think that's the correct fix for this bug because we know that 'display' extra fields work correctly, so it has to be something specific to 'form' extra fields. My guess is that \Drupal\field\FieldInfo::prepareExtraFields() is the culprit here.

plopesc’s picture

Hello

I've been reviewing \Drupal\field\FieldInfo::prepareExtraFields() and it looks fine for me...
After more digging, I think that the problem could be in Drupal\Core\Entity\EntityFormController::processForm(). This method calls $this->getFormDisplay($form_state)->getComponents() to set components weights, but extra fields aren't returned by this method because are not present in the form display entity. This leds to extra fields displayed using weights defined in CommentFormController (For comment example, 10 for author and empty for subject) instead of weights defined in hook_field_extra_fields() (-2 for author and -1 for subject).

Now, we could follow some different paths, I think:

  • Change #weight values in CommentFormController to be the same as defined in hook_field_extra_fields(). I think that it is not a good option, because it is unstable IMHO
  • Add extra fields to form display config entity by default, as in patch proposed in #7, maybe in a better way. This change should avoid the getComponents() problem because extra field will be always defined in the form display.
  • Make Drupal\entity\EntityDisplayBase::getComponents() smarter to take into account also extra fields, like Drupal\entity\EntityDisplayBase::getComponent()
  • Add some logic in Drupal\Core\Entity\EntityFormController::processForm() to not only hide extra fields, adding support for displaying extra fields not included in Drupal\entity\EntityDisplayBase::getComponents()
  • Any other better option you can propose ;)

What do you think?
Regards

amateescu’s picture

Status: Needs work » Needs review
FileSize
2.83 KB

@plopesc, I'll take your point C) D) and my comment in #8, because \Drupal\field\FieldInfo::prepareExtraFields() is totally useless at the moment :)

plopesc’s picture

FileSize
1.29 KB
3.54 KB

As commented with @amateescu on IRC, processForm() should check if the extrafield component weight has been already set in order to avoid unexpected overrides of #weight value.

Checking it, I also found that CommentFormController hardcodes #weight value for author item, which can't be overriden.

Removed the #weight property, given that it is never used since form display are taking care of extra fields weight.

Regards.

amateescu’s picture

Title: Entity form display entities doesn't store fields on creation in the yaml files » EntityFromController doesn't assign weights for extra fields that are not yet saved in a EntityFormDisplay

Yep, the patch in #11 is correct. But now I can't rtbc because I wrote the initial bits..

yched’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/EntityFormController.phpundefined
@@ -212,12 +212,15 @@ public function processForm($element, $form_state, $form) {
+      elseif (!isset($element[$extra_field]['#weight'])) {
+        $element[$extra_field]['#weight'] = $info['weight'];

I don't think this should be "assign the weight configured in the FormDisplay only if the $element doesn't contain a hardcoded #weight already" ?
Exposing your stuff as an "extra field" means you leave its ordering up to the "Manage (form) display" UI. If a component is reorderable in those UIs but in fact has a fixed order, it's a bug.
At least that's how it happens on the "view" side (see "Assign the weights configured in the display" in EntityRenderController)

Also, the weight should be the one fetched with getComponent(), not the one from $info / field_info_extra_fields(), this one is not the one "configured in the UI".

+++ b/core/modules/field/lib/Drupal/field/FieldInfo.phpundefined
@@ -543,7 +543,7 @@ public function getBundleExtraFields($entity_type, $bundle) {
     // Merge in saved settings.
     if (isset($extra[$entity_type][$bundle])) {
-      $info = $this->prepareExtraFields($extra[$entity_type][$bundle], $entity_type, $bundle);
+      $info = $extra[$entity_type][$bundle] + array('form' => array(), 'display' => array());

Yay. But the "Merge in saved settings" comment is now irrelevant :-)
Actually, the if(isset()) is irrelevant, we should always make sure $info is an array with at least the two empty 'form' and 'display' entries.

amateescu’s picture

Status: Needs work » Needs review
FileSize
1019 bytes
3.83 KB

Also, the weight should be the one fetched with getComponent()...

That's already the case a few lines above in that method, this is just a fallback behavior for extra fields that are not yet saved in a EntityFormDisplay entity (i.e. the default state of Display and FormDisplay config entities after an installation).

For the second point, something like this perhaps?

plopesc’s picture

FileSize
1.6 KB
3.89 KB

Hello

I think that if (isset()) is necessary because if it doesn't exist, you can find this error (Error: Unsupported operand types in /home/plopesc/projects/drupal8/core/modules/field/lib/Drupal/field/FieldInfo.php, line 544). You can reproduce it clearing cache.

@yched: now that you mention it, we can have problems with this if $element has already a '#weight' value in its definition. Then you can't override it only checking if #weight is set.

New patch adding if (isset()) again and overriding always #weight value for extra fields.

Regards.

amateescu’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
@@ -189,11 +189,12 @@ public function processForm($element, $form_state, $form) {
+      $component = $this->getFormDisplay($form_state)->getComponent($extra_field);
+      if (!$component) {
         $element[$extra_field]['#access'] = FALSE;
       }

This means that an extra field that's not yet saved in the FormDisplay will not have any weight specified, which is what we're trying to fix here?

plopesc’s picture

FileSize
602 bytes
3.89 KB

Yep, I think it will have its weight defined in hook_field_extra_fields() because as commented in #9, getComponent() returns this weight.

@@ -212,12 +212,15 @@ public function processForm($element, $form_state, $form) {
+      elseif (!isset($element[$extra_field]['#weight'])) {
+        $element[$extra_field]['#weight'] = $info['weight'];
+      }

If weight has been hardcoded in $element definition, you can't override it with the hook_field_extra_fields() value.
With patch in #15:

@@ -186,12 +186,16 @@ public function processForm($element, $form_state, $form) {
+      $component = $this->getFormDisplay($form_state)->getComponent($extra_field);
+      if (!$component) {
         $element[$extra_field]['#access'] = FALSE;
       }
+      else {
+        $element[$extra_field]['#weight'] = $component['weight'];
+      }

It uses always the weight defined in getComponent(), even if it has been harcoded in the $element definition.

However my last patch had a problem with the definition of $info in

@@ -543,7 +543,7 @@ public function getBundleExtraFields($entity_type, $bundle) {
+      $info += $extra[$entity_type][$bundle];

$info wasn't assigned right, fixed here.

Regards.

yched’s picture

[edit: sorry, crosspost with #17]

(yched) Also, the weight should be the one fetched with getComponent()...
(amateescu) That's already the case a few lines above in that method, this is just a fallback behavior for extra fields that are not yet saved in a EntityFormDisplay entity (i.e. the default state of Display and FormDisplay config entities after an installation).

Ah, right, sorry, I didn't look high enough in the method.
However, taking into account the "default weight" specified in the "extra field definition" is already be taken care of in EntityDisplayBase::getComponent(), so assigning the weight that comes out of getComponent() should be good enough.

This means that an extra field that's not yet saved in the FormDisplay will not have any weight specified, which is what we're trying to fix here?

An extra field that's not yet saved in the FormDisplay should come out of getComponent() with the default weight specified in its hook_field_extra_fields() entry, that's what EntityDisplayBase::getComponent() does, if I read it correctly.

@plopesc #15: $info += $extra[$entity_type][$bundle]; cannot work, that's the wrong order of merging, the 'empty' values will always win.
I think this code in getBundleExtraFields() would be simpler with just:

$info = isset($extra[$entity_type][$bundle]) ? $extra[$entity_type][$bundle] : array();
$info += array('form' => array(), 'display' => array());
plopesc’s picture

FileSize
1.06 KB
3.99 KB

Patch including yched suggestion in getBundleExtraFields().
Regards.

Status: Needs review » Needs work

The last submitted patch, 2044863-19.patch, failed testing.

plopesc’s picture

Status: Needs work » Needs review

#19: 2044863-19.patch queued for re-testing.

swentel’s picture

Status: Needs review » Closed (duplicate)

Marking duplicate of #1953528: Store 'per-bundle view mode settings' in CMI, get rid of field_bundle_settings() & its variable - we're fixing it there at the same time, will add you guys to the suggested commit message.