Remove deprecated field_attach_*_view()

Initial patch.

For now this keeps the existing calls and the functions themselves, just routes them to the new, way simpler methods in EntityViewBuilder.

patch didn't report the filterEmptyValues() we currently run twice on each field of each displayed entity (which is crazy)

- Removed the absurd $content['display_options'] argument in hook_field_attach_view_alter(),
replaced it with $content['display'] (= the EntityDisplay), which is what is actually useful for implementors.
- Testing what happens if we remove the equally absurd _field_view_prepared flag.

Sorry for staying silent, busy week :-/ A couple clarifications:

The intent of field_attach_view_alter() is to let contrib "alter the output of formatters, whenever they get rendered" - Dave Reid has a lot of modules that rely on it :-)

It does not duplicate hook_entity_view_alter(), since that one doesn't run on single-field renders, aka field_view_field() / field_view_value().

field_view_field() is currently used by:
- views for "field" Views (as opposed to "Views that display fully rendered entities"), e.g. table views; (#1867518: Leverage entityDisplay to provide fast rendering for fields has a plan to drastically optimize that pipeline, conceptually simple but not fully trivial to code :-/)
- edit.module
- ... contrib & custom cases (e.g custom preprocessors)

field_view_value() is "format a single $item with a formatter". No real uses cases in core, but is a handy tool for custom code (in CCK D5, content_format() was our only answer to "please don't print unsafe raw values, use formatters").

On the widgets side there's hook_field_widget_alter() that runs for each widget and thus covers all cases, but back in D7 we explicitly avoided doing the same on the "view" side, for performance concerns (potentially 100s invocations on a listing page). AFAIK the render cache cannot really make that more sustainable, since it only kicks in after the render array is built ?

So yes, as much as I'd like to say "deprecated in favor of the entity hooks", there's a still a need for a hook to alter formatters on both "all fields on full entity view", and on single-field view. The name of that hook can obviously be adjusted, I just didn't go there yet :-)

Then, basically, agreed with #13. The current plan is to move field_attach_view() to the EntityDisplay, and then the hook should probably follow.

Note that the current patch moves in that direction already, since it changes field_view_field() to "render fields in an EntityDisplay that only contains one visible field".
I.e, re:

we would need to make field_view_field() either take an EntityDisplay parameter, or create a mock one

Agreed, and the current patch does the latter. field_view_field() IMO still needs to support its two "modes" (either takes a view_mode name, or an array of arbitrary display settings - both internally get resolved to "an EntityDisplay"), and saving callers the task of building the EntityDisplay themselves in the second case is more DX friendly IMO.

The direction we settle for here impacts what we want to do with
- entity_get_render_display() : #2090509: Optimize entity_get_render_display() for the case of "multiple entity view"
- field_view_field() / field_view_value() : #2134887: move field_view_field() / field_view_value() to methods (the plan over there was to add view() methods on FieldItemList / FieldItem, as shortcuts to implementations living in the EntityViewBuilder, since this was also the intended location for field_attach_*_view() back then).

So I'd like to put the big picture in one place, just to check we're in agreement :

- entity_get_display($entity_type, $bundle, $view_mode)
--> static EntityDisplay::getDisplay($entity_type, $bundle, $view_mode)
internally: as currently (reads from config, or creates a fresh runtime object if not found)

- entity_get_render_display($entity, $view_mode)
--> static EntityDisplay::getRenderDisplays($entities, $view_mode)
Internally: optimized for multiple load, + as currently, invokes hook_entity_display_alter() on each $display

- field_attach_view($entity, $display)
--> $display->attachFields($entity) (name TBD)
This takes care of invoking hook_field_attach_view_alter() (hook name TBD)

Meaning, EntityViewBuilder->viewMultiple()
calls EntityDisplay::getRenderDisplays($entities, $view_mode),
then $display->attachFields($entity)

- field_view_field($entity, $field_name, $view_mode)
--> $entity->{$field_name}->view($view_mode)
== static EntityDisplay::viewField($items, $view_mode)
Internally: calls ::getRenderDisplay(), sets all but $items->getFieldName() to 'hidden', calls $display->attachFields($items->getEntity), returns the right sub render array.

- field_view_field($entity, $field_name, $options)
--> $entity->{$field_name}->view($options)
== static EntityDisplay::viewField($items, $options)
Internally: creates a fresh/empty runtime Display, sets $options for $items->getFieldName(), then same as above

- field_view_value($entity, $field_name, $item, $view_mode)
--> $entity->{$field_name}[$delta]->view($view_mode)
== static EntityDisplay::viewFieldItem($item, $view_mode)
Internally: builds an $items with $item as the only element (like field_view_value() currently), calls viewField($items, $view_mode), returns the right sub-render array

- field_view_value($entity, $field_name, $item, $options)
--> $entity->{$field_name}[$delta]->view($options)
== static EntityDisplay::viewFieldItem($item, $options)
Internally: builds $items as above, calls viewField($items, $options)

Which IMO doesn't look too bad ?

Wasn't aware of #2099131: Use #pre_render pattern for entity render caching (well, it seems I once clicked its "follow" button).
Agreed to revisit later.

the view*() static methods in EntityDisplay. It seems like they wouldn't have much logic in them; why not have that logic inlined into the (non-static) view() of FieldItemList/FieldItem instead?

In #2134887-2: move field_view_field() / field_view_value() to methods, @fago was reluctant to have "display business logic" in the "value" domain objects, and I can agree with that - same as $field->access() is a mere wrapper around a call to a method in an AccessController.

We're probably still talking about 10-ish lines of wrapping logic around a call to $display->attachFields(). I guess we'll need to actually write them and see how this looks.

Another approach would be to keep them in EntityViewBuilder:
$item->view($view_mode) == $view_builder->viewFieldItem($item, $view_mode)
calls EntityDisplay::getRenderDisplay(), prepares $display, calls $display->attachFields(), extracts the result.

updated patch for the current approach :
- field_attach_prepare_view() --> EntityDisplay::prepareFields()
- field_attach_view() --> EntityDisplay::attachFields() (invokes hook_field_attach_view())
- field_view_field() calls EntityDisplay::attachFields() on a $display with one single field.

Interdiff attached, but the patch probably makes more sense on its own.

@todo :
- agree on the method names
- rename hook_field_attach_view_alter() to ? hook_field_view_alter() ?

- Fixes fails:
rdf currently has mappings for things that are not displayed with formatters (e.g term.description - for no purpose, nothing prints the resulting attributes right now), so initializing the $items->_attributes element cannot be done just for formatters.
Patch puts this step back in EntityViewBuilder::buildContent() as in current HEAD

- code simplification: EntityDisplay can use $this->getFieldDefinitions() to iterate on "fields that are displayed with formatters"

Er, scratch patch #24, wrong copy paste, too much looping.

Updated patch & interdiff against #21.

Removes the functions, and move the existing calls over to the new API.

Gave some more thought on naming:

- $display->prepareFields($entities) works IMO

- $display->attachFields($entity) doesn't. We're not "attaching fields" to either the display nor the entity.
"attach" is a remnant of D7's "field attach API", which was "the API that entity types needed to call if they wanted to have fields attached to their entities". It's an obsolete formulation in D8.

--> renamed to $display->viewFields($entity).
This is in line with EntityViewBuilder::view($entity) / viewMultiple($entities), which established "view <-> return a render array".
Here, we're doing this for the fields handled by the Display.

--> accordingly, renamed hook_field_attach_view_alter() to hook_fields_view_alter()
This is in line with hook_entity_view_alter().

Green. This should be ready for reviews now.

One more remark:
For extra simplicity, we might consider merging prepareFields($entities) & viewFields($entity) into one single viewFields($entities) method, that takes care of calling both $formatter->prepareView() & ->view().

The only thing that's stopping us is that hook_entity_prepare_view() currently runs between the calls to prepareFields() & viewFields(). Not sure if that's worth keeping.

Thoughts ?

@sun we already have hook_entity_display_alter(), which #2165835: Rename EntityDisplay to EntityViewDisplay in accordance with its interface is renaming to hook_entity_view_display_alter(). That hook lets you alter the "EntityViewDisplay" object before starting to generate render arrays based on the settings it contains.

So, -1 for hook_entity_display_view_alter() here :-)

