Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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):
But this is how the form looks when rendered on a node page:
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.
Comment | File | Size | Author |
---|---|---|---|
#19 | 2044863-19.patch | 3.99 KB | plopesc |
#19 | interdiff.txt | 1.06 KB | plopesc |
#17 | 2044863-17.patch | 3.89 KB | plopesc |
#17 | interdiff.txt | 602 bytes | plopesc |
#15 | 2044863-15.patch | 3.89 KB | plopesc |
Comments
Comment #1
plopescHello,
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
Comment #2
plopescAfter 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:
And the comment form is displayed as below:
Once the form is saved, this config entity stores the Base fields successfully:
And the comment form is displayed as below:
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.
Comment #3
pcambraDigging 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:
But the "base fields" are not, causing the issues described above, somehow, the display order is working but the form modes are not.
Comment #4
amateescu CreditAttribution: amateescu commentedI 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 :)
Comment #5
plopescHello
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
Comment #7
plopescUps, I didn't manage the
visible
option inhook_field_extra_fields()
implementations.Second round.
Comment #8
amateescu CreditAttribution: amateescu commentedI 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.Comment #9
plopescHello
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 inCommentFormController
(For comment example, 10 for author and empty for subject) instead of weights defined inhook_field_extra_fields()
(-2 for author and -1 for subject).Now, we could follow some different paths, I think:
#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 IMHODrupal\entity\EntityDisplayBase::getComponents()
smarter to take into account also extra fields, likeDrupal\entity\EntityDisplayBase::getComponent()
Drupal\Core\Entity\EntityFormController::processForm()
to not only hide extra fields, adding support for displaying extra fields not included inDrupal\entity\EntityDisplayBase::getComponents()
What do you think?
Regards
Comment #10
amateescu CreditAttribution: amateescu commented@plopesc, I'll take your point
C)D) and my comment in #8, because\Drupal\field\FieldInfo::prepareExtraFields()
is totally useless at the moment :)Comment #11
plopescAs 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.
Comment #12
amateescu CreditAttribution: amateescu commentedYep, the patch in #11 is correct. But now I can't rtbc because I wrote the initial bits..
Comment #13
yched CreditAttribution: yched commentedI 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".
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.
Comment #14
amateescu CreditAttribution: amateescu commentedThat'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?
Comment #15
plopescHello
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.
Comment #16
amateescu CreditAttribution: amateescu commentedThis 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?
Comment #17
plopescYep, I think it will have its weight defined in hook_field_extra_fields() because as commented in #9, getComponent() returns this 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:
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
$info wasn't assigned right, fixed here.
Regards.
Comment #18
yched CreditAttribution: yched commented[edit: sorry, crosspost with #17]
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.
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:
Comment #19
plopescPatch including yched suggestion in getBundleExtraFields().
Regards.
Comment #21
plopesc#19: 2044863-19.patch queued for re-testing.
Comment #22
swentel CreditAttribution: swentel commentedMarking 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.