Part of meta-issue #1788918: [GTD] [META] Prepare for Twig main engine core inclusion until December 1

Problem/Motivation

In Drupal 7 we were using PHP print statements in field.tpl.php to add our RDFa attributes around each field formatter. We typically had these attribute variables: $attributes, $title_attributes, $content_attributes and $item_attributes in node.tpl.php, field.tpl.php, etc. The latter $item_attributes was used to wrap each field item with the appropriate attributes in field.tpl.php. Here is the example of a text field output:

<?php
<div class="field field-name-body field-type-text-with-summary field-label-hidden">
    <
div class="field-items">
        <
div class="field-item even" property="schema:text">
            <!-- 
field formatter output is inserted here, without any way to add RDFa attributes. -->
        </
div>
    </
div>
</
div>
?>

Drupal 7's limitations

Placing the attributes on the item wrapper element worked for some core fields in D7, those which were outputting simple one value items (text or number). We started to feel the limit of this approach for fields like file or image where we were forced to duplicate the URL value on the item wrapper because it was not possible to inject the RDFa markup inside the field formatter in the same element as the file link. See for example this image field output (note how the file URL is repeated):

<?php
<div class="field-item even" property="og:image" resource="http://openspring.net/sites/openspring.net/files/styles/medium/public/8553992227_c90f23718a.jpg">
    <
a href="/week-ecuador-viva-drupal-latino"><img alt="" src="http://openspring.net/sites/openspring.net/files/styles/medium/public/8553992227_c90f23718a.jpg" /></a>
</
div>
?>

In an ideal situation, the RDFa attribute 'property' would be placed on the img element where the image URL is, but because D7 didn't have any way to achieve that, we had to resort to duplicating the image URL in the wrapping element. This markup is suboptimal. The ideal markup for D8 is to simply add one RDFa attribute (property="..") to the img element inside the field formatter:

<?php
<div class="field-item even">
    <
a href="/week-ecuador-viva-drupal-latino"><img alt="" property="og:image" src="http://openspring.net/sites/openspring.net/files/styles/medium/public/8553992227_c90f23718a.jpg" /></a>
</
div>
?>

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). There is only so much structured data you can cram into a single HTML element.

Lesson learned: do not solely rely on in the item wrapper to place RDFa attributes, it's not a generic, scalable solution.

As more field types like date and link are moved to core, it becomes crucial to find a solution to this problem, and be able to inject attributes in the HTML elements closer to the actual values inside the field formatters. For date.module, we want to be able to take advantage of the HTML5 time element and inject the RDFa property attribute in the time element, and for link, we want to be able to annotate both the URL and the link text. We can't achieve this without being able to inject RDFa attributes inside the field formatters.

Proposed resolution

Pass the RDFa attributes to the field formatter and give it a chance to place the attributes at the right place in HTML markup it generates. If the field formatter does not use these attributes and leave them set in the field item, the field template will inject them in the usual location (like it currently does in Drupal 7).

This issue focuses on the fields which were part of Drupal 7 and are currently tested in the RDF module: text, taxonomy_term, file, image. Other fields are covered in #2034951: [META] Support and test all field formatters RDFa output. Compound fields are covered in #1778194: RDF module can't handle compound fields. Note that this issue does not cover data like post date, etc., as those items are not provided by fields.

User interface changes

none

API changes (most of them are API additions)

- The logic for loading the mappings and generating the attributes in hook implementations rdf_preprocess_field() and rdf_field_attach_view_alter() is removed and replaced by a single rdf_entity_prepare_view() which generates the RDFa attributes for each field items about to be displayed.
- Field items have a new property 'html_data_attributes' ($item->html_data_attributes) which contains the attributes to be placed in the HTML output by the field formatters or the field template.
- Field formatters have the ability to inject RDFa attributes inside the HTML markup they generate. The RDFa attributes are generated by the RDF module in rdf_entity_prepare_view(), and placed in the new field item property html_data_attributes, which the field formatters can then inject at the right place in their HTML output. If a field formatter leaves html_data_attributes intact, these attributes will be inserted in the field template as it is currently the case in Drupal 7. If a field formatters makes use of the HTML data attributes, it should unset them so that they are not added in the field template.
- theme_file_link() accepts a new parameter 'attributes', which contains attributes to be placed in the a element (necessary for optimal RDFa markup for file field link.)
- The mapping type mapping_type: 'rel' no longer needs to be specified in the RDF mapping definitions. These were necessary in Drupal 7 in order to generate a different set of attributes for the field wrapper depending on the type of object contained in the field wrapper. mapping_type: 'rel' was used when the object in the field output was another resource (with a URI) such as a file, as opposed to a string/literal (e.g. text field). This is no longer necessary because we can now inject the attributes directly in the field formatter.

Performance

ab showed some small performance gain in some situations, see details in #52. Additionally, this patch also reduces the markup size in the case of file and image fields, since the URL does not need to be processed on the backend and repeated in the final HTML output.

Steps for reviewing the intended results of this patch

Here is a quick outline for patch reviewers. Viewing the intended effects is relatively simple, despite the far-reaching changes to code.

  1. On a fresh installation of Drupal 8, create an entity with contents in one or all of the following fields: text, taxonomy term, file, image.
  2. View the entity and save the source for these fields for later.
  3. Apply the most recent patch
  4. Reload the entity and compare the source to that from before the patch
