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.

#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

steveoliver’s picture

Amen. And for that matter, those classes can be added in the templates themselves.

jenlampton’s picture

+1

steveoliver’s picture

Status: Active » Needs review
FileSize
481 bytes

Let's see what breaks when we remove the default class for every themed element.

Status: Needs review » Needs work

The last submitted patch, drupal-refactor-template-preprocess--1938430-3.patch, failed testing.

steveoliver’s picture

Status: Needs work » Needs review
FileSize
2.21 KB

This should fix those fails, and is what I'm proposing we do for output that wants these kinds of 'default' classes.

star-szr’s picture

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

joelpittet’s picture

+1

steveoliver’s picture

Priority: Normal » Major

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

star-szr’s picture

All these types of comments in template files should be removed as well, but that's probably a follow-up.

 * - attributes: Remaining HTML attributes for the containing element.
 *   It includes the 'class' information, which includes:
 *   - comment-wrapper: The current template type, i.e., "theming hook".
joelpittet’s picture

#1898466: update.module - Convert theme_ functions to Twig here's one for template_preprocess_update_version() which get's an extra class during conversion.

joelpittet’s picture

Issue summary: View changes

Clarified issue summary.

star-szr’s picture

Title: Refactor template_preprocess() » Don't add a theme hook class in template_preprocess()

Re-titling to reflect the proposed change.

steveoliver’s picture

Assigned: Unassigned » steveoliver
Status: Needs review » Postponed

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

star-szr’s picture

Status: Postponed » Needs work

Unpostponing, let's figure this out. It's a performance improvement and will remove plenty of @todos from our conversion issues.

tim.plunkett’s picture

Why wouldn't a node need .node? As I read this, that would go away.

star-szr’s picture

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

Fabianx’s picture

Status: Needs work » Needs review
FileSize
3.7 KB

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

Status: Needs review » Needs work
c4rl’s picture

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

star-szr’s picture

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

star-szr’s picture

Issue summary: View changes

Update summary to use template

star-szr’s picture

Issue summary: View changes

Add refactor meta

star-szr’s picture

Issue summary: View changes

Updated issue summary.

star-szr’s picture

Title: Don't add a theme hook class in template_preprocess() » Don't add a default theme hook class in template_preprocess()

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

c4rl’s picture

#19@Cottser, good points. We'll carry on as you suggested; just wanted to put the more sweeping concepts on everyone's radar. Onwards!

star-szr’s picture

Status: Needs work » Postponed

Re-postponing until we have all the .tpl.php's converted.

shanethehat’s picture

Assigned: steveoliver » shanethehat
Status: Postponed » Needs work

Going to try and get this done first, as per IRC discussion

shanethehat’s picture

Status: Needs work » Needs review
FileSize
2.09 KB

Quick reroll of #5 first, make sure there are no new failures.

shanethehat’s picture

Status: Needs review » Needs work

Now on with the show...

shanethehat’s picture

Assigned: shanethehat » Unassigned
Status: Needs work » Needs review
FileSize
7.54 KB
6.1 KB

I 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

shanethehat’s picture

@steveoliver asked in IRC whether this would have impact on the existing theme_* functions. I do not believe it will because:

This function is called for theme hooks implemented as templates only, not for theme hooks implemented as functions

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

shanethehat’s picture

Status: Needs review » Needs work
FileSize
7.46 KB

Rerolling after changes to block module. Now to check the 9 theme preprocess functions I've found...

shanethehat’s picture

Status: Needs work » Needs review

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

star-szr’s picture

Status: Needs review » Needs work

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

includes/form.inc:function template_preprocess_input(&$variables) {
includes/menu.inc:function template_preprocess_menu_tree(&$variables) {
includes/theme.inc:function template_preprocess_datetime(&$variables) {
includes/theme.inc:function template_preprocess_item_list(&$variables) {
modules/aggregator/aggregator.pages.inc:function template_preprocess_aggregator_summary_item(&$variables) {
modules/ckeditor/ckeditor.admin.inc:function template_preprocess_ckeditor_settings_toolbar(&$variables) {
modules/field/field.module:function template_preprocess_field(&$variables, $hook) {
modules/language/language.admin.inc:function template_preprocess_language_content_settings_table(&$variables) {
modules/system/tests/modules/theme_test/theme_test.inc:function template_preprocess_theme_test(&$variables) {
modules/toolbar/toolbar.module:function template_preprocess_toolbar_tab_wrapper(&$variables) {
modules/user/user.module:function template_preprocess_username(&$variables) {
modules/views/tests/views_test_data/views_test_data.module:function template_preprocess_views_view_mapping_test(&$variables) {
modules/views/views.theme.inc:function template_preprocess_views_view_field(&$vars) {
modules/views_ui/views_ui.theme.inc:function template_preprocess_views_ui_view_info(&$variables) {
modules/views_ui/views_ui.theme.inc:function template_preprocess_views_ui_view_preview_section(&$vars) {

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:

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/Layout/StaticLayout.phpundefined
@@ -82,6 +82,9 @@ public function renderLayout($admin = FALSE, $regions = array()) {
+        'class' => drupal_html_class($definition['theme'])

Trailing comma needed per http://drupal.org/coding-standards#array.

+++ b/core/modules/comment/comment.moduleundefined
@@ -1724,6 +1725,9 @@ function template_preprocess_comment_wrapper(&$variables) {
+
+  // Add comment wrapper class
+  $variables['attributes']['class'][] = 'comment-wrapper';

Comment needs to be a complete sentence (end in a period) per http://drupal.org/node/1354#drupal.

shanethehat’s picture

Thanks for the review @Cottser, datetime managed to slip though because the first pass was a search for .tpl.php.

idflood’s picture

Status: Needs work » Needs review
Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

This 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

star-szr’s picture

Agreed with the RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.