Problem/Motivation

We are eliminating the process layer from Drupal 8, since the reasons for it's existence can now be solved in different ways. One of the functions on the chopping block is template_process_field(). Since all this function does is add attributes, now renderable Attribute objects, these variables can safely be moved to the preprocess layer.

function template_process_field(&$variables, $hook) {
  static $default_attributes;
  // The default theme implementation is a function, so template_process() does
  // not automatically run, so we need to flatten the classes and attributes
  // here. For best performance, only instantiate Drupal\Core\Template\Attribute
  // when needed, and note that template_preprocess_field() does not initialize
  // the *_attributes variables.
  if (!isset($default_attributes)) {
    $default_attributes = new Attribute;
  }
  $variables['attributes'] = isset($variables['attributes']) ? new Attribute($variables['attributes']) : clone $default_attributes;
  $variables['title_attributes'] = isset($variables['title_attributes']) ? new Attribute($variables['title_attributes']) : clone($default_attributes);
  $variables['content_attributes'] = isset($variables['content_attributes']) ? new Attribute($variables['content_attributes']) : clone($default_attributes);
  foreach ($variables['items'] as $delta => $item) {
    $variables['item_attributes'][$delta] = isset($variables['item_attributes'][$delta]) ? new Attribute($variables['item_attributes'][$delta]) : clone($default_attributes);
  }
}

Proposed resolution

The template_process_field function needs to be removed from Drupal 8.

Remaining tasks

- Move all attribute variables into the preprocess layer.
- remove this function.

User interface changes

None.

API changes

TBD

#1843650: Remove the process layer (hook_process and hook_process_HOOK)

CommentFileSizeAuthor
#28 field-before.png40.83 KBjenlampton
#28 field-after.png40.58 KBjenlampton
#27 1843728-27.patch3.04 KBstar-szr
#27 interdiff.txt2.46 KBstar-szr
#16 1843528-15.patch2.1 KBmr.baileys
#8 core-remove_template_process_field-1843728-8.patch2.1 KBpatrickfgoddard
#8 core-remove_template_process_field-1843728-8-interdiff.txt1.37 KBpatrickfgoddard
#1 core-remove_template_process_field-1843728-1.patch1.28 KBjenlampton
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jenlampton’s picture

jenlampton
she / her
CreditAttribution: jenlampton commented
Status: Active » Needs review
FileSize
core-remove_template_process_field-1843728-1.patch1.28 KB

Re the comment inside the process function...

  // The default theme implementation is a function, so template_process() does
  // not automatically run, so we need to flatten the classes and attributes
  // here. For best performance, only instantiate Drupal\Core\Template\Attribute
  // when needed, and note that template_preprocess_field() does not initialize
  // the *_attributes variables.

I believe is a left-over from when attributes *were* flattened in the process layer. They are not anymore since they are an attributes object that gets flattened at render-time, which is why I think it is safe to move these variables up to preprocess. All attributes are printed correctly with the attached patch.

Status: Needs review » Needs work

The last submitted patch, core-remove_template_process_field-1843728-1.patch, failed testing.

jenlampton’s picture

jenlampton
she / her
CreditAttribution: jenlampton commented

I think the problem here is that array_merge and array_merge_recursive aren't going to work if one of those "arrays" is actually an Attributes object already. We may need another way for modules to alter attributes. :/

steveoliver’s picture

steveoliver CreditAttribution: steveoliver commented

Good news: #1982024: Lazy-load Attribute objects later in the rendering process only if needed

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs work » Needs review
Issue tags: -Twig, -theme system cleanup

#1: core-remove_template_process_field-1843728-1.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Twig, +theme system cleanup

The last submitted patch, core-remove_template_process_field-1843728-1.patch, failed testing.

patrickfgoddard’s picture

patrickfgoddard CreditAttribution: patrickfgoddard commented
Assigned: Unassigned » patrickfgoddard

re-rolling.

patrickfgoddard’s picture

patrickfgoddard CreditAttribution: patrickfgoddard commented
Assigned: patrickfgoddard » Unassigned
Status: Needs work » Needs review
FileSize
core-remove_template_process_field-1843728-8-interdiff.txt1.37 KB
core-remove_template_process_field-1843728-8.patch2.1 KB

Per: #1982024: Lazy-load Attribute objects later in the rendering process only if needed
and IRC discussion with @steveoliver and Cottser, I set attributes, title_attributes, content_attributes variables in field.module to empty array()

Hopefully this helps move this along. Please let me know if any other assistance needed with changes and/or patches for this issue.

Status: Needs review » Needs work
Issue tags: -Twig, -theme system cleanup

The last submitted patch, core-remove_template_process_field-1843728-8.patch, failed testing.

johnshortess’s picture