Files: 
CommentFileSizeAuthor
#120 1778122-120.patch814 bytesbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 58,677 pass(es).
[ View ]
#117 module_enable-1778122-117.patch861 bytestim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,322 pass(es).
[ View ]
#113 1778122-113-formatter-attributes.patch28.87 KBscor
PASSED: [[SimpleTest]]: [MySQL] 58,819 pass(es).
[ View ]
#113 interdiff.txt1.44 KBscor
#109 1778122-109-formatter-attributes.patch28.7 KBscor
PASSED: [[SimpleTest]]: [MySQL] 58,857 pass(es).
[ View ]
#109 interdiff.txt3.69 KBscor
#105 1778122-105-formatter-attributes.patch25.01 KBscor
PASSED: [[SimpleTest]]: [MySQL] 59,228 pass(es).
[ View ]
#105 interdiff.txt1.44 KBscor
#96 1778122-96-formatter-attributes.patch25.18 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 59,266 pass(es).
[ View ]
#96 interdiff.txt1.16 KBeffulgentsia
#93 1778122-92-formatter-attributes.patch24.69 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 58,935 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#93 interdiff.txt2.19 KBeffulgentsia
#90 1778122-90-formatter-attributes.patch24.19 KBscor
FAILED: [[SimpleTest]]: [MySQL] 59,335 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#90 interdiff.txt7.67 KBscor
#82 1778122-82-formatter-attributes.patch22.61 KBscor
PASSED: [[SimpleTest]]: [MySQL] 59,323 pass(es).
[ View ]
#82 interdiff.txt4.03 KBscor
#73 1778122-73-formatter-attributes.patch22.39 KBscor
PASSED: [[SimpleTest]]: [MySQL] 58,434 pass(es).
[ View ]
#73 interdiff.txt4.27 KBscor
#69 1778122-69-formatter-attributes.patch22.27 KBscor
PASSED: [[SimpleTest]]: [MySQL] 58,376 pass(es).
[ View ]
#69 interdiff.txt8.44 KBscor
#66 1778122-66-formatter-attributes.patch26.21 KBscor
PASSED: [[SimpleTest]]: [MySQL] 58,036 pass(es).
[ View ]
#66 interdiff.txt1.42 KBscor
#62 1778122-62-formatter-attributes.patch25.92 KBscor
FAILED: [[SimpleTest]]: [MySQL] 58,443 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#62 interdiff.txt9.62 KBscor
#58 1778122-58-formatter-attributes.patch25.75 KBscor
PASSED: [[SimpleTest]]: [MySQL] 58,378 pass(es).
[ View ]
#58 interdiff.txt3.06 KBscor
#56 1778122-56-formatter-attributes.patch25.04 KBscor
PASSED: [[SimpleTest]]: [MySQL] 58,428 pass(es).
[ View ]
#56 interdiff.txt2.78 KBscor
#54 1778122-54-formatter-attributes.patch23 KBscor
PASSED: [[SimpleTest]]: [MySQL] 58,305 pass(es).
[ View ]
#54 interdiff.txt7.68 KBscor
#51 1778122-51-formatter-attributes.patch24.2 KBscor
PASSED: [[SimpleTest]]: [MySQL] 58,430 pass(es).
[ View ]
#51 interdiff.txt1.38 KBscor
#50 1778122-50-formatter-attributes.patch25.58 KBscor
PASSED: [[SimpleTest]]: [MySQL] 58,595 pass(es).
[ View ]
#50 interdiff.txt2.58 KBscor
#49 1778122-49-formatter-attributes.patch28.15 KBscor
PASSED: [[SimpleTest]]: [MySQL] 58,334 pass(es).
[ View ]
#49 interdiff.txt3.25 KBscor
#47 1778122-47-formatter-attributes.patch25.53 KBscor
FAILED: [[SimpleTest]]: [MySQL] 58,323 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#47 interdiff.txt9.41 KBscor
#45 1778122-45-formatter-attributes.patch18.05 KBscor
FAILED: [[SimpleTest]]: [MySQL] 58,269 pass(es), 15 fail(s), and 0 exception(s).
[ View ]
#45 interdiff.txt9.63 KBscor
#42 1778122-42-formatter-attributes.patch16.05 KBscor
FAILED: [[SimpleTest]]: [MySQL] 57,935 pass(es), 18 fail(s), and 0 exception(s).
[ View ]
#42 interdiff.txt706 bytesscor
#41 1778122-41-formatter-attributes.patch15.97 KBscor
FAILED: [[SimpleTest]]: [MySQL] 58,315 pass(es), 17 fail(s), and 0 exception(s).
[ View ]
#41 interdiff.txt3.88 KBscor
#37 1778122-37-formatter-attributes.patch13.75 KBscor
FAILED: [[SimpleTest]]: [MySQL] 57,904 pass(es), 18 fail(s), and 0 exception(s).
[ View ]
#35 1778122-35-formatter-attributes.patch16.49 KBscor
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1778122-35-formatter-attributes.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#35 interdiff.txt7.17 KBscor
#34 1778122-34-formatter-attributes_includes2034975-and-2031787.patch16.36 KBscor
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1778122-34-formatter-attributes_includes2034975-and-2031787.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#10 1778122-10-formatter-attributes_includes2034975-2031787-2034995.patch20.97 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] 56,975 pass(es), 21 fail(s), and 4 exception(s).
[ View ]
#10 1778122-10-formatter-attributes-do-not-test.patch13.27 KBlinclark
#10 interdiff.txt2.84 KBlinclark
#8 1778122-08-formatter-attributes_includes2034975-and-2031787.patch15.95 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] 56,481 pass(es), 24 fail(s), and 4 exception(s).
[ View ]
#8 1778122-08-formatter-attributes-do-not-test.patch8.93 KBlinclark

Comments

I was in the process of posting #1778194: RDF module can't handle compound fields while you were posting this. In that issue, I explain the additional problem that the RDF mapping can't nest field property mappings inside their fields.

I do agree that using Twig for this would be the best route, I talked to yched, effulgensia, and others about this concept at DrupalCon and AFAIK there weren't any concerns raised.

Issue summary:View changes

typo

Title:Provide a way for modules like RDF to inject attributes in Twig templatesProvide a way for modules like RDF to inject attributes in field formatters

The important part of this isn't the Twig template as much as the fact that the attributes are being injected in the field formatter. Changing title to reflect this.

I spoke with swentel and alexpott about this today. They both agreed that we should make sure that all of core's field formatters use theme functions (text doesn't at the moment) and then add the attributes in the twig templates.

Note that as per a conversation with effulgentsia a few days ago, July 1st is an API freeze and not a code freeze, so that means that API implementations can still happen after July 1st, but APIs themselves need to be committed before July 1st. Let's try to identify the bits that require API addition/change, and prioritize from there.

