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
Related Issues
#1982018: [meta] Refactor template_preprocess()
#1757550: [Meta] Convert core theme functions to Twig templates
Comments
Comment #1
star-szrHere is the relevant part of @Fabianx's patch from #1938430-16: Don't add a default theme hook class in template_preprocess().
Comment #2
Fabianx CreditAttribution: Fabianx commentedCNW: The call to default_attributes and the var can be removed from template_preprocess then ;-).
Comment #3
star-szrRevised for #2, looking in to the test failures locally.
Comment #4
star-szrWith 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.
Comment #5
Fabianx CreditAttribution: Fabianx commentedThere 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().
Comment #6
Fabianx CreditAttribution: Fabianx commentedRe-rolled with default_attributes cloned for empty array addressing #4, #5.
Comment #7
Fabianx CreditAttribution: Fabianx commentedAaaand 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):
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):
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.
Comment #8
thedavidmeister CreditAttribution: thedavidmeister commentedThis 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.
Comment #9
Wim LeersAssuming 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:
Missing trailing period.
Comment #10
Wim LeersComment #11
thedavidmeister CreditAttribution: thedavidmeister commented@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?
Comment #12
Fabianx CreditAttribution: Fabianx commentedRTBC per #8
I only changed the documentation nitpick ...
Test coverage should be sufficient from Attribute() patch itself.
Comment #13
catchLooks great to me. Committed/pushed to 8.x.
Comment #14
steveoliver CreditAttribution: steveoliver commentedAny 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 fromtemplate_process_field()
, for example.Comment #15
Wim Leers#14: this issue was marked as fixed 10 days ago. Please create a new issue and refer to this one.
Comment #16
steveoliver CreditAttribution: steveoliver commentedSure. Discussion in IRC with Fabianx has resolved the issue for me, for now at least.
Comment #18
webchickNote: #2108771: Remove special cased title_attributes and content_attributes for Attribute creation was opened which proposes to back out this change for DX reasons.