The hook we're talking about here is about altering the "view" (the render array) for the fields in an entity. Thus hook_fields_view_alter() seems consistent to me.

Other possibility: as discussed in #16, this hook is an equivalent to hook_field_widget_form_alter() on the formatters side - but is called once for all fields in the entity rather than field per field for performance reasons.

So maybe hook_field_formatters_view_alter() then ?

Works for me too. Renamed the hook (+ patch reroll)

The scope of #1875974: Abstract 'component type' specific code out of EntityDisplay is just to be able to internally recognize different "kinds" of components in the EntityDisplay, not affecting how the rendering of those components happens.
Whatever happens there, we'll still need a function that just renders field formatters, and runs a hook on that, so the hook name should still apply IMO.

What would be the purpose of a function that renders only the field components separately from non-field components

Drats - I thought the prepareFields() / attachFields() part of #18 had been agreed upon :-/.

EVA are currently "extra fields". Extra fields are special cases - by definition, there's no generic rendering step for "extra fields", those are "arbitrary stuff that take care of adding themselves at some point in the build process".
The Display cannot be in charge of the "build extra fields" step, since "extra fields" have no generic "build callback" to call (If they did, what I argue in #1954188-14: Revaluate the concept of 'extra fields' is that they could probably be equated to Blocks, and while that's totally an option for D9, it's way too late for D8 IMO)
All the Display can do for extra fields is hold configured settings (visible / hidden + weight) and hand them back to whatever code does the building.

Then there's the case of "other component types", which might have "generic rendering callbacks" - like field_groups, or like EVA if they decided to stop being "extra fields" and upgrade themselves to be their own "display component type".

The scope of #1875974: Abstract 'component type' specific code out of EntityDisplay is currently only about making Displays know how to store display settings for 3rd party 'component types', not about building the render arrays too.

We can bite the bullet and say we'll include a unified API for "build the render array for all components set to be displayed, whatever their type" - but that will necessarily leave "extra fields" out.

Would mean :
- EntityDisplay->viewMultiple($entities)
This delegates to a viewMultiple() method for each 'component type' handler.
For fields, this does the equivalent of the current prepareFields() (aka f_a_prepare_view()) + viewFields() (aka f_a_view())
This also triggers a hook_[???]_view_alter() on each entity.
- EntityDisplay->view($entity)
This is a shortcut to ->viewMultiple(array($entity))

At this point, what remains in EntityViewBuilder is mostly:
- resolution of the $view_mode into the relevant EntityDisplays,
- addition of the ad-hoc render arrays for the "extra fields" owned by the entity type provider itself.

OK, 3rd approach then :-)