Single value fields with @property in field item

My initial thought was follow Lin's recommendation and to push all the attribute placeholders that we have in field.html.twig down to the field formatters. However after trying to implement this I found that this would make the markup of some of the basic value fields more complicated and would introduce additional HTML elements. In other words, the field item twig file provides (in some cases) the most appropriate place to add attributes. Consider for example the telephone field. I created a field of type telephone to see the markup Drupal generates at the moment. This is what I added to my rdf.mapping.node.article.yml (using a made-up dc:telephone property):

  field_telephone:
    properties:
      - 'dc:telephone'

It generated this markup:

Plain text formatter:

<?php
<div class="field-items"><div property="dc:telephone" class="field-item even">800 123 1234</div></div>
?>

Telephone link formatter:

<?php
<div class="field-items"><div property="dc:telephone" class="field-item even"><a href="tel:8001231234">800 123 1234</a></div></div>
?>

Both snippets generate the expected output, which extracts the telephone value (not the @href tel: value). Note that schema.org expects a "Text" type here, which is what we generate.

If we were to push the property attribute inside the field formatter, we would need to add a wrapping span HTML element around the number in the plain text formatter, or a span element inside the a element for the link formatter. By injecting the property attribute at the field level, we don't need to add additional HTML elements.

The email field is similar. Email Plain text formatter:

<?php
<div class="field-items"><div property="dc:email" class="field-item even">hello@example.com</div></div>
?>

Email field formatter:

<?php
<div class="field-items"><div property="dc:email" class="field-item even"><a href="mailto:hello@example.com">hello@example.com</a></div></div>
?>

(same conclusion as telephone above, schema.org expects the text value, not a mailto: value, so we are good).

Other fields which are working with field item:
- longtext, though longtext could be considered hybrid-compound because it has multiple values (default and trimmed) but only one is displayed at a given time. Offering the ability to define different mappings is dealt with in a separate issue.
- text

Other fields with attributes in their field formatter

On the other hand, some fields require to have the attributes added inside the field formatter:
- Date: the default field formatter output a time element where we want to add the property attribute.
- File: this is a single value field (the URI of the file) and we want to add @property in the a element of theme_file_link. Note however that file theme functions haven't been converted to Twig yet (#1898070: file.module - Convert theme_ functions to Twig). In D7 we added @property in the field item tpl with @resource file URI.
- Image: same as file. Image hasn't been converted to Twig yet: #1939068: Convert theme_image() to Twig.
- Link: similar to file. We want to add @property in the a element to capture @href. When "URL only" and "Show URL as plain text" as checked, @property should be added to the field item because the field formatter outputs a bare URI value. Link hasn't been converted to Twig yet: #1898426: link.module - Convert theme_ functions to Twig.
- Compound fields in general (addressfield, voting API).

Conclusion

Given the analysis above, I would suggest to have a dual approach where field formatters can decide whether the RDFa attributes should be pushed up to the field item template, or injected in the field formatter output. By default, unless a field formatter specifies one way or the other, no attribute would be added anywhere, this would prevent malfunctioning RDFa markup to be generated like in D7 where @property can get added to the field item when there is no valid value in the field formatter. This would solve #1778034: Field formatter developers can easily pollute a site's RDFa output.

This is the API we should hash out here, and implementations can be done post July 1st. I'm going to leave it at that for this comment as it's getting pretty late here. More tomorrow. In the meantime maybe others can say whether they agree and start tossing ideas on how to do this.

I think that we want to consider placing the attributes in the templates for all field properties. For example, the 'value' property for text fields would be placed in the field formatter. This makes the API consistent.

ok, so I think I found a good compromise. The hybrid approach I was talking about in #4 is a nice to have, but not mission critical. On the other hand we need to inject markup in field formatters, so let's focus on that in this issue and add a wrapping element in the field formatter when needed. Deal?

Could we make the additional wrapper optional (aka not hard-coded / forced) in the field formatter so that it is not added when there is no mapping, and so it can be disabled if a contrib wanted to implement the hybrid approach above?

I would suggest to leave the implementations of this API in separate issues, so that we can move quicker on this issue which is the one bringing API changes in core.

Yes, I was thinking that the placement of attributes would be conditional. We wouldn't want additional markup to be added if RDF was not enabled... and, while I hadn't thought of it, your suggestion not to add additional markup if there is no mapping makes sense.

Status:Active» Needs review
StatusFileSize
new8.93 KB
new15.95 KB
FAILED: [[SimpleTest]]: [MySQL] 56,481 pass(es), 24 fail(s), and 4 exception(s).
[ View ]

So this is just a quick POC of a possible solution. It introduces a hook_formatter_prepare_view, which gives RDF module the chance to add the HTML data attributes it needs to.

The only formatters that have been altered to demonstrate the pipeline are TextPlainFormatter and MailToFormatter. You can see the passing EmailFieldRdfaTest, which tests these two formatters. The email formatter doesn't actually need this more fine-grained attribute placement, it's already exposing data correctly without any hackery, but it was the easiest to test and demonstrate with.

The patch also removes rdf_preprocess_field, which might come back later once we have an API for handling compound fields, and rdf_field_attach_view_alter, which engaged in some hackery and should be rendered unnecessary by this work. Many tests will fail because these were removed.

I expect that the patch will negatively impact performance. I'm open to suggestions for improving the performance. However, I don't think that we can accept the status quo as an option. We need to ensure that the data that we expose is more reliable than it is in D7.

This patch depends on #2034975: Test RDFa output in email formatter and #2031797: Provide a RDF wrapper twig template for renderable variables + attributes.

Status:Needs review» Needs work

The last submitted patch, 1778122-08-formatter-attributes_includes2034975-and-2031787.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.84 KB
new13.27 KB
new20.97 KB
FAILED: [[SimpleTest]]: [MySQL] 56,975 pass(es), 21 fail(s), and 4 exception(s).
[ View ]

This patch demonstrates how the solution would work for taxonomy links. Because we insert the attributes into the <a> tag itself, we no longer need to use the 'rel' property, which we are trying to get away from as explained in #1780090: Decide: Use RDFa Lite's property attribute, or give users choice.

