Enable modules to inject attributes into field formatters, so that RDF attributes get output

Do we want to let each formatter actually control (or forget) whether it needs to call the FormatterBase::prepareView() ?
If not, since this is not really formatter-related, I'd be inclined to move the hook invocation inside field_attach_prepare_view() ?

More generally, adding those extra properties directly in the $items held by the entity is a bit hackish. I do get that $items it's the only thing formatters receive currently, so is the only thing fitted to pass additional data.
#2021817: Make widgets / formatters work on EntityNG Field value objects. is about making formatters (prepareView() / view() / viewElement() methods) receive $items as EntityNG Field objects instead of the old D7 array format (that is currently generated from the native Field object before calling the formatter, and transleted back). Wondering what the perf impact would then be of adding ad-hoc properties in those NG FieldItems. Could use @fago's opinion on this one...

Strictly speaking, those elements are not part of the entity itself, they are derived data computed at display time.
For those kind of things, I've been considering whether it would make more sense to let the "prepareView" phase build an $extra array, ditched after rendering, rather than cram random additional properties in the $items themselves (which makes the Entities stateful, since viewing them alters their content currently)

Summary of the discussion we had with @linclark
Myself from #16

I've been considering whether it would make more sense to let the "prepareView" phase build an $extra array, ditched after rendering, rather than cram random additional properties in the $items themselves (which makes the Entities stateful, since viewing them alters their content currently)

Basically this means introducing a $view_state similar to the $form_state we have on the FAPI side.
While it makes sense IMO, I'm not sure of the extend of the API impact at this time in the cycle...

So I guess this boils down to :
@fago, @Berdir, do you guys think the approach in #10 - let 3rd party code add random properties (not listed in getPropertiesDefinition()) in FieldItem objects during the render callstack - is OK performance-wise ?

From #4:

On the other hand, some fields require to have the attributes added inside the field formatter

That makes this either a task or a bug report, but not a feature request. We have rdf.module in core, and as of D8, we also have date and link field types in core, and we need to make sure those all work together correctly. I'm categorizing this as a task for now, because I don't quite understand what the specific bug is in terms of what happens when the RDFa is on the less optimal item wrapper element, as it is currently in HEAD. If someone can explain it as a bug, then please recategorize this accordingly.

Also, raising to major, because entities, fields, and semantic correctness of their output is so foundational to Drupal.

Updated issue summary.

  <span {{ country_attributes }}>
    {{country}}
  </span>

That all looks and sounds reasonable, but wouldn't the span stay even if the attributes are empty, i.e. RDF module is disabled? I guess front-end people wouldn't be happy about that, so could we make it in a way it only creates the wrapper-spans when necessary?

@fago, @Berdir, do you guys think the approach in #10 - let 3rd party code add random properties (not listed in getPropertiesDefinition()) in FieldItem objects during the render callstack - is OK performance-wise ?

I think it's ok, but I don't think it's an optimal solution as putting stuff in $item just for copying it further later on is a bit weird. Couldn't that be an alter of the render array instead? What we are doing here is not really prepare-viewing, i.e. pre-load stuff or so - is it? It's RDF taking part at the "view" process itself, not?

As I wrote in #16, I totally agree that adding those in each $item in prepare_view is a bit off.

Hence my proposal of adding them to a $view_state array, similar to $form_state, that would be passed along the view callstack.
That seems like a reasonable flow: collect additional info from 3rd party at prepare_view time, then let the formatter deal (or not) with that info - since, if I get things right, we need to move to a model where formatters (or at least some of them) need to be aware of RDF matters (or at least be aware that some modules might want to add some granular HTML attributes).

But as @fago suggests, if this can be handled by having rdf_entity_view_alter() post-adding the needed attributes to the render arrays built by the formatters, it might be easier at this point in the cycle (less API change impact). It then becomes the responsibility of the theme function used by the formatter to use make use (or not) of those elements from the render array. The formatter class itself stays completely ignorant of RDF / 3rd party additions.

I guess that in the first approach, the formatter will probably just stick the attributes in the render array and let the theme function handle them as well - which would in the end still make outputting rdf be the responsibility of the theme function ?

I have a bit of a hard time deciding which approach is cleaner :-) Thoughts ?

Is there any other hook that RDF/3rd party modules could implement that would run before viewElements(), so that the attributes are available during viewElements?

There's hook_entity_prepare_view(), which would be a better fit than the hook_formatter_prepare_view() introduced in #10 (hook_entity_prepare_view() costs one single hook invoke for an entity_view() or entity_view_multiple(), while the hook introduced in #10 would be one hook invoke per field).