johnshortess
he/him
Primary language English
Location Fairfax, Virginia, USA
CreditAttribution: johnshortess commented
Status: Needs work » Needs review
Issue tags: +Twig, +theme system cleanup

#8: core-remove_template_process_field-1843728-8.patch queued for re-testing.

johnshortess’s picture

johnshortess
he/him
Primary language English
Location Fairfax, Virginia, USA
CreditAttribution: johnshortess commented
Assigned: Unassigned » johnshortess

Status: Needs review » Needs work

The last submitted patch, core-remove_template_process_field-1843728-8.patch, failed testing.

johnshortess’s picture

johnshortess
he/him
Primary language English
Location Fairfax, Virginia, USA
CreditAttribution: johnshortess commented
Assigned: johnshortess » Unassigned
mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys commented
Assigned: Unassigned » mr.baileys

Tackling this as part of the DC PDX Friday sprint.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys commented
Status: Needs work » Needs review

Patch from #8 no longer applied so re-rolled it.

The empty array() initialization in #8 is causing "Array to String"-conversion errors during tests, so for now I converted these to "new Attribute" inits in theme_preprocess_field.

+  $variables['attributes'] = new Attribute;
+  $variables['title_attributes'] = new Attribute;
+  $variables['content_attributes'] = new Attribute;

Not sure if this is the approach we are going for in light of #1982024: Lazy-load Attribute objects later in the rendering process only if needed, but this patch removes field_process_field and makes the tests pass.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys commented
FileSize
1843528-15.patch2.1 KB

Actual patch file...

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys commented
Assigned: mr.baileys » Unassigned

Status: Needs review » Needs work
Issue tags: -Twig, -theme system cleanup

The last submitted patch, 1843528-15.patch, failed testing.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys commented
Status: Needs work » Needs review

#16: 1843528-15.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1843528-15.patch, failed testing.

jenlampton’s picture

jenlampton
she / her
CreditAttribution: jenlampton commented
Status: Needs work » Needs review

#16: 1843528-15.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Twig, +theme system cleanup

The last submitted patch, 1843528-15.patch, failed testing.

jenlampton’s picture

jenlampton
she / her
CreditAttribution: jenlampton commented

Tests may have been passing in #15, but they aren't now, and these test failures are legit. I'm seeing problems with the body field attributes:

<div class="field-item even" array="">
  <p>itqYO0Oh</p>
</div>

Ideas?

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented

I don't think we can just clear out those Attribute objects. This one is complex because at least according to the docs both theme functions and template files go through these (pre)process functions. We may need to keep those isset's for now.

jenlampton’s picture

jenlampton
she / her
CreditAttribution: jenlampton commented

I tried adding the issets back but still got the array="" on the body tag for comments. I don't think that's the only issue. :/

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Assigned: Unassigned » star-szr

Going to see if I can sort this one out.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs work » Needs review
FileSize
interdiff.txt2.46 KB
1843728-27.patch3.04 KB

Let's see what testbot thinks about this, the failing tests above are green locally.

Before patch:

rdf_preprocess_field() - creates new array of attributes at $variables['item_attributes'][$delta], disregarding any existing value
template_process_field() - converts $variables['item_attributes'][$delta] to an Attribute object

After patch:

template_preprocess_field() - creates new Attribute object for $variables['item_attributes'][$delta]
rdf_preprocess_field() - creates new array of attributes at $variables['item_attributes'][$delta], disregarding any existing value. In this case the Attribute object that we need for printing the attributes gets clobbered so we convert it back to an Attribute object.

I will ping @scor about this issue, maybe there is a better way to solve this. My concern is that currently rdf_preprocess_field() overwrites $variables['item_attributes'][$delta] completely - but arguably that is a separate bug/limitation that can be addressed in a new issue (I couldn't find an existing one).

jenlampton’s picture

jenlampton
she / her
CreditAttribution: jenlampton commented
Status: Needs review » Reviewed & tested by the community
FileSize
field-after.png40.58 KB
field-before.png40.83 KB

Sounds good. I think we can resolve the RDF overwriting issue in a follow-up issue (and in the RDF module?) Would love to kill process first. :) Testing...

Source before move:
field-before.png
Source after move:
field-after.png

Looking good!

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs work

Let's do the RDF fix here, I don't think that's covered by any existing issues.

scor’s picture

scor CreditAttribution: scor commented
Status: Needs work » Reviewed & tested by the community

Given that this bug in rdf.module will have to be backported to D7, and in the interest of separation of concerns/modules, I've created a dedicated issue to fix the bug in RDF: #2034003: item_attributes are overridden in rdf_preprocess_field() with a patch forthcoming...

Setting this issue back to RTBC.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

OK thanks for the other issue and the patch. Committed/pushed this one to 8.x.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Assigned: star-szr » Unassigned

Thanks @scor and @catch!

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