The interdiff only includes the changes relevant to changing how the attributes are placed, it does not include the merge I did to bring the taxonomy test into this patch.

Status:Needs review» Needs work

+++ b/core/modules/rdf/lib/Drupal/rdf/Tests/Field/TaxonomyTermReferenceRdfaTest.php
@@ -84,19 +84,28 @@ public function setUp() {
+    $this->_testFormatter('taxonomy_term_reference_plain', 'http://schema.org/about', $term_uri, 'uri');

we might want to follow recommendations from #2036765: Drupal\rdf\Tests\StandardProfileTest needs clean up regarding the test method name.

+++ b/core/modules/rdf/rdf.module
@@ -221,6 +221,33 @@ function rdf_theme() {
+  // Get the field mapping for this field.

s/Get/Gets

+++ b/core/modules/rdf/rdf.module
@@ -221,6 +221,33 @@ function rdf_theme() {
+  // Attach the prepared RDF attributes to the item. These attributes will be

s/Attach/Attaches

+++ b/core/modules/rdf/rdf.module
@@ -221,6 +221,33 @@ function rdf_theme() {
+      // Add an about attribute to create the relationship between the field

s/Add/Adds

+++ b/core/modules/rdf/rdf.module
@@ -221,6 +221,33 @@ function rdf_theme() {
+      $item['html_data_attributes']['about'] = url($uri_info['path']);

let's use the 'resource' attribute here which is part of RDFa Lite.

As far as I can tell, the MailTo and TextPlain formatters add the wrapper no matter whether there are RDF mappings, right, or did I miss something? Is there a way to only add the wrapper if there are going to be attributes in it?

This is a Proof of Concept, so I was hoping for some architectural feedback. For example, it adds a new hook as I mention in #8. I'm not sure whether we want to do this, or whether there are better alternatives.

+++ b/core/includes/common.inc
@@ -3482,6 +3482,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;
+  }

I'm curious, don't we have any use case of this in core yet? I guess not since they would break without this code. What happens right now if a module passes an array in #title? Surely RDF is not an isolated use case for having markup in a link title.

The content model of the <a> element changed in HTML5. It didn't allow nested block elements before.

EDIT: clarified that it didn't allow nested block elements before

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)

Thanks for taking a look, yched.

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() ?

This makes sense to me.

More generally, adding those extra properties directly in the $items held by the entity is a bit hackish.

I agreee. Either of your suggestions sound good to me, though I would lean towards the second. I'll try it in the next patch.

I was looking at how to move the logic into field_attach_prepare_view, but it looks like field_attach_prepare_view is deprecated for D8. yched and I discussed this at NYC Camp and agreed that we should get @fago or @berdir to look at this, as yched explains in #16.

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 ?

Title:Provide a way for modules like RDF to inject attributes in field formattersEnable modules to inject attributes into field formatters, so that RDF attributes get output on the appropriate elements
Category:feature» task
Priority:Normal» Major

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.

Issue summary:View changes

Updated issue summary.

@effulgentsia: I've updated the issue summary (see limitations section), let me know if it's more clear. Bottom line is we should move away from using the wrapper to place our RDFa attributes.

Issue summary:View changes

explains more about D7's limitations

  <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?

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?

I agree with you. That would depend on individual field formatters, some of them already output a wrapper no matter what. Address field for example already wraps each individual values in a span, for example:

<?php
<span class="postal-code">01821</span>
?>

we would take advantage of that and add our RDFa attribute in that same existing span. Some fields such as the text field don't have a wrapper inside the field formatter, so in this case it would be optionally added when RDF is enabled. The patch #10 already has this concept of html_data_wrapper.

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 ?

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.

you're spot on, Yves. I like this approach. rdf_entity_view_alter() could populate the appropriate attributes for each field formatter value (the column values, not sure what they are called now in D8), and the field formatter would be responsible for adding these attributes at the right place in the HTML output. The field formatter could check whether there are attributes or not for a given value, and may decide to not wrap the value if there is no attribute to add. I'm not sure whether it would be up to the RDF / 3rd party module to navigate the render array to find all the column values, or alternatively, there could be a dedicated "value_attributes" array/registry where 3rd party module could add attributes for each rendered value, and the field formatter would pick the attributes from this array/registry when rendering. Here is a quick and dirty untested POC of the second idea for an address field (combining column name + CSS classes from the D7 addressfield and schema.org properties):

<?php
// column value attributes which were populated by 3rd party modules.
// using old school arrays for easier reading, but should use Twig Attribute().
$value_attributes['country'] = array(
   
'class' => array('country'),
   
'property' => array('schema:addressCountry'),
),
$value_attributes['postal_code'] = array(
   
'class' => array('postal-code'),
   
'property' => array('schema:postalCode'),
),
$value_attributes['locality'] = array(
   
'class' => array('locality'),
   
'property' => array('schema:addressLocality'),
),
?>

<?php
// field formatter output built in field formatter viewElements() method,
// pulling attributes from the value_attributes array.
$output .= '<span ' . drupal_attributes($value_attributes['country']) . '>' . drupal_render($country) . '</span>';
$output .= '<span ' . drupal_attributes($value_attributes['locality']) . '>' . drupal_render($locality) . '</span>';
// optionally decide if a wrapper is needed or not for a particular value.
if (empty($value_attributes['postal_code'])) {
   
$output .= drupal_render($postal_code);
}
else {
   
$output .= '<span ' . drupal_attributes($value_attributes['postal_code']) . '>' . drupal_render($postal_code) . '</span>';
}
?>

I'm using a generic concept of 'value attributes' here since modules might want to add CSS classes or other attributes beyond semantic RDFa or HTML data.

So one problem is that many formatters do not use theme functions. For example, theme_link isn't actually used for most core formatters that output links, but instead '#type' => 'link', which results in a call to l() if I'm not mistaken.

If we feel OK about converting ALL formatters to use formatter-specific templates/theme functions, then this would work. However, we would need to verify that that change is acceptable.

so, I misread yched's comment. My assumption was that rdf_entity_view_alter() would run before field formatters run their viewElements(), which is not the case. 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? Then viewElements() could use these attributes as needed (see my example in #25).

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...

These are excellent questions, yched.

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.

yes, I think this is essential to solving this issue, and it allows the field formatter to be smart about deciding to add a wrapper if there are attributes. This is also required because as pointed out in #26, some field formatter don't use Twig templates, so there might not be a way to add these attributes at a later stage in the rendering process.

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 can't think of a field type where the attributes would differ based on the formatter. So yes, I think it's safe to assume that the attributes for all values can be prepared before we hit the formatter, and then the formatter can place these where it makes sense. So the placement of the attributes might differ from one formatter to the other, but the attributes themselves would remain the same for all formatters.

- for a date field, do the exact same attributes make sense whether the date is displayed as a human readable, ISO, timestamp... ?

I just checked and all the date field formatters use the HTML5 time element with the datetime attribute already prefilled with the right machine readable value, so we would just need add our 'property' attribute to that element (no matter which formatter is selected).

- 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... ?

We would markup the URL of the image, whichever size it rendered. The attributes would not include the URL of the image, only the mapping (e.g. schema:image), so in this case also, the attributes are the same and formatter agnostic. The contract given with these value attributes is this: here is the attributes that should be placed on the HTML element that contains the image URL. The formatter is in charge of placing these attributes (more likely one 'property' attribute in this particular case) on the right HTML element. If it's a large image with no link, place the RDFa attribute in the img element. If it's the original image with a link, place the RDFa attributes in the img element (ignore the a element). If it's a link to the image file, place the attributes in the a element.

In a nutshell, I think all your assumptions are sound, yched.

Now, to the hook used to populate these attributes, I guess one possibility is hook_entity_prepare_view() like you said in #28. 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.

Another alternative would be to invoke a new hook at the beginning of viewElements(), maybe called hook_formatter_value_attributes() - better naming welcome. We would need the field data and the entity passed in the hook, from there RDF could do its work. This hook would be invoked for each field that is being rendered.

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.

I agree with yched.

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.

StatusFileSize
new16.36 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1778122-34-formatter-attributes_includes2034975-and-2031787.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This is a straight reroll of #10.

Assigned:Unassigned» scor
Category:task» bug
Status:Needs work» Needs review
StatusFileSize
new7.17 KB
new16.49 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1778122-35-formatter-attributes.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Per #20, moving this to bug report because (1) file and image fields can't support RDFa Lite markup without this and (2) date field is currently hard-coding its RDFa attributes in its formatter.

I removed hook_formatter_prepare_view() which was introduced earlier in this issue, and use the existing hook_entity_prepare_view() instead which is only called once per entity. I added support for TextDefaultFormatter and updated the other formatters to use the new attributes which are now added by rdf_entity_prepare_view() to each property of the entity. The RDF module adds its attributes to each $property->html_data_attributes. I'd appreciate some feedback on whether there is a better place to store them during the rendering.

I need to update the tests next.

Status:Needs review» Needs work

The last submitted patch, 1778122-35-formatter-attributes.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new13.75 KB
FAILED: [[SimpleTest]]: [MySQL] 57,904 pass(es), 18 fail(s), and 0 exception(s).
[ View ]

oops, forgot to remove FieldRdfaTestBase.php from the patch. Here is a patch that applies.

Status:Needs review» Needs work

The last submitted patch, 1778122-37-formatter-attributes.patch, failed testing.

+++ 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.

Status:Needs work» Needs review
StatusFileSize
new3.88 KB
new15.97 KB
FAILED: [[SimpleTest]]: [MySQL] 58,315 pass(es), 17 fail(s), and 0 exception(s).
[ View ]

The entity_test entity type does not have a render controller defined, so I switched to a node based test class to be able to test the formatters. email formatter tests are passing locally.

StatusFileSize
new706 bytes
new16.05 KB
FAILED: [[SimpleTest]]: [MySQL] 57,935 pass(es), 18 fail(s), and 0 exception(s).
[ View ]

address comments #39 and #40 by using '#type' => 'html_tag' in theme_html_data_wrapper().

Status:Needs review» Needs work

The last submitted patch, 1778122-42-formatter-attributes.patch, failed testing.

  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?

Status:Needs work» Needs review
StatusFileSize
new9.63 KB
new18.05 KB
FAILED: [[SimpleTest]]: [MySQL] 58,269 pass(es), 15 fail(s), and 0 exception(s).
[ View ]

@Fabianx: thanks for the review :) Addressed all your comments, see also below.

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

For this particular case of MailToFormatter, $elements[$delta] is initialized just in the code above as an array, so there is no chance for $elements[$delta]['#attributes'] to already be set.

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

It depends if we can use an element that the formatter already generates. In the case of a taxonomy term link or email link, we can directly add our RDFa attributes to the a element. It's also the case for the telephone field for example, which will be dealt with in #2034951: [META] Support and test all field formatters RDFa output. But in the case of text, there is typically no wrapper so that's when we need to add our own.

In this patch I'm also adding TaxonomyTermReferenceRdfaTest.php from #10 in order to keep tests in sync with the formatters which are being updated in this patch.

Status:Needs review» Needs work

The last submitted patch, 1778122-45-formatter-attributes.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.41 KB
new25.53 KB
FAILED: [[SimpleTest]]: [MySQL] 58,323 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

- updated rdf_entity_prepare_view() to support multiple field items
- added support for taxonomy term reference as plain text
- disabled the tests for the type and title of referenced taxonomy terms. It's something that Drupal 7 outputs, I've created this follow up issue: #2072791: Output RDFa markup for type + title in taxonomy term reference and entity reference formatters. Let's keep the current issue on basic field formatters.
- added support for image and file field formatters. The latter required to add a new key to theme_file_link() in order to allow the formatter to pass some attributes to be added to the a element.
- removed mapping type rel from standard profile since we no longer rely on it in the formatters.

Status:Needs review» Needs work

The last submitted patch, 1778122-47-formatter-attributes.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.25 KB
new28.15 KB
PASSED: [[SimpleTest]]: [MySQL] 58,334 pass(es).
[ View ]

- disabled the tests for the type and title of referenced taxonomy terms in the standard profile test (to be handled in #2072791: Output RDFa markup for type + title in taxonomy term reference and entity reference formatters)
- added support for TextTrimmedFormatter formatter (teaser)

Issue summary:View changes

add anchor

Issue summary:View changes

reduce the summary to focus on what's relevant for this isssue, and switch focus from Twig templates to field formatters (since most field formatters are in fact not Twig templates).

StatusFileSize
new2.58 KB
new25.58 KB
PASSED: [[SimpleTest]]: [MySQL] 58,595 pass(es).
[ View ]

Let's handle the new fields in #2034951: [META] Support and test all field formatters RDFa output. I've removed the email field hunks from this patch, in order to focus on the older fields that were present in Drupal 7 and are already tested in rdf.module.

Issue summary:View changes

issue coverage

Issue summary:View changes

add reference to compound field issue

Issue summary:View changes

Updated issue summary.

StatusFileSize
new1.38 KB
new24.2 KB
PASSED: [[SimpleTest]]: [MySQL] 58,430 pass(es).
[ View ]

There were some leftover calls to parent::prepareView() which I removed in this patch. They were first introduced with the hook_formatter_prepare_view() which is no longer used.

I ran some performance benchmark locally measuring page load times with ab and devel_generate, and notice a tiny performance improvement on frontpage with many nodes, and no impact on pages with lots of comments.

50 nodes on the frontpage, each with an image, a body and a text field with 10 items (average times on 3 runs of ab -n 100):
8.x = 803.5ms
w/ patch = 791.5ms
impact: 1.5% faster

node with 50 comments (average times on 3 runs of ab -n 100):
8.x = 1256ms
w/ patch = 1258ms
impact: 0%

I suspect the performance gain is due to removing the calls to hook_preprocess_HOOK() for field.tpl.php, since we now only call hook_entity_prepare_view() once for each entity.

  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.

StatusFileSize
new7.68 KB
new23 KB
PASSED: [[SimpleTest]]: [MySQL] 58,305 pass(es).
[ View ]

Thanks yched for this review. Fixed all your remarks except the ones discussed below.

I was able to use entity_test_render instead of node, thanks to larowlan for pointing this out.

+++ 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 ?)

Yes, this is not specific to this patch. I've created #2076769: Optimize rdf_get_mapping() performance to track this (see my remarks in the issue summary).

+++ 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.

So far only text fields have required this wrapper, so I'm not sure if it will be that common across all formatters. Also, depending on the formatter, you might want to use a different tag (div or span), so the method would have to have this as paramter. We had a theme function earlier, but I'm not sure this abstraction level brings us much benefits (#39). Let's revisit this once we're covered all formatters, I've added your suggestion as a comment in the relevant META issue.

+++ 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 ?

Are you suggesting to attach the mappings to the field and dispatch them to each item in the formatter? I can roll a new patch with that approach.

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**);

StatusFileSize
new2.78 KB
new25.04 KB
PASSED: [[SimpleTest]]: [MySQL] 58,428 pass(es).
[ View ]

Thanks again, @yched. Fixed both issues in attached patch, and added a test for the value passed to rdf_rdfa_attributes().

+++ 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 ?

StatusFileSize
new3.06 KB
new25.75 KB
PASSED: [[SimpleTest]]: [MySQL] 58,378 pass(es).
[ View ]

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.

Good point, I've fixed that in the attached patch. 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.

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.

I'm not sure this is necessary because the html_data_attributes are unlikely to change in most cases since the mappings remain the same (we don't support changing these on the fly during the same request). The only case I can think of is if we use datatype_callback which adds the value variant into a @content attribute. 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? (in which case the attribute would inherit the right up to date values).

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 ?

could you explain how that would work? if you have already rendered an entity and you change its values and render it again (in same or a different view mode), rdf_entity_prepare_view() will be run again, and the html_data_attributes will be re-applied appropriately.

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")

Issue summary:View changes

Updated issue summary.

Issue tags:+API change, +Field API

@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

Issue summary:View changes

API changes

Issue tags:+RDF
StatusFileSize
new9.62 KB
new25.92 KB
FAILED: [[SimpleTest]]: [MySQL] 58,443 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

thanks again @yched for the thorough reviews! I've fixed all your code styles remarks from #60 (plus some minor docs improvements). See below for my answers to your other questions from the last 3 comments.

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.

It's still unclear how to best achieve that, so I've created a followup issue: #2079745: Clean up html_data_attributes after entity rendering has happened.

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 ?

That test change is because I reused an existing datatype callback for the initial patch, but it didn't work so well for handling field items like you had suggested to pass to the callback, instead of a string like I originally used in #56.

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")

As you noted, I've updated the issue summary, see the Proposed resolution and API changes sections.

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

No, at least not without further consideration, which should take place in a follow up issue. theme_field() / field.html.twig only include placeholders in case modules (like rdf in D7) want to inject attributes in specific location in the field wrapper output. While it's true that rdf.module will no longer need to do that for fields with this patch, other module might. All templates (block, node, comment, field) were normalized in D7 to have the $attributes, $title_attributes and $content_attributes there tpl for consistency, so if we were to remove them from theme_field(), we should also remove them from the other tpls. However other modules are actually using these for other purposes, for example title_attributes is used by overlay and search (D8). I'm sure D7 contrib has found use for these too, so this would be another API change. There is no logic in theme_field() regarding these placeholders, it's pretty dumb. I'm open to be proven wrong here, but we should probably discuss this in another issue if someone thinks it's worth removing any or all of these generic template attribute placeholders.

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

correct, it's theme_file_link(), I've updated the summary ;)

