The field_view_field() / field_view_value() functions would make much more sense as view() methods on FieldItemList and FieldItemBase respectively (and added to the respective interfaces)

The phpdocs should make clear that, like the former functions, those methods are intended for one-off display of isolated field values in specific cases, but should not be used instead of entity_view().

Most arguments can now go away :
- $entity can be accessed by $this->getEntity()
- $field_name is $this->name,
- $item is $this
- $langcode is $this->getLangcode() (no need to specify the requested language as a param, the caller needs to set the entity to the expected language first, then calls view() on one of the fields / field items)

--> FieldItemListInterface::view($display), FieldItemInterface::view($display)

This could be a "relatively advanced" Novice task :-)

CommentFileSizeAuthor
#30 field_view_field-2134887-30.patch32.75 KBplopesc
#29 interdiff.txt2.91 KByched
#29 field_view_field-2134887-28.patch32.71 KByched
#26 interdiff.txt9.49 KByched
#26 field_view_field-2134887-26.patch32.74 KByched
#23 field_view_field-2134887-23.patch34.32 KBplopesc
#20 field_view_field-2134887-20.patch37.23 KBplopesc
#15 field_view_field-2134887-15.patch31.67 KBzwischenzug
#10 interdiff.txt838 byteszwischenzug
#10 field_view_field-2134887-10.patch30.08 KBzwischenzug
#9 interdiff.txt26.86 KBzwischenzug
#9 field_view_field-2134887-9.patch30.08 KBzwischenzug
#8 field_view_field-2134887-8.patch3.96 KBzwischenzug
#6 field_view_field-2134887-6.patch3.91 KBzwischenzug
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

yched CreditAttribution: yched commented
Issue summary: View changes
fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

I'm hesitant to adding the view logic of fields to those classes also - it's like letting the data module render itself. I'd prefer a separation here, as we do it for entities as a whole. I'd be fine with a shortcut method though.

So instead, it should go on the entity view builder imo. We do the same for access already - both entity+field is handled via the respective controller.

yched’s picture

yched CreditAttribution: yched commented

Putting the methods on EntityViewBuilder, with shortcut view() methods directly on FieldItem / FieldItemList: right, makes more sense indeed.

Then we'd need two methods on EntityViewBuilder though: 

viewFieldItemList(FieldItemListInterface $items, $display)
viewFieldItem(FieldItemInterface $item, $display) ?

LongNames-- ;-)
Or maybe viewField() / viewFieldItem() ?

yched’s picture

yched CreditAttribution: yched commented

Started working on this with a co-worker, he should post an initial patch shortly.

zwischenzug’s picture

zwischenzug CreditAttribution: zwischenzug commented
Assigned: Unassigned » zwischenzug
zwischenzug’s picture

zwischenzug CreditAttribution: zwischenzug commented
Status: Active » Needs review
FileSize
field_view_field-2134887-6.patch3.91 KB

Not finished yet ...
I worked with yched,

Remains to be done :
- Transfer the code of the two methods (FieldItemBase::view() and FieldItemList::view()) in 'EntityViewBuilder'.
- Replace existing calls to field_view_field() / field_view_value() and remove the functions
- Add PhpDoc.

Status: Needs review » Needs work

The last submitted patch, 6: field_view_field-2134887-6.patch, failed testing.

zwischenzug’s picture

zwischenzug CreditAttribution: zwischenzug commented
Status: Needs work » Needs review
FileSize
field_view_field-2134887-8.patch3.96 KB

Reroll

zwischenzug’s picture

zwischenzug CreditAttribution: zwischenzug commented
FileSize
field_view_field-2134887-9.patch30.08 KB
interdiff.txt26.86 KB

Completely replaced the old functions.
Still todo: move the actual implementations to EntityViewBuilder.

zwischenzug’s picture

zwischenzug CreditAttribution: zwischenzug commented
FileSize
field_view_field-2134887-10.patch30.08 KB
interdiff.txt838 bytes

There was a merge conflict.

The last submitted patch, 10: field_view_field-2134887-10.patch, failed testing.

The last submitted patch, 9: field_view_field-2134887-9.patch, failed testing.

yched’s picture

yched CreditAttribution: yched commented
Issue tags: +beta target

beta target, since this is blocking #2067079: Remove the Field Language API

yched’s picture

yched CreditAttribution: yched commented
Priority: Normal » Major

"beta target" -> bumping priority accordingly

zwischenzug’s picture

zwischenzug CreditAttribution: zwischenzug commented
FileSize
field_view_field-2134887-15.patch31.67 KB

- FieldItemList::view() implementations moved to EntityViewBuilder::viewFieldItemList().
- FieldItemBase::view() implementations moved to EntityViewBuilder::viewFieldItem().
- Completely replaced the old functions.
- field_view_field() and field_view_value() deleted.
Still todo :
- some phpdoc.

yched’s picture

yched CreditAttribution: yched commented

Will need a reroll after #2075185: When an entity is in-place edited (i.e. saved), other instances of that entity on the same page are not updated (no propagation), that modified the place where EditController calls field_view_field() - see http://drupalcode.org/project/drupal.git/commitdiff/ebad0cd9420c5d6e5166...

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented

15: field_view_field-2134887-15.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 15: field_view_field-2134887-15.patch, failed testing.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Priority: Major » Normal
Issue tags: -beta target

+1 to this issue, but #2067079: Remove the Field Language API landed without this, so reverting #13's and #14's issue attribute changes.

plopesc’s picture

plopesc
he/him
Primary language Spanish
CreditAttribution: plopesc commented
Status: Needs work » Needs review
FileSize
field_view_field-2134887-20.patch37.23 KB

Re-rolling patch.