So, am I right in stating that we have an agreement on the following ?
1) RDFa (well, "extra attributes") need to be collected before formatters kick in and produce the final render array, so that formatters (or the theme functions they end up using, if any) can use them (or not) in whatever way they see fit.
2) For each field, those "extra attributes" and their granularity depend only on the field definition (field type...) and on the field values for a given entity. They are formatter-agnostic, and do *not* depend on the specific formatter that will output the values (well, except - no need to compute attributes for fields that are not displayed, right ?)

I'd especially like confirmation of 2), since I have a hard time convincing myself that the attributes are completely independent of the final shape of the HTML.

I have a hard time convincing myself that the attributes are completely independent of the final shape of the HTML.

To give concrete examples:
- for a date field, do the exact same attributes make sense whether the date is displayed as a human readable, ISO, timestamp... ?
- for an image, do the exact same attributes make sense whether the image is displayed as a "thumbnail", "large", original image, raw file name, link to the file... ?

Because of course, if the actual set of attributes has to depends on the formatter, then the problem becomes much more complex...

one possibility is hook_entity_prepare_view(). The RDF module would go through each field and populate such attributes. Not sure yet where these attributes would be stored so they are preserved until we get to the formatter viewElements() code.

Exactly, this is the issue that was raised earlier about sticking extra data right into the field value items. We still need to solve that, but at least now that we've established the code flow that we need, we know that this is what we need to figure out.

Another alternative would be to invoke a new hook at the beginning of viewElements(). (...) This hook would be invoked for each field that is being rendered.

Right, and that's a performance no-go IMO. Better to leave hook_entity_prepare_view($entities) implementations the burden of iterating on the entities and the fields inside those entities. entity_view() / entity_view_multiple() are very much on a critical path.

Just a heads up, we are trying to solve the issue of #type doing just rendering via l(), etc. in #2052253: [META] Add #render property to drupal_render() and convert #type "#pre_render -> #markup" calls to use it and related to that #2052271: Introduce theme_info() call, deprecate theme() and have theme_info() just return render array information (like element_info()) (even if theme_info() will probably not happen).

So if anyone could again very clearly state the need / problem that exists here (with an example), it would help us to take care of that in the theme system overhaul.