Status:Needs review» Needs work
Issue tags:-RDF, -Field API

The last submitted patch, 1778122-62-formatter-attributes.patch, failed testing.

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 ?

Issue tags:+RDF, +Field API

(re-adding tags that the bot removed)

re #64: I'm not convinced we should do this. For one it would complicate the API, and provide two ways for adding RDFa, which means developers might be potentially confused on which one to choose. Secondly, we decided to move away from using the field template because it was not convenient for most fields (see summary and #4). If the formatter hasn't output any RDFa markup, then it's best to not let the field wrapper output some "default" RDFa, because chances are it's going to be wrong. There is no silver bullet as we've learnt in D7, and it's best not to add any markup at all instead of doing guess work and asserting wrong statements in RDFa in the field template. The only field which would make sense to have its RDFa attributes in the field template is the text field. IMO the only case where RDFa attributes would make sense to be added to the field template is if the formatter asked to do so, see my hybrid approach in the conclusion of #4. The advantage of keeping everything in the field formatter is that we don't need to rely on the field template (which could be altered / overridden by developers). Either way, do you agree this should be discussed and possibly developed in a follow up issue?

- 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

Could that workflow even be possible? can the field template execute logic after the field formatter code has been executed?

Status:Needs work» Needs review
StatusFileSize
new1.42 KB
new26.21 KB
PASSED: [[SimpleTest]]: [MySQL] 58,036 pass(es).
[ View ]

