Problem/Motivation

Previously discussed in #3534619: Support for legacy preprocesses but the two subjects are distinct because extra fields are not rendered by preprocess hooks but by ViewBuilder handlers or view hooks.

We have already an ExtraFieldSource but ::getPropValue() is rendering a placeholder instead of the expected renderable.

Proposed resolution

Extra fields renderables can be found in:

  • NodeViewBuilder::buildComponents() for links extra field
  • CommentViewBuilder::buildComponents() for links extra field
  • UserHooks::userView() (hook_ENTITY_TYPE_view implementation) for member_for extra field

hook_ENTITY_TYPE_view is triggered from EntityViewBuilder::buildMultiple() which is executed by EntityViewBuilder::buildMultiple(), so let's keep a single logic.

CommentFileSizeAuthor
#26 display_builder-3571038-extra-field-support-26.patch15.22 KByannickoo

Issue fork display_builder-3571038

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

pdureau created an issue. See original summary.

pdureau opened merge request !220

pdureau’s picture

pdureau commented
Status: Active » Needs work

A first commit has been pushed, with some WIP proposal.

When the Entity ID is not known (for example, when working in entity view display), we are still rendering the placeholder.

When the Entity ID is known, we are executing EntityViewBuilderInterface::buildComponents() and extracting the built extra field renderable.

Nearly done, however:

  • if we set the current view mode in EntityViewBuilderInterface::buildComponents() last parameter, we are stuck in an infinite loop: the entity display is rendered, which is rendering the extra field, which is executing entity display rendering, which is rendering the extra field...
  • if we set a fake view mode, the extra field may not be rendered if it uses the current view mode in its logic. This may be the case for node's links for example.

Also, user's member_for has not been tested yet.

yannickoo’s picture

yannickoo
Location Berlin
commented

FYI it is still possible to use hook_entity_view_alter to implement the rendering part for extra fields. It seems like it is possible to add new elements to the $build render array using that way.

pdureau’s picture

pdureau commented
Assigned: Unassigned » pdureau

I take it for rebase and to test Yannick proposal

pdureau’s picture

pdureau commented
Assigned: pdureau » Unassigned
Issue tags: -display_builder-1.0.0-beta3 +display_builder-1.0.0-beta

It is possible to add new elements to the $build render array using that way.

Tested. hook_entity_view_alter is triggered not at the best moment of the rendering pipeline:

  • if the extra field is at root level, it is already built as a display_builder:placeholder component by ExtraFieldSource::getPropValue(). We can add some annotations in the renderable to identify it in the source tree and replace it by the renderable available in hook_entity_view_alter.
  • if the extra field is nested in a component, the source tree inside this component has not been resolved yet so the component slots are not a renderable tree yet, and I don't know how to replace the value.

yannickoo opened merge request !230

yannickoo’s picture

yannickoo
Location Berlin
commented
Status: Needs work » Needs review

@pdureau since I was not able to add commits to your MR I have created a new one which is adjusting the extra field source plugin implementation ➡️ https://git.drupalcode.org/project/display_builder/-/merge_requests/230

pdureau changed the visibility of the branch 3571038-render-extra-field to hidden.

pdureau’s picture

pdureau commented
Issue tags: -display_builder-1.0.0-beta +display_builder-1.0.0-beta3

OK cool. I hide my branch and move back the issue to beta3 scope.

pdureau’s picture

pdureau commented
Status: Needs review » Needs work

Hi Yannick,

I got a look on your proposal. I like a lot your are staying in modules/display_builder_entity_view/src/Plugin/UiPatterns/Source/ExtraFieldSource.php without spreading out the logic elsewhere.

However, it doesn't work on my side. $this->moduleHandler->invokeAll('entity_view', [&$build,$entity, $display, $view_mode ]); is returning a (nearly) empty render array, without the expected extra field.

Also:

  • honest question, are we sure we want to unhide extra fields in src/Plugin/display_builder/Island/BlockLibraryPanel.php?
  • "Imported but not supported" in ::settingsForm() can be removed
mogtofu33’s picture

mogtofu33 commented
Issue tags: -display_builder-1.0.0-beta3 +display_builder-1.0.0-beta4
yannickoo’s picture

yannickoo
Location Berlin
commented

The current problem is that we can only get rendered extra fields that are provided via hook_entity_view. The node and user module are doing this in their view builders which is hard to catch since we have this infinite loop when trying to just use buildComponent.

Any contrib module works without issues but the core extra fields are not provided like that :/

fmb’s picture

fmb commented

I like the way this is presented in Display Builder.

In order to use this patch, make sure that:

  1. You are implementing (directly or indirectly through a module like Extra Field) hook_entity_view(). hook_ENTITY_TYPE_view() will not work, even though some tutorials suggest to implement the latter.
  2. You do not have a condition on $display->getComponent('my_own_pseudo_field'), which does not work. I suppose this is because $display is a different beast from a Display Builder display.

Should we allow the use of hook_ENTITY_TYPE_view()? What about the condition on $display->getComponent() ?

yannickoo’s picture

