Problem/Motivation
As originally proposed in #306358-10: Add $classes to templates with new theme_process_hook functions, the function template_preprocess()
adds in some arbitrary variables for every template. Many of these are superfluous and are simply "nice-to-haves" that tend to create messy markup or involves an extra layer of processing (such as hook_template_preprocess_default_variables_alter()
).
With the move to Twig, there are also unintended consequences. For example:
// Initialize html class attribute for the current hook.
$variables['attributes']['class'][] = drupal_html_class($hook);
With the Attribute object, this will *always* insert a class based on the hook name when we use the following:
Twig:
<div{{ attributes }}>
PHPTemplate:
<div<?php print $attributes; ?>>
When converting a theme function to a template file (ala #1757550: [Meta] Convert core theme functions to Twig templates) this default behaviour results in added classes that are not needed, and hurts performance.
Proposed resolution
This issue proposes that we do not need a HTML class name for every single template, and that classes should be added on a case-by-case basis via template_preprocess_HOOK() or even directly in template files when necessary.
Remaining tasks
TBD
User interface changes
n/a
API changes
template_preprocess() will no longer add a default class to the ‘attributes’ variable for each theme hook. Classes will need to be on a case-by-case basis in template_preprocess_HOOK() or directly in template files.
Related Issues
#1982018: [meta] Refactor template_preprocess()
#1757550: [Meta] Convert core theme functions to Twig templates
Comment | File | Size | Author |
---|---|---|---|
#31 | interdiff.txt | 1.75 KB | shanethehat |
#31 | remove-default-theme-hook-class-1938430-31.patch | 7.75 KB | shanethehat |
#28 | remove-default-theme-hook-class-1938430-28.patch | 7.46 KB | shanethehat |
#26 | interdiff.txt | 6.1 KB | shanethehat |
#26 | remove-default-theme-hook-class-1938430-26.patch | 7.54 KB | shanethehat |
Comments
Comment #1
steveoliver CreditAttribution: steveoliver commentedAmen. And for that matter, those classes can be added in the templates themselves.
Comment #2
jenlampton+1
Comment #3
steveoliver CreditAttribution: steveoliver commentedLet's see what breaks when we remove the default class for every themed element.
Comment #5
steveoliver CreditAttribution: steveoliver commentedThis should fix those fails, and is what I'm proposing we do for output that wants these kinds of 'default' classes.
Comment #6
star-szrThis is definitely a step in the right direction, and I actually moved the "block" class directly to block.html.twig in #1898034: block.module - Convert PHPTemplate templates to Twig.
Comment #7
joelpittet+1
Comment #8
steveoliver CreditAttribution: steveoliver commentedBumping this to major as it's a dependency for some Twig conversion issues like #1939062: Convert theme_item_list() to Twig and others (need references, can't find 'em -- peoples help me out here).
Comment #9
star-szrAll these types of comments in template files should be removed as well, but that's probably a follow-up.
Comment #10
joelpittet#1898466: update.module - Convert theme_ functions to Twig here's one for template_preprocess_update_version() which get's an extra class during conversion.
Comment #10.0
joelpittetClarified issue summary.
Comment #11
star-szrRe-titling to reflect the proposed change.
Comment #12
steveoliver CreditAttribution: steveoliver commentedAfter discussion with joelpittet and Cottser, postponing this on complete conversion, until then using an array_diff workaround to remove unnecessary classes until we can make sure this doesn't break anything without test coverage.
Comment #13
star-szrUnpostponing, let's figure this out. It's a performance improvement and will remove plenty of @todos from our conversion issues.
Comment #14
tim.plunkettWhy wouldn't a node need .node? As I read this, that would go away.
Comment #15
star-szrThe patch needs quite a bit of work, node will still get .node but we will add the classes as needed in preprocess since many cases that we are converting do not need this class at all, e.g. #1843746: Convert views/templates/views-view-field.tpl.php to Twig .
Comment #16
Fabianx CreditAttribution: Fabianx commentedHere is another approach, which instead makes drupal_html_class use static caching, which accounts for 2ms overall then. It also has some lazy instantiation of attribute objects only when needed, which makes things faster, but I don't know where to put it.
Lets see if this still passes tests.
Comment #18
c4rl CreditAttribution: c4rl commentedJust catching-up with this issue. For the record, my original intentions of filing this issue was really to gut the *entirety* of template_preprocess(), which may not have been clear.
So, not to distract too much, I'd like pose the question: What if we removed template_preprocess() and _template_preprocess_default_variables() and hook_template_preprocess_default_variables_alter() entirely? I'd argue that the variables that these aim to provide or alter can be done in other places (i.e. specific preprocessors) and via other Drupal API functions.
I'd be interested in (A) hearing others' arguments for/against this and potential use-cases. I'll explore (B) what the implications are of doing the (rather sweeping) changes I proposed above. If outcomes of both A and B seem productive and are aligned, that may refocus the discussion here.
Nevertheless, thanks for the efforts here.
Comment #19
star-szr@c4rl - Thanks for checking in. I fear a more sweeping change (not to mention the discussion around it) would delay Twig conversion even further and I don't think we can afford that. I suggest we take baby steps instead. If we find after refactoring that we don't need template_preprocess() at all, great. Otherwise, let's continue to refactor performance and functionality one bit at a time.
I created #1982018: [meta] Refactor template_preprocess() and I am moving @Fabianx's patch to two separate issues which will be part of the new meta.
Comment #19.0
star-szrUpdate summary to use template
Comment #19.1
star-szrAdd refactor meta
Comment #19.2
star-szrUpdated issue summary.
Comment #20
star-szrUpdated the issue summary and added a link to the meta issue. Tweaked the title too because I keep thinking 'default theme hook' when trying to find this issue.
Comment #21
c4rl CreditAttribution: c4rl commented#19@Cottser, good points. We'll carry on as you suggested; just wanted to put the more sweeping concepts on everyone's radar. Onwards!
Comment #22
star-szrRe-postponing until we have all the .tpl.php's converted.
Comment #23
shanethehat CreditAttribution: shanethehat commentedGoing to try and get this done first, as per IRC discussion
Comment #24
shanethehat CreditAttribution: shanethehat commentedQuick reroll of #5 first, make sure there are no new failures.
Comment #25
shanethehat CreditAttribution: shanethehat commentedNow on with the show...
Comment #26
shanethehat CreditAttribution: shanethehat commentedI think I've got them all. I went though all 61 .tpl.php files currently in core and compared the markup on a clean install vs one with #24 applied. Most templates did not require any changes to the preprocess for various reasons listed below.
Needed class adding:
core/modules/system/templates/html.tpl.php
core/modules/comment/templates/comment-wrapper.tpl.php
core/modules/node/templates/node.tpl.php
core/modules/system/templates/maintenance-page.tpl.php
core/modules/system/templates/region.tpl.php
core/modules/comment/templates/comment.tpl.php
core/modules/overlay/templates/overlay.tpl.php
core/modules/block/templates/block.tpl.php
core/modules/taxonomy/templates/taxonomy-term.tpl.php
core/modules/layout/layouts/static/twocol/two-col.tpl.php
core/modules/layout/layouts/static/one-col/one-col.tpl.php
core/modules/views_ui/templates/views-ui-display-tab-setting.tpl.php
core/modules/views_ui/templates/views-ui-display-tab-bucket.tpl.php
core/modules/views/templates/views-view-table.tpl.php
Did not need any changes:
core/modules/system/templates/page.tpl.php - page class is added to html template
core/modules/field/templates/field.tpl.php - already added in preprocess
core/modules/node/templates/node-edit-form.tpl.php - no classes assigned
core/modules/system/templates/system-plugin-ui-form.tpl.php - classes hard-coded to template
core/modules/system/tests/modules/theme_test/templates/theme_test.template_test.tpl.php - no classes used
core/modules/system/tests/themes/test_theme/theme_test.template_test.tpl.php - no classes used
core/modules/system/tests/themes/test_theme/node--1.tpl.php - no classes used
core/modules/block/tests/themes/block_test_theme/page.tpl.php - no classes used
core/modules/block/templates/block-admin-display-form.tpl.php - classes hard-coded in template
core/modules/user/templates/user.tpl.php - class is hard-coded in template
core/modules/user/templates/user-picture.tpl.php - template not used as of #1292470: Convert user pictures to Image Field and will be removed in #1898468: user.module - Convert PHPTemplate templates to Twig
core/modules/aggregator/templates/aggregator-wrapper.tpl.php - template not used as of #293318: Convert Aggregator feeds into entities and will be removed in #1896060: aggregator.module - Convert PHPTemplate templates to Twig
core/modules/aggregator/templates/aggregator-feed-source.tpl.php - classes hard-coded into template
core/modules/aggregator/templates/aggregator-item.tpl.php - classes hard-coded in template
core/modules/aggregator/templates/aggregator-summary-items.tpl.php - no classes used
core/modules/forum/templates/forums.tpl.php - no classes used
core/modules/forum/templates/forum-icon.tpl.php - classes hard-coded in template, and combined with a specific $icon_class variable
core/modules/forum/templates/forum-list.tpl.php - no classes used
core/modules/forum/templates/forum-topic-list.tpl.php - no classes used
core/modules/forum/templates/forum-submitted.tpl.php - class is hard-coded in template
core/modules/book/templates/book-all-books-block.tpl.php - class is hard-coded in template
core/modules/book/templates/book-export-html.tpl.php - attributes not used, $html_attributes used instead
core/modules/book/templates/book-node-export-html.tpl.php - class is combination of hard-coded and $depth variable
core/modules/book/templates/book-navigation.tpl.php - classes are hard-coded in template
core/modules/layout/tests/layouts/static/one-col/one-col.tpl.php - classes are hard-coded in template
core/modules/layout/tests/themes/layout_test_theme/layouts/static/two-col/two-col.tpl.php - classes are hard-coded in template
core/modules/views/templates/views-exposed-form.tpl.php - classes are hard-coded in template
core/modules/views/templates/views-more.tpl.php - class is hard-coded in template
core/modules/views/templates/views-view-field.tpl.php - no attributes used
core/modules/views/templates/views-view-fields.tpl.php - no attributes used
core/modules/views/templates/views-view-grid.tpl.php - no change needed, before the patch the views-view-grid class was applied twice
core/modules/views/templates/views-view-grouping.tpl.php - classes are hard-coded in template
core/modules/views/templates/views-view-list.tpl.php - attributes not used
core/modules/views/templates/views-view-row-rss.tpl.php - no attributes used
core/modules/views/templates/views-view-rss.tpl.php - not HTML
core/modules/views/templates/views-view-summary-unformatted.tpl.php - attributes not used
core/modules/views/templates/views-view-summary.tpl.php - classes are hard-coded in template
core/modules/views/templates/views-view-unformatted.tpl.php - no attributes used
core/modules/views/templates/views-view.tpl.php - attributes array reset in preprocess
core/modules/views/tests/views_test_data/templates/views-view--frontpage.tpl.php - not sure about this, see if tests still pass
core/themes/bartik/templates/page.tpl.php - theme preprocess doesn't reset attributes
core/themes/bartik/templates/node.tpl.php - theme preprocess doesn't reset attributes
core/themes/bartik/templates/comment.tpl.php - theme preprocess doesn't reset attributes
core/themes/bartik/templates/maintenance-page.tpl.php - theme preprocess doesn't reset attributes
core/themes/bartik/templates/comment-wrapper.tpl.php - theme preprocess doesn't reset attributes
core/themes/seven/templates/page.tpl.php - theme preprocess doesn't reset attributes
core/themes/seven/templates/maintenance-page.tpl.php - theme preprocess doesn't reset attributes
Comment #27
shanethehat CreditAttribution: shanethehat commented@steveoliver asked in IRC whether this would have impact on the existing theme_* functions. I do not believe it will because:
- Source
Also, I have compared the markup of a large selection theme_* functions the utilise $variables['attributes'], and have not found and differences between a clean install and the patch above. Disclaimer: I have not compared all 166 theme functions, but took a wide sample from form.inc and various modules.
Comment #28
shanethehat CreditAttribution: shanethehat commentedRerolling after changes to block module. Now to check the 9 theme preprocess functions I've found...
Comment #29
shanethehat CreditAttribution: shanethehat commentedFound more than 9 candidates in the end, using this if anyone wants to confirm: https://gist.github.com/shanethehat/5567510
I don't think any further changes are required, so marking this for review.
Comment #30
star-szrThis is looking very good, excellent and thorough work here @shanethehat!
I went through all the templates myself and verified what was reported in #26.
Looking over the results of the shell script posted in #29:
No changes needed here other than a theme function that is actually no longer in use, theme_datetime() - instead datetime.html.twig is used. This is definitely an oddity because it was converted as part of the initial Twig patch: #1696786: Integrate Twig into core: Implementation issue
So we'll need a 'datetime' class added in template_preprocess_datetime() please.
…and a couple very minor doc/coding standards fixes:
Trailing comma needed per http://drupal.org/coding-standards#array.
Comment needs to be a complete sentence (end in a period) per http://drupal.org/node/1354#drupal.
Comment #31
shanethehat CreditAttribution: shanethehat commentedThanks for the review @Cottser, datetime managed to slip though because the first pass was a search for .tpl.php.
Comment #32
idflood CreditAttribution: idflood commentedComment #33
Fabianx CreditAttribution: Fabianx commentedThis looks good, has been thoroughly manually tested and saves performance and makes nicer markup. It will save more performance in the future and for contrib themes.
=> RTBC
Comment #34
star-szrAgreed with the RTBC.
Comment #35
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #36.0
(not verified) CreditAttribution: commentedUpdated issue summary.