patch #62 failed to due to #1497374: Switch from Field-based storage to Entity-based storage. This new patch makes the necessary adjustments to the field entity creation code in the tests.

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.

This sounds reasonable to me. Even though it depends on some implicit interactions between the two parts of the system, I think it is better than other options.

So +1 to #67.

Issue summary:View changes

correct name of them function: theme_file_link()

StatusFileSize
new8.44 KB
new22.27 KB
PASSED: [[SimpleTest]]: [MySQL] 58,376 pass(es).
[ View ]

Thanks for clarifying yched, I now understand your idea better and I'm starting to like it! As a bonus, it allows to implement the hybrid approach I explained in the conclusion of #4, so that's great. I've implemented it in the attached patch. Implementation is simpler than I thought, let me know if that's what you had in mind.

Status:Needs review» Reviewed & tested by the community
Issue tags:-API change+API addition

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)

Status:Reviewed & tested by the community» Needs work

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"?

Issue summary:View changes

explain hybrid approach

Status:Needs work» Needs review
StatusFileSize
new4.27 KB
new22.39 KB
PASSED: [[SimpleTest]]: [MySQL] 58,434 pass(es).
[ View ]

Thanks for all your help & guidance on this issue yched, I think that with this hybrid approach, we got the best of both approaches!! (field tpl and field formatters)