+++ b/core/modules/system/system.module
@@ -3284,6 +3288,12 @@ function theme_exposed_filters($variables) {
+function theme_html_data_wrapper($variables) {
+  $element = $variables['element'];
+  $attributes = new Attribute($element['#attributes']);
+  return "<span$attributes>" . drupal_render_children($element) . "</span>";
+}
+

Twig template should be used here instead of theme function and we can move logic in preprocess.

Even #type => html_tag might be enough.

1. +++ b/core/includes/common.inc
   @@ -3481,6 +3481,13 @@ function drupal_pre_render_html_tag($element) {
   +  // The title might be a render array. Render it to pass a string to l().
   +  if (is_array($element['#title'])) {
   +    $element['#title'] = drupal_render($element['#title']);
   +    $element['#options']['html'] = TRUE;
   +  }
   

   Nope, this is already happening in l().

   The html to TRUE should be set explicitly by the calling code.

2. +++ b/core/modules/email/lib/Drupal/email/Plugin/field/formatter/MailToFormatter.php
   @@ -38,6 +38,9 @@ public function viewElements(EntityInterface $entity, $langcode, FieldInterface
   +      if (!empty($item->html_data_attributes)) {
   +        $elements[$delta]['#attributes'] = $item->html_data_attributes;
   +      }
   

   Uhm, I think this should merge, rather than overwrite.

   Also needs tests for this case.

3. +++ b/core/modules/system/system.module
   @@ -190,6 +191,9 @@ function system_theme() {
   +    'html_data_wrapper' => array(
   +      'render element' => 'element',
   
   @@ -3284,6 +3288,17 @@ function theme_exposed_filters($variables) {
   +function theme_html_data_wrapper($variables) {
   

   Remove the theme function, use #type => html_tag in the render array directly.

4. +++ b/core/modules/text/lib/Drupal/text/Plugin/field/formatter/TextDefaultFormatter.php
   @@ -42,7 +42,19 @@ public function viewElements(EntityInterface $entity, $langcode, FieldInterface
   +        $elements[$delta] = array(
   +          '#theme' => 'html_data_wrapper',
   

   Use #type => html_tag here. and in the other case as well.

Do most of these field formatters really have no wrapper that the attributes could be merged to?

add reference to compound field issue

Updated issue summary.

1. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/Field/FieldRdfaTestBase.php
   @@ -58,9 +65,13 @@
   +    $build = entity_view_multiple(array($this->entity), 'default');
   

   why not entity_view() ?

2. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/Field/FieldRdfaTestBase.php
   @@ -78,9 +89,9 @@ protected function createTestField() {
        entity_create('field_instance', array(
   -      'entity_type' => 'entity_test',
   +      'entity_type' => 'node',
   

   it's a little sad to switch tests from entity_test to node - can't we just add EntityRenderController as a render controller in EntityTest annotations ?

3. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/Field/TaxonomyTermReferenceRdfaTest.php
   @@ -0,0 +1,109 @@
   +    $this->entity = $this->drupalCreateNode(array('type' => 'test_node_type'));
   +    $this->entity->get($this->fieldName)->offsetGet(0)->get('target_id')->setValue($this->term->id());
   

   Could be just $this->entity->{$this->fieldName}->target_id = $this->term->id();
   Possibly even, using more implicit magic : $this->entity = $this->drupalCreateNode(array('type' => 'test_node_type', $this->fieldName => $this->term->id())); (I'm less sure about that)

4. +++ b/core/modules/rdf/rdf.module
   @@ -197,6 +197,27 @@ function rdf_rdfa_attributes($mapping, $data = NULL) {
   +  foreach ($entities as $id => $entity) {
   +    $mapping = rdf_get_mapping($entity_type, $entity->bundle());
   

   Perf: rdf_get_mapping() doesn't seem to be statically cached, so this will load a config entity over and over again for all entities of the same bundle ?

   (although I guess HEAD as the same issue ?)

5. +++ b/core/modules/rdf/rdf.module
   @@ -197,6 +197,27 @@ function rdf_rdfa_attributes($mapping, $data = NULL) {
   +        foreach($entity->get($name) as $property) {
   +          $property->html_data_attributes = rdf_rdfa_attributes($field_mapping);
   +        }
   

   This is where I'm lost. This code adds one html_data_attributes for all deltas, rather than one per item ?
   Also, the actual field values are not passed to rdf_rdfa_attributes() (they were, in previous rdf_preprocess_field()). So those can't be the final data_attributes (per item, built on item values), right ?

   + This is where https://drupal.org/node/2072791 would kick in ? Do we have a notion what it would look like ?

   + Minor:
   Space after foreach
   Current code uses $field or $items rather than $property

6. +++ b/core/modules/system/system.module
   @@ -8,6 +8,7 @@
   +use Drupal\Core\Template\Attribute;
   

   Doesn't seem needed ?

7. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/field/formatter/LinkFormatter.php
   @@ -50,6 +50,13 @@ public function viewElements(EntityInterface $entity, $langcode, FieldInterface
   +          if (!isset($elements[$delta]['#options']['attributes'])) {
   +            $elements[$delta]['#options']['attributes'] = array();
   +          }
   

   Could be shortened to
   $elements[$delta]['#options'] += array('attributes' => array());

   Also, a similar construct would streamline the hunk in ImageFormatter.

8. +++ b/core/modules/text/lib/Drupal/text/Plugin/field/formatter/TextTrimmedFormatter.php
   @@ -77,7 +77,18 @@ public function viewElements(EntityInterface $entity, $langcode, FieldInterface
   +      if (empty($item->html_data_attributes)) {
   +        $elements[$delta] = array('#markup' => $output);
   +      }
   +      else {
   +        // Adds a wrapping element if attributes are specified for this item.
   +        $elements[$delta] = array(
   +          '#type' => 'html_tag',
   +          '#tag' => 'div',
   +          '#attributes' => $item->html_data_attributes,
   +          '#value' => $output,
   +        );
   +      }
   

   Wondering how much of this could / should be moved to a helper method in FormatterBase ? Probably no big deal, though.

Oh, ok, sorry, I misread the code snippet, it does assign html_data_attributes to each item separately. I guess it's because var names are misleading. I'd suggest:

foreach ($entity->get($name) as $item) {
  $item->html_data_attributes = rdf_rdfa_attributes($field_mapping);
}

Then what I don't get is that html_data_attributes doesn't depend on the actual field values ?
rdf_rdfa_attributes() only gets passed $field_mapping here, not the actual field values.
In current HEAD, rdf_preprocess_field() does $variables['item_attributes'][$delta] = rdf_rdfa_attributes($field_mapping, **$item**);

+++ b/core/modules/rdf/rdf.module
@@ -197,6 +197,27 @@ function rdf_entity_prepare_view($mapping, $data = NULL) {
+          $item->html_data_attributes = rdf_rdfa_attributes($field_mapping, $item->getString());

getString() seems weird, $item->getValue() would be more like what you need - i.e "the item as a ($property_name => $property_value) array", which is what rdf_preprocess_field() was passing rdf_rdfa_attributes() so far.
(sorry, I could have mentioned that method explicitly in my earlier post)

AAMOF, *if* rdf_rdfa_attributes() works exclusively on field items (but I'm not sure that's the case), it would be worth refactoring it in a followup to accept FieldItem objects directly.

The last thing that bugs me is that all those $item->html_data_attributes properties stay in the $entity for the rest of the request, and can get stale if field values are changed, or if the same entity is displayed again in a different view mode later in the request.
Can't easily check right now, but I'm not sure there's a place / hook where those could be cleaned up / removed once formatters have done their job.
We should at least make sure that rdf_entity_prepare_view() resets html_data_attributes to at least an empty value on all fields in the entity ?

Note that rdf_rdfa_attributes() is not only used for fields, we use it for example to generate the number of comments link which is not a field

Hmm, is it not in D8 ? It's not a "configurable field" of course, but D8's entity API unifies how "base fields" (former "properties") & "configrable fields" (former "Field API fields") are handled and manipulated within an entity.
Anyway, that's food for thought for later at best, but it might make sense that in the end "fields" (bese, configurable or computed) are the things that can have rdfa attributes.

But I would think that if entity values are to be changed during a request, shouldn't they be changed prior to viewing/rendering the entity?

That's the typical case, of course, and in practice it's probably going to work in 99% code flows, so no biggie. But that's still less than ideal, this is exactly why we try to avoid entities to have a "state" - so that you don't hit weird edge cases (for example when writing tests).

However, *if* formatters are the only things that are ever going to use this html_data_attributes property, then it's not really a problem in practice. As you point, rdf_entity_prepare_view() will always re-compute html_data_attributes before formatters run, so it's not a big deal if field values changed meanwhile.
I'd still find it cleaner if we had a way to remove html_data_attributes after the rendering has happened, so that we don't clutter the entity with possibly stale values, but that's not a blocker.

Code-wise I'm fine with the patch. I don't really follow the test changes that were made (notably in the last patch ?) - are they all strictly related to the code changes made here ?

Code style / nitpicks review:

1. +++ b/core/modules/file/file.module
   @@ -1564,19 +1564,19 @@ function file_managed_file_pre_render($element) {
     *   - description: A description to be displayed instead of the filename.
   + *   - attributes: Associative array of attributes to be placed in the a tag.
   

   "An associative array...", to be consistent with the line above ?

2. +++ b/core/modules/file/lib/Drupal/file/Plugin/field/formatter/GenericFileFormatter.php
   @@ -38,6 +38,10 @@ public function viewElements(EntityInterface $entity, $langcode, FieldInterface
   +        // Passes HTML data attributes to the theme function.
   

   Nitpick. Pphdoc for functions/methods should be 3rd person, but inline code comments are usually imperative.
   -> e.g. "Pass the HTML data attributes"
   (same in ImageFormatter)

3. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/Field/FieldRdfaDatatypeCallbackTest.php
   @@ -0,0 +1,61 @@
   +namespace Drupal\rdf\Tests\Field;
   +
   +
   

   One empty line too many

4. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/Field/FieldRdfaDatatypeCallbackTest.php
   @@ -0,0 +1,61 @@
   +  }
   +}
   

   Missing an empty line after the last method in the class

5. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/Field/FieldRdfaTestBase.php
   @@ -58,9 +58,14 @@
   +    // The field formatter will be rendered inside the entity. Sets the field
   

   Same as above - s/Sets/Set

6. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/Field/TaxonomyTermReferenceRdfaTest.php
   @@ -0,0 +1,111 @@
   +  }
   +}
   

   Empty line after last method

7. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/Field/TestDataConverter.php
   @@ -0,0 +1,26 @@
   +namespace Drupal\rdf\Tests\Field;
   +
   +
   

   two empty lines

8. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/Field/TestDataConverter.php
   @@ -0,0 +1,26 @@
   +class TestDataConverter {
   +  /**
   

   Missing empty line after the class statement, and after the last method

9. +++ b/core/modules/rdf/rdf.module
   @@ -197,6 +197,27 @@ function rdf_rdfa_attributes($mapping, $data = NULL) {
   +  // Iterates over the RDF mappings for each entity and prepares the RDFa
   

   s/Iterates/Iterate
   (well, same in the rest of the patch ;-))

10. +++ b/core/profiles/standard/config/rdf.mapping.node.article.yml
    @@ -23,7 +23,6 @@ fieldMappings:
    -    mapping_type: 'rel'
    

    Still don't get this - not that I would expect to ;-), but maybe add a word in the issue summary about how this is related / why this is needed ?

    More generally, it would help the patch to be committed if the issue summary was updated with:
    - an overview of the code changes in the patch (new code flow, hooks involved, stuff added/removed)
    - a section about the impact in terms of "API changes" (like "formatters are now in charge of outputting RDFa data, not RDFa gets printed if they fail to do so")

Updated issue summary.

@scor: I see you updated the OP, thanks!

field.html.twig no longer has to worry about the RDFa markup of fields

Does it mean the patch should be changing / simplifying things in theme_field() / field.html.twig ?

field_theme() accepts a new parameter '$attributes', which are attributes to be placed in the a element (necessary for optimal RDFa markup for file field)

This is about theme_file_link(), not field_theme(), right ? :-)

+ adding a couple tags, not sure whether there are tags that make sense on the RDF side.
We'll need to get a core committer's approval for the API changes

API changes

Thanks for the clarifications.

I've been wondering one thing: if the field template can still print attributes as in D7, would it be possible to still output the attributes in there if the formatter hasn't taken care of it ?

Then what the patch does is add the *possibility* (not the obligation) for formatters to output the attributes if they feel they'll do it better than the default output, but there is still a default output if the formatter doesn't want to bother. This way we don't put an additional burden on all formatters (which, even if in theory other modules might add attributes, looks like "all formatters need to account for the optional rdf.module"), and the patch becomes a strict API addition, not an API change.

Could work like:
- the formatter should unset $item->html_data_attributes if it has taken care of displaying them
- field rendering displays the html_data_attributes it finds in the $items, in the D7 way

What do you think ?

re #65: I'm not sure we understood each other :-)

I'm not talking about adding another way to add another set of rdf info. There would still be one single set of data, $item->html_data_attributes, added in rdf_entity_prepare_view(). What I'm proposing is only about how this set of data gets into the HTML.

- current patch does: the formatter needs to catch it - if it doesn't the rdf data is lost even though it's supposed to be relevant since rdf.module placed stuff in there and expects it to be displayed. This adds burden on every single implementation of a formatter (contrib, or custom one-offs in a custom module for a specific site) and requires them to bother with stuff (rdf) they didn't need to know about so far.

- proposal is: formatter receives $item->html_data_attributes as in the current patch, and can do custom output if it needs - if so, it unsets $item->html_data_attributes before returning. If it doesn't, it means it has no special formatting needs for the rdf data, and is ok with the "default output" (the one we have in D7 at the field.tpl level)
rdf_preprocess_field() then catches the $item->html_data_attributes that are left (it does not re-compute a new set of attributes), and formats them as in current D7.
--> Image / File can still have their specific output, the other 95% formatters don't have to bother about html_data_attributes, the contract of "being a formatter" stays simple.

Awesome ! And yay on reverting the changes in text formatters :-)

Nitpick:

+      // Merge HTML data attributes in item_attributes.
+      // @todo https://drupal.org/node/2034003
+      $variables['item_attributes'][$delta] = new Attribute($item['html_data_attributes']);

It's a known bug in HEAD, that has its own issue already, and this patch doesn't really change things (doesn't introduce it or make it worse), so we can ditch the @todo IMO. One of the patches will need to be rerolled when the other gets in, but we usually don't put @todos for that.

Other than that, RTBC :-)

The OP will need to be updated, there are no API changes per se now :-)
(well, not sure if the mapping_type: 'rel' thing constitutes an API break or if its something that is just not needed anymore)

I've been following this issue since the beginning because it so strongly relates to in-place editing. I manually tested this patch to ensure it did not break in-place editing (because Drupal does not yet have JS test coverage), precisely because it touches the same areas. Everything continues to work just fine.

The updated code looks a lot cleaner, very nice!

I'm slightly worried that without the follow-up #2072791: Output RDFa markup for type + title in taxonomy term reference and entity reference formatters being addressed (which is generally no longer accepted in this stage), this issue leaves core in a regressed state. But I trust scor enough to follow through on that, so I think it's a low risk.

So: yay!

But: I've got a bunch of stupid nitpicks — sorry :( If all of it was just typos I could fix, I'd have rerolled the patch, but I don't feel comfortable writing test class docs and there is one question that I can't answer.

1. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/Field/FieldRdfaDatatypeCallbackTest.php
   @@ -0,0 +1,60 @@
   +class FieldRdfaDatatypeCallbackTest extends FieldRdfaTestBase {
   
   +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/Field/TaxonomyTermReferenceRdfaTest.php
   @@ -0,0 +1,113 @@
   +class TaxonomyTermReferenceRdfaTest extends FieldRdfaTestBase {
   

   Test class docs missing.

2. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/Field/FieldRdfaDatatypeCallbackTest.php
   @@ -0,0 +1,60 @@
   +      'name'  => 'Field formatter - datatype callback',
   +      'description'  => 'Tests RDFa output for field formatters with a datatype callback.',
   
   +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/Field/TaxonomyTermReferenceRdfaTest.php
   @@ -0,0 +1,113 @@
   +      'name'  => 'Field formatter - taxonomy term reference',
   +      'description'  => 'Tests RDFa output by taxonomy term reference field formatters.',
   

   Double spaces before the double arrows.

   And that dash should probably be an em-dash :)

3. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/Field/FieldRdfaDatatypeCallbackTest.php
   @@ -0,0 +1,60 @@
   +  }
   +}
   

   Missing newline.

4. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/Field/TestDataConverter.php
   @@ -0,0 +1,27 @@
   +   * @param mixed $data
   

   Should be array as per the code below, not mixed.

   If in general it could be mixed, then why isn't there an interface for it so that you can just inherit the docs here?

5. +++ b/core/modules/rdf/rdf.module
   @@ -197,6 +197,27 @@ function rdf_rdfa_attributes($mapping, $data = NULL) {
   +    // Only prepare the RDFa attributes for the fields which are configured
   +    // to be displayed.
   

   80 col w wrapping is slightly off.

6. +++ b/core/modules/rdf/rdf.module
   @@ -321,35 +342,13 @@ function rdf_preprocess_node(&$variables) {
   +      // Merge HTML data attributes in item_attributes.
   

   "Merge … with", not "Merge … in"?

explain hybrid approach

Yay. Back to RTBC then.

RTBC+1

The patch in #73 did remove // @todo https://drupal.org/node/2034003 though — is that intentional?

Alright :) Definitely RTBC then.

API changes (most of them are API additions)

This looks great to me as well. Another RTBC+1. I was about to point out that the issue summary says:

Besides requiring duplicate values in the wrapping element, D7's suboptimal method of placing RDFa markup in the wrapping element failed when it came the time to RDFa-ize compound fields from contrib which have multiple elements inside each item, for example addressfield (street, locality, country, zip code) or fivestar (rating, number of votes).

And it doesn't look like this patch deals with that at all, but then I see that #72 links to the issue for addressing that.

Minor question: is $item->html_data_attributes the best name for this injected property? RDFa attributes don't generally start with data-: for example, none of the ones in the issue summary do.

+++ b/core/modules/rdf/rdf.module
@@ -197,6 +197,27 @@ function rdf_rdfa_attributes($mapping, $data = NULL) {
+          $item->html_data_attributes = rdf_rdfa_attributes($field_mapping, $item->getValue());

Presumably, the reason we're calling it html_data_attributes, rather than rdf_attributes, is that we want other modules (e.g., edit.module) to be able to add attributes here as well. In which case, this should be a += (or some other merge operation) rather than a =.

Both discussing an improved name and the merging can be follow ups though, so leaving at RTBC.

This looks good overall. I had a couple of minor nitpicks. Also while the benchmarks are encouraging, I'd be more comfortable if it was profiling. On the other hand the shift from hook_preprocess() to hook_entity_prepare_view() does look like it should improve things. And agreed the code looks a lot nicer now.

1. +++ b/core/modules/file/lib/Drupal/file/Plugin/field/formatter/GenericFileFormatter.php
   @@ -38,6 +38,13 @@ public function viewElements(EntityInterface $entity, $langcode, FieldInterface
   +          $elements[$delta]['#attributes'] = $item->html_data_attributes;
   

   Have they actually been rendered to a string already when we get here? Looks like they're prepared for rendering in this same hunk, so being removed to avoid duplication?

2. +++ b/core/modules/image/lib/Drupal/image/Plugin/field/formatter/ImageFormatter.php
   @@ -120,6 +120,14 @@ public function viewElements(EntityInterface $entity, $langcode, FieldInterface
   +          $elements[$delta]['#item']['attributes'] += $item->html_data_attributes;
   

   Why does this have to do extra set up compared to the GenericFileFormatter - could do with a comment. The formatter below also does the extra setup, so is GenericFileFormatter missing something?

3. +++ b/core/modules/rdf/rdf.module
   @@ -321,35 +342,12 @@ function rdf_preprocess_node(&$variables) {
   +  // If the field formatter didn't inject the HTML data attributes in its HTML
   +  // output, add these attributes to the field template.
   +  foreach ($variables['element']['#items'] as $delta => $item) {
   +    if (!empty($item['html_data_attributes'])) {
   +      // Merge HTML data attributes with item_attributes.
   +      $variables['item_attributes'][$delta] = new Attribute($item['html_data_attributes']);
   

   When does this code run? It says it only runs when the formatter didn't inject HTML data attributes, is the comment reversed?

re @catch #80.3:

The patch implements the following sequence:
1) rdf_entity_prepare_view() computes rdf attributes for each entity / field / delta, and puts them in a place where the formatters can access them : $entity->$field_name[$delta]->html_data_attributes
2) formatters then run, and *can* make use of html_data_attributes to place the attributes in their HTML in a custom way if they need to - they then need to unset $item->html_data_attributes to prevent setp 3) from happening.
3) rdf_preprocess_field() grabs the html_data_attributes that are still present and makes sure they end up in the HTML in the "default / fits most cases" output (i.e. the D7 output, wrapping the HTML generated by the formatter)

This is what this comment in rdf_preprocess_field() is saying - "inject" as in "took care of placing the content of $item->html_data_attributes into the HTML".

1. +++ b/core/modules/file/lib/Drupal/file/Plugin/field/formatter/GenericFileFormatter.php
   @@ -40,9 +40,10 @@ public function viewElements(EntityInterface $entity, $langcode, FieldInterface
   +          $elements[$delta] += array('#attributes' => array());
              $elements[$delta]['#attributes'] = $item->html_data_attributes;
   

   Then the second line should do =+ instead of =, or the 1st line is useless ?

2. +++ b/core/modules/image/lib/Drupal/image/Plugin/field/formatter/ImageFormatter.php
   @@ -124,8 +124,8 @@ public function viewElements(EntityInterface $entity, $langcode, FieldInterface
   +          // Unset HTML data attributes since they've been added to the
   

   Minor, didn't catch it before, but let's fix it since it seems this will need another roll: core code comments try to avoid elisions (don't -> do not, they've -> they have...)

   Also, I'd argue that "added to the formatter" is a bit sloppy :-). Maybe "included in the formatter output" ?

Also, I'm actually not sure html_data_attributes / "HTML data attributes" is the right name for the property.
First, as you pointed above, it's open to mis-interpetation: it means "html-data attributes", not "html data-attributes" - i.e it's not HTML (markup).
Then, those are rdfa attributes, nothing else ? We can't really pretend there can be other stuff in there, since:
- it's rdf_entity_prepare_view() that sets the property (overwriting anything another module might have placed)
- it's rdf_preprocess_field() that takes care of transferring it into the markup
So it does look like it's really the realm of rdf.module ?

So it does look like it's really the realm of rdf.module ?

No. We should fix those issues you point out instead:
- Move the initializing of $item->html_data_attributes into EntityRenderController::buildContent() prior to invoking hook_entity_prepare_view().
- Move the transferring to the markup from rdf_preprocess_field() to template_preprocess_field().
- Make rdf_entity_prepare_view() add to $item->html_data_attributes, not overwrite it.
- Add a test that verifies that a test module can also add to $item->html_data_attributes within hook_entity_prepare_view(), and that it makes its way into the markup.

Any thoughts on a better name than "html_data_attributes"? Some ideas:
- "attributes": However, this overlaps with some field types (e.g., Link) that define this as a (editable) property of the field. But, maybe that's ok, since we can just merge the two anyway? But, maybe the Link field formatter unsetting an intrinsic property of its field would be bad?
- "item_attributes"
- "injected_attributes"
- any others?

#84 sounds like a good plan.

the Link field formatter unsetting an intrinsic property of its field would be bad?

Yes, I think it would :-)

No real insight on better names on my side, I could go with item_attributes I guess.
@scor, what do you think ?

One other suggestion (I don't know if I like it more or less than "item_attributes", so just putting it out there to see what others think), is "_attributes". This would follow the same convention that Symfony uses for route parameters: parameters that are route/controller specific don't lead with "_", those that have special meaning to the routing system and non-route-specific subscribers lead with "_". See #2043781: Drupal::request()->attributes->get('account') may conflict with an account object loaded from the path for an example issue where this was discussed with respect to _account and #2052389: All elements added to the Request attributes should have a _ prepended unless they come from the path where it was extended to all non-route-specific attributes. So if we want to adopt this pattern, field item properties that are defined by the field type should not lead with "_", but those that are added by the field management system and its non-field-type-specific hooks should lead with "_".

My hesitance with this, however, is that I still have old habits of thinking that a leading "_" means "private". But we have OOP now, where "private" is a language feature, not an "_" convention any more, so perhaps it's appropriate to redefine the meaning of "_" in this way, as Symfony has already done.

WIdgets already place a '_weight' property for reorderable multiple fields (although that one is just in submitted form values and doesn't actually reach the $item objects)
So why not '_attributes', yes. I'll defer to @scor on that one.

(@effulgentsia: only tangentially related, but while you're here :-p - you might be interested in #2083835: theme('field') displays no value on empty fields even if the formatter returned an output)

Aw. Now that I mention it...
#2083835: theme('field') displays no value on empty fields even if the formatter returned an output is indeed not strictly related, but it makes me realize that the approach taken here means that image field's "default image" will not get RDF attributes, since it's added by the image formatter and is not present in the $entity yet when rdf_entity_prepare_view() runs.

Not sure if that's a big issue - because other than that, populating rdf data in rdf_entity_prepare_view() makes a damn lot of sense... :-(

$variables['items'] is the thing fed to the template, it contains the render arrays returned by the formatters.
So yes, $element['#items'] is the one you want to use to grab $item->_attributes properties.
A bit confusing indeed, but this is probably not the right issue to change variable names in templates :-).

Problem in latest patch is that those added lines in template_preprocess_field() have the effect of overwriting the $variables['item_attributes'][$delta] that are set in the lines just above. Not sure if those are obsolete now and can be removed, but right now it looks at least weird ?

Not sure if those are obsolete now and can be removed

Not sure when they were ever not obsolete, but yeah, they can be replaced as so. template_preprocess_field() is always the first theme('field')-specific preprocess function to run, so $variables['item_attributes'] can't already be set to anything prior to it running.

There is getProperties() but that returns everything.

getProperties() would be correct: it shouldn't return anything that would trigger a "field is unknown" exception. But for computational efficiency, here's the way I recommend.

#96: 1778122-96-formatter-attributes.patch queued for re-testing.

Ouch, nice catch !
I opened #2089273: upgrade path puts D7 deleted fields in EntityDisplay objects for FieldUpgradePathTest / test_deleted_field

Fix looks correct. #1875974: Abstract 'component type' specific code out of EntityDisplay might get us to a point where we can ask a $display object "give me all the displayed component that are 'fields'", but for now this looks like the best we can do.

Actually, why would the following not work ?

foreach ($entity as $field_name => $items) {
  if ($displays[$entity->bundle()]->getComponent($field_name)) {
    foreach ($items as $item) {
      $item->_attributes = array();
  }
}

Re #100, it would, but it would result in getProperties() being called (for the iterator), which means a get() on every field name, instantiating a bunch of objects that might not be needed if they're not displayed. That's what I tried to explain in the code comment, but maybe not well enough?

Ah, you're right, not all Field objects have been created at this time.

Maybe "Avoid needlessly instantiating Field objects by only iterating on those that are actually displayed" ? Then the comment about "not all displayed components are actual entity fields" could go above the if (getPropertyDefinition()) line ?

Also, I was wrong in #89, the "default image" will still receive rdf attributes, for the same reason it does now: the default image gets added as a regular FieldItem object by the formatter in prepareView() / field_attach_prepare_view(), and this runs before _attributes are initialized and populated.

It sucks that image formatters modify the entity by adding a field value - if the entity was saved after being displayed (which usually doesn't happen of course, and node preview only displays a clone of the entity being edited), the "default image" would get saved as a regular value. "Do not display then save" is sad, but nothing new here.

Patch is ready IMO, aside from the minor/optional comment nit in #102. RTBC :-)

Doing :

foreach (array_intersect($entity->getPropertyDefinitions(), $displays[$entity->bundle()]->getComponent()) as $field_name) {
  foreach ($entity->get($field_name) as $item) {

might save a couple cycles ?

#105: 1778122-105-formatter-attributes.patch queued for re-testing.

It sucks that image formatters modify the entity by adding a field value - if the entity was saved after being displayed (which usually doesn't happen of course, and node preview only displays a clone of the entity being edited), the "default image" would get saved as a regular value. "Do not display then save" is sad, but nothing new here.

Yep, unrelated to this issue, but #2078155: Access protected field items being removed has just fixed one occurrence of that, and #2089123: Centralize access check for formatters is open to see if we can do it better. Now that $items is an object, I'm concerned this problem might be quite a bit more widespread than it was in D7. Not sure if we want to introduce cloning for the entire view pipeline, or make a blanket rule that formatters and other view hooks should never modify.

Thanks for the benchmarks in #105, @scor.

patch #96: 0.830703306198 ms/operation on 100 fields
this patch: 0.698279190063 ms/operation on 100 fields

Not sure I get this - does it mean the difference is
0.830703306198-0.698279190063 ms ?
or (0.830703306198-0.698279190063) *100 ms ?

If the former, then the gain seems really marginal, and @effulgentsia's original code is slightly more understandable, so I'd vote for code clarity.
If the latter, then the CPU gain might be worth keeping.

Right, let's switch back to #86 then. Sorry for the noise :-)

Thanks ! Let's do this.

The $item->_attributes stuff is weird and disconcerting, but it's been discussed plenty on this issue and I don't have a better idea at the moment. Since it's an existing problem, this doesn't make it much worse and there's definitely a need for this.

Committed/pushed to 8.x, thanks!

Needs a change notice.

This added a module_enable() call in core/modules/system/lib/Drupal/system/Tests/Entity/EntityViewControllerTest.php

Manually confirmed the HEAD fail and that the patch fixes it.

Sorry. Committed/pushed to 8.x.

Committed and pushed that to 8.x too. :) Thanks!

.

Description

• Adds rdf_entity_prepare_view() - loops over each defined rdf mapping for the entity in question, adding rdf data to the $item->_attributes property.
• Adds $item->_attributes property to field items.
• Removes rdf_preprocess_field() and rdf_field_attach_view_alter()
• As hook_entity_prepare_view() runs prior to markup being generated by field formatters, field formatters now have the option of using rdf attributes rather than having to wrap fields with rdf markup in template files.
• In the case that the field formatter does not do anything with the rdf _attributes passed to it, they will be added to fields in the same way they were in Drupal 7.

Drupal 8: Field formatter passing item attributes to the theme function

// Pass field item attributes to the theme function.
if (isset($item->_attributes)) {
  $elements[$delta] += array('#attributes' => array());
  $elements[$delta]['#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);
}

Impacts:
* Module Developers

add note on performance

https://drupal.org/node/2155247 was posted in December, thanks @jesse.d!