Problem/Motivation

Currently there is no way to know when HTML attributes arrays (i.e. attributes, title_attributes, content_attributes, etc.) should be cast to Attribute objects. template_preprocess(), for example, creates three Attribute objects, but the template to be rendered may not use any of them.

Proposed resolution

Instantiate Attribute objects in a consistent fashion and/or location. Since the main benefit of the Attribute class is only printing the attributes once, consider moving all instantiation of Attribute objects to as late as possible in the rendering process.

Remaining tasks

TBD

User interface changes

n/a

API changes

TBD

#1982018: [meta] Refactor template_preprocess()
#1757550: [Meta] Convert core theme functions to Twig templates

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

Status: Active » Needs review
FileSize
2.39 KB

Here is the relevant part of @Fabianx's patch from #1938430-16: Don't add a default theme hook class in template_preprocess().

Fabianx’s picture

Status: Needs review » Needs work

CNW: The call to default_attributes and the var can be removed from template_preprocess then ;-).

star-szr’s picture

Assigned: Unassigned » star-szr
FileSize
559 bytes
2.53 KB

Revised for #2, looking in to the test failures locally.

star-szr’s picture

Assigned: star-szr » Unassigned
Status: Needs work » Needs review
FileSize
2.16 KB
3.28 KB

With this patch an Attribute object is always created, but at least this approach seems to get rid of empty class attributes. Haven't dug into why that is yet.

Before:
<h2 class="">

After:
<h2>

Empty arrays should be fine to print in a template with Twig but it won't fly with PHPTemplate, not sure how we should handle that.

Fabianx’s picture

+++ b/core/includes/theme.incundefined
@@ -2637,13 +2646,10 @@ function template_preprocess(&$variables, $hook) {
-  if (!isset($default_attributes)) {
-    $default_attributes = new Attribute(array('class' => array()));

There is still a variable defined static at the top :)

> Empty arrays should be fine to print in a template with Twig but it won't fly with PHPTemplate, not sure how we should handle that.

Hm, right. In that case you can just clone a static default one or we could have an EmptyAttributes object, which returns '' for each ->get().

Fabianx’s picture

Re-rolled with default_attributes cloned for empty array addressing #4, #5.

Fabianx’s picture

Aaaand benchmark (first is baseline vs. baseline), all with 50 nodes and APC classloader:

This first is just to show that the used measuring method is precise as a re-benchmark leads to no difference.

=== head-50-nodes..8.x compared (5185b9518e91e..5185bad3711d6):

ct  : 259,866|259,866|0|0.0%
wt  : 943,793|943,769|-24|-0.0%
cpu : 940,059|940,060|1|0.0%
mu  : 24,751,760|24,752,568|808|0.0%
pmu : 25,306,760|25,307,816|1,056|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5185b9518e91e&...

And this is with the patch from #6:

=== head-50-nodes..lazy-load--attribute--6 compared (5185b9518e91e..5185bb5ce59fb):

ct  : 259,866|258,688|-1,178|-0.5%
wt  : 943,793|938,141|-5,652|-0.6%
cpu : 940,059|940,059|0|0.0%
mu  : 24,751,760|24,730,840|-20,920|-0.1%
pmu : 25,306,760|25,280,264|-26,496|-0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5185b9518e91e&...

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

I think this looks good!

1178 function calls saved! If this again was not my patch, this would be RTBC.

Afterwards any "new Attribute(...)" (as followup) can be removed from _preprocess functions.

thedavidmeister’s picture

Status: Needs review » Reviewed & tested by the community

This looks RTBC to me too. It takes the Attribute() objects that are being set unconditionally in preprocess and moves them to the bottom of theme() - after both preprocess and process phases. It also makes the objects only be created if they're required to be rendered.

There's no documentation changes to review here, it's all just logic.

Wim Leers’s picture

Assuming there's test coverage for all this, this is indeed RTBC.

Also: THANK YOU. The current behavior is infuriating when you need to merge in more attributes: #1972514-2: Impossible to set attributes for all entities.


Only one nitpick:

+++ b/core/includes/theme.incundefined
@@ -1157,6 +1158,20 @@ function theme($hook, $variables = array()) {
+          // Create empty attributes

Missing trailing period.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work
thedavidmeister’s picture

@Wim Leers - I don't think it's even possible to write tests to see what part of theme() the Attribute objects are being generated in, is it?

Fabianx’s picture

RTBC per #8

I only changed the documentation nitpick ...

Test coverage should be sufficient from Attribute() patch itself.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Looks great to me. Committed/pushed to 8.x.

steveoliver’s picture

Status: Fixed » Needs review
FileSize
1.86 KB

Any reason not to give theme functions the same treatment as templates? The attached patch would allow for the removal of the first three Attribute() instantiations from template_process_field(), for example.

Wim Leers’s picture

Status: Needs review » Fixed

#14: this issue was marked as fixed 10 days ago. Please create a new issue and refer to this one.

steveoliver’s picture

Sure. Discussion in IRC with Fabianx has resolved the issue for me, for now at least.

Status: Fixed » Closed (fixed)

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

webchick’s picture

Note: #2108771: Remove special cased title_attributes and content_attributes for Attribute creation was opened which proposes to back out this change for DX reasons.