yannickoo
Location Berlin
commented

I agree with point #1 that we should allow that hook as well!

mogtofu33’s picture

mogtofu33 commented
Assigned: Unassigned » mogtofu33
mogtofu33’s picture

mogtofu33 commented
Assigned: mogtofu33 » pdureau
Status: Needs work » Needs review
mogtofu33’s picture

mogtofu33 commented
Assigned: pdureau » mogtofu33
Status: Needs review » Needs work
yannickoo’s picture

yannickoo
Location Berlin
commented

Hey Jean! It turned out that we had an infinite call which could be avoided in this commit. This was breaking all projects but is fixed with the latest changes.

pdureau’s picture

pdureau commented

Discussed with Jean... (a few weeks ago, so from memory...)

There are two kinds of extra fields...

Those hardcoded in view builders entity handlers:

  • NodeViewBuilder::buildComponents() for links extra field
  • CommentViewBuilder::buildComponents() for links extra field

For those, let's also do a specific, hardcoded, mechanism on our side.

Those based on hook_ENTITY_TYPE_view:

  • UserHooks::userView() (hook_ENTITY_TYPE_view implementation) for member_for extra field
  • and many in contrib...

For those, let's manage theme with a clean, generic, solution.

mogtofu33’s picture

mogtofu33 commented

Tested with links (node and user) and contrib module Flag, addtoany, works fine!

mogtofu33’s picture

mogtofu33 commented
Assigned: mogtofu33 » pdureau
Status: Needs work » Needs review
pdureau’s picture

pdureau commented
Assigned: pdureau » mogtofu33
Status: Needs review » Needs work
Issue tags: -display_builder-1.0.0-beta4 +display_builder-1.0.0-beta5

Tests

Links extra field in node (hardcoded in view builder entity handler):

  • ❌ the extra field is not visible in the display, I have this renderable {"#type":"html_tag","#tag":"span","lazy":{"#lazy_builder":["Drupal\\node\\NodeViewBuilder::renderLinks",["1","default","en",false,null]]}}" but no links. Is it normal?
  • ⚠️ I believe the extra field selector in the source form must be mandatory. It is weird to move an extra field source to a display and not picking one.

"Member for" extra field in user (based on hook_ENTITY_TYPE_view):

  • ✅ it works :)

Test with an entity type without extra field (content block):

  • ⚠️ extra field source is available in block library, it can be dropped to the builder, but no extra field to pick in the selector

MR analysis

I agree with the removal of modules/display_builder_entity_view/src/Plugin/UiPatterns/Source/ContentEntitySource.php, we have added it too soon, we have dedicated ticket to address this feature and we need to #3540247: Tidy source contexts in UI Patterns first. I just hope it will not break the display of too much people.

metadata attribute, used in ExtraFieldSource, is an undocumented feature of UI Patterns we will try to remove soon (see #3591167: Tidy metadata plugin attributes).

Do we some dedicated kernel tests? Are the addition display_builder_entity_view_test.module enough?

mogtofu33’s picture

mogtofu33 commented
Assigned: mogtofu33 » pdureau
Status: Needs work » Needs review

ContentEntitySource was an old test, it was a mistake of mine to commit it. It was not possible to use it it seems because on context. I've never seen it in ui, that's why I forgot to delete it sooner.

The test is minimal, it was a bit tricky but I think it covers what's needed.

#lazy_builder is empty because it's lazy and will load on render, the render array is good, strange you have nothing... Keep in mind core links which is only 'read more' for node if no comments and is not in default/full but teaser. Enable comments to see if it works.

Empty extra field in ui will be a follow up as it's not blocking or failing anything.

pdureau’s picture

pdureau commented
Assigned: pdureau » mogtofu33

ContentEntitySource was an old test, it was a mistake of mine to commit it. It was not possible to use it it seems because on context. I've never seen it in ui, that's why I forgot to delete it sooner.

That's clear, and that's nice.

Keep in mind core links which is only 'read more' for node if no comments and is not in default/full but teaser.

Oohhh! that must be the explanation.

Empty extra field in ui will be a follow up as it's not blocking or failing anything.

OK, in a follow-up.

So, there are only 2 open topics to discuss before RTBC:

  • I believe the extra field selector in the source form must be mandatory. It is weird to move an extra field source to a display and not picking one.
  • metadata attribute, used in ExtraFieldSource, is an undocumented feature of UI Patterns we will try to remove soon (see #3591167: Tidy metadata plugin attributes) and I would advise not using it.
yannickoo’s picture

yannickoo
Location Berlin
commented
StatusFileSize
new display_builder-3571038-extra-field-support-26.patch15.22 KB

Just dropping a static patch to avoid having floating MR patch file 😇

mogtofu33’s picture

mogtofu33 commented
Assigned: mogtofu33 » Unassigned
Status: Needs review » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • mogtofu33 committed 85d21809 on 1.0.x authored by yannickoo 
    feat: #3571038 Render extra field values

By: pdureau
By: yannickoo
By:...

Status: Fixed » Closed (fixed)

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