Thanks Wim for checking that this patch doesn't break in-place editing ;) I don't think anyone had checked that.

This patch fixes the code style issues reported by Wim and yched.

Status:Needs review» Reviewed & tested by the community

Yay. Back to RTBC then.

RTBC+1

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

yes, see #70.

Alright :) Definitely RTBC then.

Issue summary:View changes

API changes (most of them are API additions)

Issue summary:View changes

refining

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.

Thanks for chiming in, Alex! Regarding the naming: we're using the concept of "HTML data" as a broad concept, which not only includes the custom application-specific data-* attributes (used by edit), but also all the syntaxes that allow to embed structured data in HTML for third party consumption such as RDFa, microdata, microformats and microformats-2, as discussed in the HTML data guide W3C Note. html_data_attributes can be used for all HTML Data attributes, not only for the HTML data attributes. Like you said, if someone comes up with a better name, we can probably handle that in a follow up.

Status:Reviewed & tested by the community» Needs work

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".

Status:Needs work» Needs review
StatusFileSize
new4.03 KB
new22.61 KB
PASSED: [[SimpleTest]]: [MySQL] 59,323 pass(es).
[ View ]

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?

You're right, the comment was misleading. I've updated the comments of the 3 formatters which inject the HTML data attributes to the following:

  // Unset HTML data attributes since they've been added to the
  // formatter and thus should not be rendered in the field template.

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?

good catch, fixed GenericFileFormatter to also initialize the array.

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

What yched said. I've expanded on the comment above, let me know it's more clear.

  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 ?

Status:Needs review» Needs work

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 ?

I would be fine with item_attributes, it removes the ambiguity around HTML data attribute and is a more generic name. Note that item_attributes is already a variable in the field template. I guess it is fine to have both, and they should not collide since the new item_attributes (ex html_data_attributes) would be in the field item, which would be merged into the item_attributes in template_preprocess_field().

If people are fine with item_attributes, I'll roll a patch with @yched and @effulgentsia suggestions.

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... :-(

Status:Needs work» Needs review
StatusFileSize
new7.67 KB
new24.19 KB
FAILED: [[SimpleTest]]: [MySQL] 59,335 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

I like _attributes. In this patch:
- rename html_data_attributes to _attributes
- initialize _attributes in EntityRenderController
- rdf_preprocess_field() has been removed and its code has been placed in template_preprocess_field()

I left a todo in template_preprocess_field() for us to resolve here: should we use $variables['items'] instead of $element['#items']? I'd be curious to know the difference between two.

I still have to write a test for hook_entity_prepare_view() and _attributes, but wanted to post an update to show you how the new code flow looks. I think it's getting better and better. thanks for your continued reviews!

$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 ?

Status:Needs review» Needs work

The last submitted patch, 1778122-90-formatter-attributes.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.19 KB
new24.69 KB
FAILED: [[SimpleTest]]: [MySQL] 58,935 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 1778122-92-formatter-attributes.patch, failed testing.

The NodeTypeInitialLanguageTest fails because of this error:

Error message
InvalidArgumentException: Field language is unknown. in Drupal\Core\Entity\EntityNG->getTranslatedField() (line 299 of /Users/stephane.corlosquet/htdocs/d8-dev/core/lib/Drupal/Core/Entity/EntityNG.php).

which comes when $name is equal to 'language' in:

<?php
foreach ($displays[$entity->bundle()]->getComponents() as $name => $options) {
    foreach (
$entity->get($name) as $item) {
        ..
    }
}
?>

because language is set to be displayed in the node output by testLanguageFieldVisibility() in NodeTypeInitialLanguageTest.php.

The FieldUpgradePathTest fails because of this error:

Error message
InvalidArgumentException: Field test_deleted_field is unknown. in Drupal\Core\Entity\EntityNG->getTranslatedField() (line 299 of /Users/stephane.corlosquet/htdocs/d8-dev/core/lib/Drupal/Core/Entity/EntityNG.php).

This happens when we do this in EntityRenderController::buildContent():

<?php
foreach ($displays[$entity->bundle()]->getComponents() as $name => $options) {
    foreach (
$entity->get($name) as $item) {
        ..
    }
}
?>

$name ends up being a deleted field at some point in FieldUpgradePathTest and $entity->get($name) fails.

It seems that both fails above could be solved if we could somehow ensure that we only initialize these attributes on actual fields. Is there a way to run some kind of $entity->getFields()? There is getProperties() but that returns everything.

The reason why these fails didn't show up before is because we were not initializing _attributes, and only setting it if an RDF mapping was defined for the field.

Status:Needs work» Needs review
StatusFileSize
new1.16 KB
new25.18 KB
PASSED: [[SimpleTest]]: [MySQL] 59,266 pass(es).
[ View ]

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.

Status:Needs review» Needs work
Issue tags:-RDF, -API addition, -Field API

The last submitted patch, 1778122-96-formatter-attributes.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+RDF, +API addition, +Field API

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?

Status:Reviewed & tested by the community» Needs review

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 ?

Status:Needs review» Reviewed & tested by the community

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 :-)

