Change #items within a theme_field() render array from an *array* to the same $items *object* used throughout the rest of the formatter pipeline

Also, theme_file_formatter_table() and theme_image_formatter(), used in image and file formatters (through a #theme property in their render arrays), currently receive an old-style $items array that would need to be switched to a Field object too.

I think per our discussion yesterday, $items as a name is fine.

$items / #items as a name is fine, but #items is currently the array result of $items->getValue(TRUE) (see FormatterBase::view())
Question is: do we want to leave it that way, or pass #items as "the FieldItemList object"

We discussed this today and yes, we should pass $items in there I think. We change everything else to it too (e.g. widgets), so let's make it consistent.

As a bonus, we *could* remove most of the other #stuff in there, because you can get all of it through $items, although we might want to keep a few that are useful for accessing directly. But it would save some bits on those structures.

As a bonus, we *could* remove most of the other #stuff in there, because you can get all of it through $items, although we might want to keep a few that are useful for accessing directly. But it would save some bits on those structures.

As discussed yep, but it probably should be discussed in its own issue.

Getting started. Also made the same change for files and images.

Here's a re roll with some minor improvements, most of the tests should be passing...

Why on earth would the bot remove tags from the issue? :)

What would be the next step for this?

Thanks for rerolling !

1. +++ b/core/lib/Drupal/Core/Field/WidgetBase.php
   @@ -406,7 +406,7 @@ public function massageFormValues(array $values, array $form, array &$form_state
        if ($this->fieldDefinition->isFieldMultiple() && isset($items[0]->_weight)) {
   -      $itemValues = $items->getValue(TRUE);
   +      $itemValues = $items->getValue();
          usort($itemValues, function ($a, $b) {
            $a_weight = (is_array($a) ? $a['_weight'] : 0);
            $b_weight = (is_array($b) ? $b['_weight'] : 0);
   

   Not sure we need ->getValue() anymore here. Using $a->_weight in the closure should work ?

2. +++ b/core/modules/field/lib/Drupal/field/Plugin/views/field/Field.php
   @@ -706,7 +706,7 @@ public function getItems($values) {
   -        $items[$count]['raw'] = $render_array['#items'][$count];
   +        $items[$count]['raw'] = $render_array['#items'][$count]->getValue();
   

   Similarly, the code downstream could probably be updated to work with the FieldItem object instead of the ->getValue() array. Possibly far-reaching though.

3. +++ b/core/modules/file/file.field.inc
   @@ -516,7 +516,7 @@ function file_field_find_file_reference_column(FieldInterface $field) {
   - *   - items: An array of file attachments.
   + *   - items: An array of file field items.
   

   Maybe we could directly mention the FileFieldItem class here ?

4. +++ b/core/modules/image/lib/Drupal/image/Plugin/Field/FieldFormatter/ImageFormatter.php
   @@ -113,14 +113,15 @@ public function viewElements(FieldItemListInterface $items) {
   -          $elements[$delta]['#item'] += array('attributes' => array());
   -          $elements[$delta]['#item']['attributes'] += $item->_attributes;
   +          $elements[$delta]['#item']->attributes = array();
   +          $elements[$delta]['#item']->attributes += $item->_attributes;
   

   $elements[$delta]['#item'] = $item, right ? So what this does is the item ends up with both ->attributes and ->_attributes ?

5. +++ b/core/modules/picture/picture.module
   @@ -173,23 +173,21 @@ function theme_picture_formatter($variables) {
   -  elseif (isset($variables['item']['entity'])) {
   -    $picture['#uri'] = $variables['item']['entity']->getFileUri();
   -    $picture['#entity'] = $variables['item']['entity'];
   +  elseif (isset($variables['item']->entity)) {
   +    $picture['#uri'] = $variables['item']->entity->getFileUri();
   +    $picture['#entity'] = $variables['item']->entity;
   

   "if (isset($item->entity))" could be just "if ($item->entity)" - or "if ($entity = $item->entity)" in tnat case.
   (+ other places in the patch)

   isset() tends to indicate "sometimes it will be there, sometimes not", which is slightly misleading. It will always be "there" (computed, in fact), but can be NULL.

   Also, nitpicky, but while we're in there, functions like that could really use a "$item = $variables['item']" at the top for readability :-)

4. I have no idea what this snippet is about, but it's the same result, just object/array, no?

Not really - previously in this snippet in ImageFormatter::viewElements(), $item and $elements[$delta]['#item'] were two separate things:
- $item was a FieldItem object,
- $elements[$delta]['#item'] was an array (result of $item->getValues())
The 1st one has an $item->_attributes property, the 2nd doesn't have an '_attributes' entry, the code adds a 'attributes' (no underscore) equal to $item->_attributes - the intent is basically, take a attributes from a property in FieldItem, and put it in a place where the theme layer can see it ($elements[$delta]['#item']).

With the patch, $item === $elements[$delta]['#item'] === the FieldItem object.
So the adapted code makes little sense - no need to take a property in the object and put it back in the same object :-)

About 1. Even if FieldItemList ends up extending array access, seems we can't usort those:

Warning: usort() expects parameter 1 to be array, object given in Drupal\Core\Field\WidgetBase->sortItems() (line 435 of core/lib/Drupal/Core/Field/WidgetBase.php).

Changed 2. Couldn't find any place this is invoked so I guess we're fine in here?

For 3, used "- items: An array of FileFieldItem items." but the same text is in many files around.

Removed 4 altogether as for #13

Taken care of 5 and the $item = $variable['item'] things.

Also fixed most of the tests, there's one still around with the entity reference autocreate that is really strange, as is testing a feature that is impossible to achieve through the UI (autocreate nodes from another node), not sure why this is tested and why this patch breaks it.

Related : #2160611: Provide {{ item.item.alt }} Twig syntax for getting data from $item['#item']['alt']

Anyone up for a reroll ?

Yes.

Thanks @Berdir!

+++ b/core/modules/image/lib/Drupal/image/Plugin/Field/FieldFormatter/ImageFormatter.php
@@ -113,18 +113,10 @@ public function viewElements(FieldItemListInterface $items) {
-        // Pass field item attributes to the theme function.
-        if (isset($item->_attributes)) {
-          $elements[$delta]['#item'] += array('attributes' => array());
-          $elements[$delta]['#item']['attributes'] += $item->_attributes;
-          // Unset field item attributes since they have been included in the
-          // formatter output and should not be rendered in the field template.
-          unset($item->_attributes);
-        }

This causes the fails.
I'm looking into it.

This should fix it.

#24:

1. Agreed. I think we could be smarter than that in here now, but yeah, separate issue.
Opened #2170979: remove the getValue() / setValue() dance in WidgetBase::sortItems()

2. Fixed. In fact we should typehint to FileFieldItemList, the function uses it as such (calls $item->isDisplayed())

Updated the issue summary.

Committed/pushed to 8.x, thanks!

Cool. Probably needs a change notice though.

Drafting a change notice.

Draft change notice:

Summary
Previously, field values were passed to theme_field() using an array, with $element['#items']. This array was generated from the runtime FieldItemList object using TypeData’s getValue(), like so:

$element['#items'] = $items->getValue($compute_and_include_all_computed_properties = TRUE);

The viewElements() methods of the image formatters used something similar for each individual field item when passing it to the theme functions they respectively used.

We are now moving away from these arrays to value objects, FieldItem and FieldItemList. Using objects improves performance as well as consistency in API structures: previously either objects or arrays were used depending on the place in the callstack.

This new method passes real business objects down the business pipeline.

Now the render arrays generated by FormatterBase::view() have '#items' (a real FieldItemList object instead of a "translated" array structure).

You can manipulate this with $items[$delta]->property instead of $items[$delta]['property'].

Here are some examples of hook implementation that "see" this '#items' element that need to be updated:

• template_preprocess_field()
• hook_field_attach_view_alter()
• hook_entity_view_alter()

Render arrays generated by ImageFormatter/PictureFormatter::viewElements() also need to be updated. For example:

• theme_image_formatter()
• theme_picture_formatter()

File formatters use a #theme property in their render arrays that also need an update, such as:

• theme_file_formatter_table()

So, essentially any code making use of the render arrays generated by FormmaterBase::view() will need to be updated to treat $items as an object rather than an array.

Twig templates that access this value do not need to be updated, as the twig syntax already handles the object/array syntax variants.

Before
Accessing a field value was handled with an array, like so:

$items[$delta]['property']

After
Now a field value is handled as an object, like so:

$items[$delta]->property

Looks good, mdrummond

Previously, in Drupal 7, field values were passed to theme_field() using an array, with $element[‘#items’]. This array was generated from the runtime FieldItemList object using TypeData’s getValue()

This is actually earlier in the D8 cycle; D7 didn't have a TypedData API.

The following functions that “see” this ‘#items’ element need to be updated:

The following functions hook implementations, maybe?

$items[‘delta’][‘property’]

That looks like the wrong single-quote character to me.

Edited the draft to reflect the suggested changes.

The draft change notice above has been updated with some suggested changes. Would be great to get some additional feedback from those who worked on this patch.

So, essentially any code making use of the render arrays generated by FormmaterBase::view() will need to be updated to treat $items as an object rather than an array.

Small typo in FormmaterBase instead of FormatterBase.

Here are some examples of hook implementation that "see" this '#items' element that need to be updated:

This is a bit confusing to read in my opinion. How about...

The following examples of hook implementations make use of the change from array key to object property:

Revised change notice:

Summary
Previously, field values were passed to theme_field() using an array, with $element['#items']. This array was generated from the runtime FieldItemList object using TypeData’s getValue(), like so:

$element['#items'] = $items->getValue($compute_and_include_all_computed_properties = TRUE);

The viewElements() methods of the image formatters used something similar for each individual field item when passing it to the theme functions they respectively used.

We are now moving away from these arrays to value objects, FieldItem and FieldItemList. Using objects improves performance as well as consistency in API structures: previously either objects or arrays were used depending on the place in the callstack.

This new method passes real business objects down the business pipeline.

Now the render arrays generated by FormatterBase::view() have '#items' (a real FieldItemList object instead of a "translated" array structure).

You can manipulate this with $items[$delta]->property instead of $items[$delta]['property'].

The following examples of hook implementations make use of the change from array key to object property:

• template_preprocess_field()
• hook_field_attach_view_alter()
• hook_entity_view_alter()

Render arrays generated by ImageFormatter/PictureFormatter::viewElements() also need to be updated. For example:

• theme_image_formatter()
• theme_picture_formatter()

File formatters use a #theme property in their render arrays that also need an update, such as:

• theme_file_formatter_table()

So, essentially any code making use of the render arrays generated by FormatterBase::view() will need to be updated to treat $items as an object rather than an array.

Twig templates that access this value do not need to be updated, as the twig syntax already handles the object/array syntax variants.

Before
Accessing a field value was handled with an array, like so:

$items[$delta]['property']

After
Now a field value is handled as an object, like so:

$items[$delta]->property

Change record created: https://drupal.org/node/2181753

Change notice looks good.

It's a bit special that the $items array was generated with getValue(), but the main reason this was done is to simulate the behavior that existed in 7.x, where $items was the raw array data structure.

There will probably be more people that will look at that and are coming from 7.x, not the temporary 8.x state. So maybe it can be re-formulated a bit to clarify separate that? Not sure how...

Thanks @mdrummond !
Agreed with @Berdir though. I refactored the change notice a bit more into a "Drupal 7 / Drupal 8" shape.

Pointed by @Berdir in #2190767: Preview shouldn't mean serializing the render arrays :

As a result of this change here, we serialize the field item list objects including their definitions if the resulting render array gets cached, which happens for example for preview.

Opened #2190767: Preview shouldn't mean serializing the render arrays