As per #44:
EntityDisplay::view($entity)
EntityDisplay::viewMultiple($entities)
Those build the render arrays for all the components set to be visible in the Display, including fields & other 3rd party component types after #1875974: Abstract 'component type' specific code out of EntityDisplay, but without 'extra fields'.

TODO : find a name for the alter hook triggered in there, proposals welcome.
hook_entity_view_display_alter() is already taken, to alter the options held in the Display before rendering happens.

If we want this to be self contained, Display::view() should assign weights.
(canceled test for the previous patch)

Wrong interdiff for #48, here's the correct one.

no drupalSetContent() in DUTB...

@andypost:

Display::build() / hook_entity_display_build_alter() : why not.

Note that after #2165835: Rename EntityDisplay to EntityViewDisplay in accordance with its interface it would become hook_entity_*view*_display_build_alter().

We'd have:
- hook_entity_view_display_alter() : alter the Display before it generates the render arrays
- hook_entity_view_display_build_alter() : alter the generated render arrays.

And similarly on the form side:
- hook_entity_form_display_alter()
- hook_entity_form_display_build_alter()

Does that sound OK ?

1. 2. : see #43 / #44
The current approach is that Display::view() (or build() or whatever) lets the display generate render arrays for all components set to be visible - except "extra fields", since "extra fields" have no generic render logic, only ad-hoc code.

"All components except extra fields" means "just fields" in current core. But after #1875974: Abstract 'component type' specific code out of EntityDisplay, this would really mean "all components except extra fields".

So yes, EntityDisplay:build() would:
- delegate to a build() method in each component handler (the code that runs formatters in the current patch would move to the "field component handler"),
- and then call the _build_alter hook, the scope of which is the render arrays of all components.

- Renamed Display::view() / viewMultiple() to buidl() / buildMultiple()

- The "alter the render array" hook is now hook_entity_display_build_alter()
Moves from field.api.php to entity.api.php

(/me secretely hopes we're done with names :-p)

Updated the issue summary accordingly.

1. LOL - patch mashup
2. I think I'll never learn to write "instantiate" correctly
3. Yup, the hints were missing from the old hook

+ fix tests (forgot a couple method renames)

Oh gee. I uploaded the wrong patch in #62...
/me slaps himself in the face with a phonebook, then goes hide in the closet.

Interdiff #62 was correct, but reuploading it here.

Any chance a good soul with admin rights could delete comments #63 -> #72 ? - this issue is hard enough to get consensus on without 10 comments of silly retests...

Reroll after #2165835: Rename EntityDisplay to EntityViewDisplay in accordance with its interface

Forgot one 'entity_display' / 'entity_view_display' rename

re @Berdir #78:

1. Right - switched to ContentEntityInterface in most places, *except* in the phpdoc of EntityViewBuilderInterface::buildContent()...
It's a bit of a WTF IMO, but EntityViewBuilderInterface is currently used by a non-ContentEntity : BlockViewBuilder (extends EntityViewBuilder).
Its viewMultiple() implementation doesn't call buildContent(), and its buildContent() implementation is empty, but still... Switching to ContentEntityInterface there wouldn't be accurate.
(actually, I don't understand why buildContent() is part of EntityViewBuilderInterface - the public entry points should be view() / viewMultiple() ?)

What is true though, is that EntityDisplays are only used for ContentEntities, thus adjusted the hints in this area.
(+ field_view_field() / field_view_value() were inconsistent, one hinted on EntityInterface, the other on ContentEntityInterface)

2. Yes, ultimately those tests should move out of field.module and to a test class in Core/Field
I'd rather do that in a followup, to limit patch size (also, moving tests around can be done at any time before or even after release)

No, this will probably require a change notice of its own (+ updates of a couple ones).
Will work on a draft, but probably won't be before a couple days.

Awesome! Will finish change notices in the next couple days.

Then, see you in
- #2095195: Remove deprecated field_attach_form_*()
- #2134887: move field_view_field() / field_view_value() to methods

Updated the change record created for the issue : https://drupal.org/node/2193813

And updated existing changes records mentioning the old functions:
https://drupal.org/node/1875952
https://drupal.org/node/1907792
https://drupal.org/node/1882428
https://drupal.org/node/2181753
https://drupal.org/node/1825844