Status:Needs review» Reviewed & tested by the community

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 ?

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new1.44 KB
new25.01 KB
PASSED: [[SimpleTest]]: [MySQL] 59,228 pass(es).
[ View ]

I'm posting a patch with yched's approach. I compared the time it takes to process 100 fields for both approaches, and I found a tiny improvement with the new approach:
patch #96: 0.830703306198 ms/operation on 100 fields
this patch: 0.698279190063 ms/operation on 100 fields

Might not be measurable on entities with a small amount of fields, but it might be worth it anyways?

Status:Needs review» Needs work
Issue tags:-RDF, -API addition, -Field API

The last submitted patch, 1778122-105-formatter-attributes.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+RDF, +API addition, +Field API

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.

StatusFileSize
new3.69 KB
new28.7 KB
PASSED: [[SimpleTest]]: [MySQL] 58,857 pass(es).
[ View ]

I added a test for the _attributes element in hook_entity_prepare_view() as requested by @effulgentsia in #84. EntityFieldTest looked like a good home for this test, but it sets up the entity_test entity type which does not have a render controller. So I used EntityViewControllerTest instead.

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.

The patch #105 is 0.13ms faster on a bulk of 100 fields. So that is 0.0013 ms faster per field on average, which is negligible for bundles with a low amount of fields. I agree #96 makes the logic more explicit and is more readable.

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

StatusFileSize
new1.44 KB
new28.87 KB
PASSED: [[SimpleTest]]: [MySQL] 58,819 pass(es).
[ View ]

reverted back to #96 while keeping the new test from #109.

Status:Needs review» Reviewed & tested by the community

Thanks ! Let's do this.

Title:Enable modules to inject attributes into field formatters, so that RDF attributes get output on the appropriate elementsChange notice: Enable modules to inject attributes into field formatters, so that RDF attributes get output
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

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.

Title:Change notice: Enable modules to inject attributes into field formatters, so that RDF attributes get outputHEAD broken: Enable modules to inject attributes into field formatters, so that RDF attributes get output
Priority:Major» Critical

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

Status:Active» Needs review
Issue tags:-RDF, -Needs change record, -Field API
StatusFileSize
new861 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,322 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

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

Title:HEAD broken: Enable modules to inject attributes into field formatters, so that RDF attributes get outputChange notice: Enable modules to inject attributes into field formatters, so that RDF attributes get output
Priority:Critical» Major
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Sorry. Committed/pushed to 8.x.

Title:Change notice: Enable modules to inject attributes into field formatters, so that RDF attributes get outputEnable modules to inject attributes into field formatters, so that RDF attributes get output
Status:Active» Needs review
StatusFileSize
new814 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,677 pass(es).
[ View ]

durp durp durpal:

<?php
/**
* Implements hook_entity_prepare_view().
*/
function entity_test_entity_prepare_view($entity_type, array $entities, array $displays) {
 
// Add a dummy field item attribute on field_test_text if it exists.
 
if ($entity_type = 'entity_test_render') {
    foreach (
$entities as $entity) {
      if (
$entity->getPropertyDefinition('field_test_text')) {
        foreach (
$entity->get('field_test_text') as $item) {
         
$item->_attributes += array('data-field-item-attr' => 'foobar');
        }
      }
    }
  }
}
?>

pretty sure that check on entity_type is missing a '='? i got here because #1605290: Enable entity render caching with cache tag support starts failing on on this code after this patch.

attached patch adds the '='.

Title:Enable modules to inject attributes into field formatters, so that RDF attributes get output[Followup] Enable modules to inject attributes into field formatters, so that RDF attributes get output
Status:Needs review» Reviewed & tested by the community

Title:[Followup] Enable modules to inject attributes into field formatters, so that RDF attributes get outputEnable modules to inject attributes into field formatters, so that RDF attributes get output
Status:Reviewed & tested by the community» Fixed

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

Title:Enable modules to inject attributes into field formatters, so that RDF attributes get outputChange notice: Enable modules to inject attributes into field formatters, so that RDF attributes get output
Status:Fixed» Active

.

Status:Active» Needs review

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

Issue summary:View changes

add note on performance

Category:Bug report» Task
Issue summary:View changes

#124 looks good to me. Jesse, could you post that as a change notice please? https://drupal.org/list-changes

Title:Change notice: Enable modules to inject attributes into field formatters, so that RDF attributes get outputEnable modules to inject attributes into field formatters, so that RDF attributes get output
Assigned:scor» Unassigned
Status:Needs review» Fixed
Issue tags:-Needs change record

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

Status:Fixed» Closed (fixed)

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