Also adding methods to EntityViewBuilderInterface. This decision leds to duplicate code in EntityViewBuilder and BlockViewBuilder. Could be a good idea create a new abstract class implementing these methods and extending it both EntityViewBuilder and BlockViewBuilder to avoid duplicate code?

Regards.

yched’s picture

yched CreditAttribution: yched commented
Status: Needs review » Postponed

Thanks @plopesc.

#2167267: Remove deprecated field_attach_*_view() is probably going to change the content of field_view_field() quite a bit, so it might be wiser to wait for that one.
The discussion there could also impact where the replacement methods end up...

So, postponing...

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Postponed » Needs review

dependency commited

plopesc’s picture

plopesc
he/him
Primary language Spanish
CreditAttribution: plopesc commented
FileSize
field_view_field-2134887-23.patch34.32 KB

Quick re-roll of 20.

Let's see testbot...
Regards

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Priority: Normal » Major
Issue tags: +beta target

I'd actually still suggest we make this a beta target.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

23: field_view_field-2134887-23.patch queued for re-testing.

yched’s picture

yched CreditAttribution: yched commented
FileSize
field_view_field-2134887-26.patch32.74 KB
interdiff.txt9.49 KB

Thanks for the rerolls !

Updated patch:

  1. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -273,6 +275,75 @@ public function resetCache(array $entities = NULL) {
+    // Return nothing if the field doesn't exist.
+    if (!$entity->hasField($field_name)) {
+      return $output;
+    }

    That check is useless now that we receive a FieldItemList object instead of [an entity + a field name]. If that FieldItemList object exists, then we can reasonably assume that the field exists in the entity. 

  2. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -273,6 +275,75 @@ public function resetCache(array $entities = NULL) {
+    if ($entity->hasField($field_name)) {

    Same here. 

  3. +++ b/core/modules/block/lib/Drupal/block/BlockViewBuilder.php
@@ -99,4 +102,75 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
+  public function viewField(FieldItemListInterface $items, $display_options = array()) {
...
+  public function viewFieldItem(FieldItemInterface $item, $display_options = array()) {

    Hm - not sure why BlockViewBuilder needs to duplicate the implementations from EntityViewBuilder.
    Trying without it. 

  4. +++ b/core/modules/field/lib/Drupal/field/Plugin/views/field/Field.php
@@ -863,9 +864,10 @@ function field_langcode(EntityInterface $entity) {
+      // @todo This might not be needed anymore. Check with plach.
       $langcode = $this->entityManager->getTranslationFromContext($entity, $langcode)->language()->id;

    Looked into this. The rendering pipeline in \Drupal\field\Plugin\views\field\Field could use a serious cleanup, but for now clarified the multilingual logic a bit. 

  5. +++ b/core/modules/field/lib/Drupal/field/Tests/DisplayApiTest.php
@@ -190,16 +192,18 @@ function testFieldViewField() {
+      debug($this->content);
+      debug($setting . '|' . $value['value']);

    removed leftover debug()

Let's see what testbot says.

yched’s picture

yched CreditAttribution: yched commented

Green. This should be ready to go.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

The main confusion here is that new methods accept 2 different arguments, both optional
Maybe it make sense to check the usage and split them?

  1. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilderInterface.php
@@ -84,4 +87,66 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
+   * The function takes care of invoking the prepare_view steps. It also
+   * respects field access permissions.
...
+   * @param \Drupal\Core\Field\FieldItemListInterface $items
+   *   FieldItemList containing the values to be displayed.
+   * @param array $display_options
+   *  Can be either:
+   *   - The name of a view mode. The field will be displayed according to the
...
+   *   - An array of display options. The following key/value pairs are allowed:

    what is the reason for this confusion? 

  2. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilderInterface.php
@@ -84,4 +87,66 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
+   * @param array $display_options
+   *   Can be either the name of a view mode, or an array of display settings.
+   *   See EntityViewBuilderInterface::viewFieldItemList() for more information.

    same here?

yched’s picture

yched CreditAttribution: yched commented
FileSize
field_view_field-2134887-28.patch32.71 KB
interdiff.txt2.91 KB

Pointed by @swentel on IRC : some docs referred to an incorrect method name.

@andypost: This is how field_view_field() / field_view_value() work currently, and there are use cases for both.
Splitting that would mean 2 methods on FieldItemList, 2 methods on FieldItem, and 4 methods on EntityViewDisplay().
I'd rather leave it alone :-)

plopesc’s picture

plopesc
he/him
Primary language Spanish
CreditAttribution: plopesc commented
FileSize
field_view_field-2134887-30.patch32.75 KB

Re-rolling after #2002134: Move TypedData metadata introspection from data objects to definition objects

swentel’s picture

swentel CreditAttribution: swentel commented
Status: Needs review » Reviewed & tested by the community

Ok, let's do this.

yched’s picture

yched CreditAttribution: yched commented

Thanks for the reroll !
This will need a draft change notice before it gets committed. Can't write it before monday though :-/

swentel’s picture

swentel CreditAttribution: swentel commented

Just created the draft notice, but without any usefull body for now, hopefully that helps, will try and see if I can add more data tomorrow on the sprint.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

If we don't have a change record that's not a "todo", this isn't RTBC. Sorry.

swentel’s picture

swentel CreditAttribution: swentel commented
Status: Needs work » Needs review
Issue tags: -Needs change record

Update record, going to trigger a re-test to be sure as well.

swentel’s picture

swentel CreditAttribution: swentel commented

30: field_view_field-2134887-30.patch queued for re-testing.

yched’s picture

yched CreditAttribution: yched commented
Status: Needs review » Reviewed & tested by the community

Thanks @swentel. Expanded the change notice a bit.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Awesome, thanks for the fast turnaround on that!

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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