Problem/Motivation

For phase 1 of the consensus banana, we need to move all classes from preprocess into the templates. This meta issue will serve as a place to list everything, discuss strategy, and group the changes into smaller, workable pieces. Once everything is done, or done in parts, we can add mega patches to this issue.

Proposed resolution

Move all classes provided in core preprocess functions into core template files. The only goal here is to move the classes. No changes to markup! Existing functionality should be maintained.

Note: Check if a change affects a template overridden in Bartik or Seven. If so, adjust their respective template.

  1. Find logical groupings of functions/templates.
  2. Create a subissue of this meta for working on the change.
  3. Link that issue here in the summary. (See the child issues in the sidebar.)
  4. Update this summary when the change is done.

Two API additions occurred before this issue to facilitate moving the classes. One is the addition of the attribute addClass method, the other is the clean_class filter. Check the related issues, and use the work done for node as a guide. #2254153: Move node classes out of preprocess and into templates

Use the following Google Doc to match preprocess functions with open issues. #8 contains the script used to generate the list.

https://docs.google.com/spreadsheets/d/1vc79t_bwRfMryyo63BCbHblLpJZqRSa2...

If you create a new issue, please add it to the spreadsheet and add this issue as its parent.

Steps and Examples

The issue summary of each child issue should have the preprocess functions and templates that need modification. The classes created in the preprocess function(s), for example $variables['attributes']['class'][] = 'clearfix';, should be removed.

The class should then be added directly in the template. The class also needs to be added to any derivative template files. For example, in the Field issue all the classes originally created in template_preprocess_field() had to be added to field.html.twig and also field--comment.html.twig.

Lastly, check that any comments in the preprocess functions or templates that refer to the classes are updated.

Classes are added to an attribute using the addClass() method.

Examples

Adding one class:
attributes.addClass('field')

Adding two classes:
attributes.addClass('field', 'clearfix')

If more than two classes need to be added, or logic needs to be used to create a class name, set the classes in a variable. This will keep the template markup easy to understand.

We are standardizing on the word 'classes' for the variable name. Use 'classes' when adding to 'attributes', 'title_classes' when adding to 'title_attributes', etc.

Adding more than two:

{%
  set classes = [
    'field',
    'clearfix',
    'wrapper',
  ]
%}
<div{{ attributes.addClass(classes) }}>

Note the trailing comma after 'wrapper'

Adding dynamic classes using ternary logic:

{%
  set classes = [
    'field-label-' ~ label_display,
    label_display == 'inline' ? 'clearfix',
  ]
%}
<div{{ attributes.addClass(classes) }}>

Using the clean_class filter:

{%
  set classes = [
    'field-label-' ~ label_display,
    label_display == 'inline' ? 'clearfix',
    'field-name-' ~ field_name|clean_class,
  ]
%}
<div{{ attributes.addClass(classes) }}>

The clean_class filtered should be used on any string that won't be properly formatted for use as a CSS class name.

Comments

davidhernandez’s picture

Issue summary: View changes
davidhernandez’s picture

Issue summary: View changes
davidhernandez’s picture

Issue summary: View changes
wheatpenny’s picture

Issue summary: View changes
wheatpenny’s picture

Issue summary: View changes
wheatpenny’s picture

Issue summary: View changes
joelpittet’s picture

Issue summary: View changes

This will get a good sense of where all the files preprocess functions are that have classes in them:

find core \( -name \*.module -o -name \*.inc \)  -exec grep -lHn "\['class'\]" {} \;

Which is 37 files, I'll avoid dumping them here for scroll blindness sake but if you agree maybe this can replace the one in the issue summary?

I don't think we need the list of twig templates, just looks daunting. What were your thoughts on that?

And there are about 222 lines in those files, just remove the -l from grep:

find core \( -name \*.module -o -name \*.inc \)  -exec grep -Hn "\['class'\]" {} \;

I think it would be best to break up this list into issue by preprocess/template. So going to write a quick script in PHP to find these preprocess functions. Stay tuned.

joelpittet’s picture

Here's a hitlist, how would you like to divide and conquer?

https://docs.google.com/spreadsheets/d/1vc79t_bwRfMryyo63BCbHblLpJZqRSa2...

#7 was missing the theme functions.

That excel was generated with:

// @file find_preprocess_classes.php
exec('find core \( -name \*.module -o -name \*.inc -o -name \*.theme \)  -exec grep -lHn "\[\'class\'\]" {} \;', $files_with_class);
foreach ($files_with_class as $key => $file) {
  $file_lines = file($file);
  $in_preprocess = FALSE;
  $last_preprocess = NULL;
  $last_preprocess_line = NULL;
  $preprocess_with_class = [];
  foreach ($file_lines as $num => $line) {
    if (preg_match('#function ([^\(]+_preprocess_[^\(]+)\(&\$variables#', $line, $matches)) {
      $in_preprocess = TRUE;
      $last_preprocess_line = $num;
      $last_preprocess = $matches[1];
    }

    if ($in_preprocess) {
      if (preg_match('#\'class\'\]#', $line)) {
        $preprocess_with_class[$last_preprocess] = $last_preprocess_line;
      }
    }

    if ($line == '}') {
      $in_preprocess = FALSE;
    }
  }

  if (!empty($preprocess_with_class)) {
    print "\n\t" . $file . "\n";
    foreach ($preprocess_with_class as $preprocess => $preprocess_num) {
      print $preprocess_num . "\t" . $preprocess . "\n";
    }
  }
}

Edit: Needed to open the for 'class' because some are added like ('class' => and not just ['class']
To run save to your drupal root and run:: php find_preprocess_classes.php

davidhernandez’s picture

Awesome, Joel. I'll try to look at this some more tomorrow, but the grouping in the spreadsheet is basically what I assumed; group by module. That gives us something in the 20s. I don't think that is too bad, but I'm open to suggestion. The one thing I would try to avoid is editing the same file in different child issues.

I think the spreadsheet is a great idea. It is difficult to manage columns in an issue summary. Much easier to scan this way.

joelpittet’s picture

Great. I'll suggest a separate issue for each preprocess function. But to your point if it's XXXX_preprocess_field for example that, should be in one issue. eg:

  • template_preprocess_field
  • bartik_preprocess_field
  • rdf_preprocess_field(fake module override for example purposes).

And that may change a templates such as:

  • core/modules/comment/templates/field--comment.html.twig
  • core/modules/node/templates/field--node--title.html.twig
  • core/themes/bartik/templates/field--taxonomy-term-reference.html.twig
  • core/modules/system/templates/field.html.twig
  • core/themes/seven/templates/field.html.twig (fake)

Edit: but to be clear not one issue for each in theme.inc, because the patch will be a bit too big.

davidhernandez’s picture

I was thinking per module that contain the preprocesses. For example, comment module contains template_preprocess_comment and comment_preprocess_field which I believe map directly to template files that live in the comment module. comment.html.twig and field--comment.html.twig

davidhernandez’s picture

Oh, I see what you're saying. I'll have to think about that some more. Way past bed time.

davidhernandez’s picture

So my original thought was not to include changes to parent preprocess functions. I'll stick with Comment as an example. It contains:

core/modules/comment/comment.module:

  • template_preprocess_comment
  • comment_preprocess_field

These feed classes into:

  • core/modules/comment/templates/comment.html.twig
  • core/modules/comment/templates/field--comment.html.twig

If these changes are grouped together, I think it would be easier for people to conceptualize the change, and keep it consolidated to the one module. The downside is the classes originating from template_preprocess_field need to be duplicated. The 'field' class, for example, would be added to field--comment.html.twig (Which would happen regardless of approach. We don't have to change it later.) and would be a duplicate of the class 'field' coming from template_preprocess_field. The duplication won't affect markup, as Attribute processing removes duplicates, and then the upstream duplicate would go away once template_preprocess_field is changed. I agree this can only be doable given the context of making all the changes.

If we do it by parent preprocess, as listed in #10, we hit all field based preprocesses and templates together. That has the advantage of avoiding any duplication. The downside is multiple issues will be making changes to the same files. The issue we create to work on field changes and a separate issue to work on comment changes both need to hit functions in comment.module, (or they separately make changes to field--comment.html.twig) so we'll need to manage those merges. There will also be a lot hitting theme.inc

Those are my thoughts for the moment. Are there any other factors to consider, or any other approaches people can think of? Also, keep in mind the later phase of splitting out classes from the template files and the 'classy' theme, and whether we can use the same approach or not.

joelpittet’s picture

  • template_preprocess_comment
  • comment_preprocess_field

These two don't share anything in common for classes or templates. They just share being in the same file. Git will deal with fuzzing lines that are offset so that's not to worry about unless you are changing the same function or template(which we should avoid and any of those should be in the same issue).

But I don't want to hold you guys up, give that idea a try in a child issue as a test for comment, see how it goes, it's not like you can't change your mind and doing is much more productive;)

davidhernandez’s picture

Issue summary: View changes

Removing the wall of text in favor of the Google Doc.

Cottser’s picture

I think it will make more sense to handle all the field classes in one issue, so field--comment would happen in a field.module or field.html.twig issue I'd say. Also I didn't realize we'd be adding things like field--comment but it makes sense I guess.

davidhernandez’s picture

Ok, lets's work through field and node, since we already have issues being worked on for them, and they are tackling it from the preprocess direction. See what we learn. Also, identify a few more prime candidates to work on.

davidhernandez’s picture

So Cottser and I discussed this yesterday in IRC. I think we were all in agreement on the majority of it, if looking at it from different angles. I'm going to start updating the Google doc, grouping things essentially as Joel has in #10. My main concern is making sure we don't overlook any derivative functions/templates or inherited classes. Hopefully, with it organized we can make sure nothing slips through.

joelpittet’s picture

@davidhernandez don't worry about missing things, it will happen things change. New templates get added or removed, we just need to (re-)roll(patches) with it.

Feel free to go crazy with the spreadsheet for whatever purposes you see fit.

webchick’s picture

Coming here from #2326423: Move "region" classes from preprocess to template which is the first one of these I've seen. From the look of it, we're going to end up with code like this:

+{%
+  set classes = [
+    'region',
+    'region-' ~ region|clean_class,
+  ]
+%}

...at the top of every Twig template file. However, this runs counter to the Twig Documentation I've read, which is generally shows templates being 80-90% HTML and a few {{ things }} here and there. :\

Glancing through that page though, and this is probably a stupid suggestion, but is there any way we could at least do something more compact like:

{% classes: region, region-clean_class %}

Or... something? I actually thought what we talked about in Austin was hard-coding the regions in the HTML so they were easily editable, and then adding a dynamic "get the rest of the classes" thing to pull in any that were added dynamically, ala:

  <div{{ attributes }}>
+  <div class="region region-{{name}} {{ attributes.addClass(classes) }}">

But at any rate, this approach gets my "code smell" radar up. :\ Can we discuss it a bit more before going all-out on this?

mdrummond’s picture

Having a variable block at the top of the template is to my eyes actually a lot easier than trying to mix hard-coded classes in the HTML with classes added through an addClass declaration.

I'd also prefer to see each class set on one line, because for some of these, there is a ternary operation. Mixing those in a single line definition will get really messy.

Keep in mind, the reason we're doing this is to help alleviate themers from digging into preprocess functions. So this is much cleaner and easier to understand than that. It's a new pattern, yes, but if we're using it consistently, it's pretty straightforward. Want to change the classes for a template? Go to the top and you can add and remove the classes set there as needed.

The Twig team has talked back and forth on this extensive and finally settled on this. More than happy to further discuss it, though. If I or somebody else catches you on IRC, maybe we can chat about it a bit?

davidhernandez’s picture

The array set won't be at the top of every template, but certainly the ones with a good number of classes to add, or logic in the name creation. The region one we could probably make more compact, but that was made keeping the same format as the others we are working on, for consistency sake.

To add a "region-clean_class" variable we'd have to keep the logic in the preprocess function and pass everything as separate variables to the template. We'd still be essentially generating the classes in preprocess, and hiding the logic from themers, which defeats some of the purpose. Also, it would be hard to keep them compact in the cases where we have many classes. The region template is probably the smallest use case, so maybe not the best example.

<div class="region region-{{name}} {{ attributes.addClass(classes) }}">

This wouldn't work. {{ attributes.addClass(classes) }} adds the additional classes, and then prints a class attribute along with all the other attributes, resulting in something like this: <div region-footer""="" region="" class="region region-footer class=">

Field might be a better example. Because of this approach we're able to give rid of lines like

<div class="field-label{% if title_attributes.class %} {{ title_attributes.class }}{% endif %}"{{ title_attributes|without('class') }}>{{ label }}:&nbsp;</div>

and replace it with

<div{{ title_attributes.addClass('field-label') }}>{{ label }}:&nbsp;</div>

The template itself goes from this

<div{{ attributes }}>
  {% if not label_hidden %}
    <div class="field-label{% if title_attributes.class %} {{ title_attributes.class }}{% endif %}"{{ title_attributes|without('class') }}>{{ label }}:&nbsp;</div>
  {% endif %}
  <div class="field-items"{{ content_attributes }}>
    {% for delta, item in items %}
      <div class="field-item"{{ item_attributes[delta] }}>{{ item }}</div>
    {% endfor %}
  </div>
</div>

to this

{%
  set classes = [
    'field',
    'field-' ~ entity_type|clean_class ~ '--' ~ field_name|clean_class,
    'field-name-' ~ field_name|clean_class,
    'field-type-' ~ field_type|clean_class,
    'field-label-' ~ label_display,
  ]
%}
<div{{ attributes.addClass(classes) }}>
  {% if not label_hidden %}
    <div{{ title_attributes.addClass('field-label') }}>{{ label }}:&nbsp;</div>
  {% endif %}
  <div{{ content_attributes.addClass('field-items') }}>
    {% for delta, item in items %}
      <div{{ item_attributes[delta].addClass('field-item') }}>{{ item }}</div>
    {% endfor %}
  </div>
</div>

It is longer with the array block, but the markup is cleaner and a little more comprehendible, since we can avoid this pattern of hard-coding the class and butting a print up next to the close quote. (Typos and whitespace for everyone!) This also puts the logic and class name generation directly in the face of the themer. If they want to remove lines, modify them, etc, it's right there.

Note that this may not be the final set of classes. We're currently just recreating the logic and classes that already exist. The later phase involves determining which ones we can remove or simplify.

webchick’s picture

Hm. Ok, I guess go ahead and proceed then. Just unfortunate that we can't encapsulate that logic in a separate include file or something to make the templates themselves much cleaner.

mdrummond’s picture

Here's another way to think about this.

What themers really want is control over the HTML output from their site. That includes the HTML elements that are used, as well as the HTML attributes, particularly the items in the class attribute.

Putting the logic that selects which classes are added to the class attribute for a particular element in a template inside of a preprocess function means that a themer has to dig into PHP code to figure out where they can tweak that logic to get the classes they want.

Putting that class selection logic in a separate template file also adds a level of abstraction, where a themer needs to track down a separate file in order to change the HTML output of the class attribute.

So what we're proposing instead is to keep the HTML output decisions for the class attribute as close as possible to the HTML elements in question.

The function we're using addClass (and its companion, removeClass) is very intuitive to most people who work with themes, as it has a very similar syntax to equivalent jQuery functions.

In some cases, addClass is always adding the same class, and that class doesn't have any logic associated with how its name is constructed or whether or not it should be inserted. In that case, addClass can often be added right to the attribute variable on an HTML element.

However, sometimes addClass has quite a number of classes to add. Some classes may or may not get added due to a variable value, or a variable value may be incorporated into the class itself. In that case, a logic block at the top of the template allows those decisions to appear in plain sight for themers, where it is easy to see where these classes are coming from, so they can alter them or remove them if they desire.

Putting those class declarations into a separate file doesn't make the template cleaner: it removes critical HTML information from the template, the HTML of the items that appear in the class attribute.

This is very similar to the other sorts of logic that we put into a template, where we determine whether or not to show a block of HTML based on the value of some variable, or we provide alternative markup based on the value of a variable.

Does that help to clear things up?

davidhernandez’s picture

There are child issues covering all the functions now. Check the child list in the sidebar. I would say more than half are small and simple, so it shouldn't be too daunting. Morten is recruiting people to work on a bunch of them during Frontend United, so hopefully we'll see some patches go up.

I think we're close to being done with node and field. If we get those in soon, I hope the others will domino.

davidhernandez’s picture

Issue summary: View changes
davidhernandez’s picture

Issue summary: View changes
davidhernandez’s picture

Issue summary: View changes
davidhernandez’s picture

Issue summary: View changes
Cottser’s picture

valderama’s picture

Hm, after some thinking I guess the approach being applied here is quite OK. It makes adding _and_ removing classes a no-brainer. By the cost of making the template less "clean".

I think the most important use-case is to add classes - that should be really easy to accomplish. So I still think template code like this would nice as well.
<article class="{{classes}} my-classes">

To remove all the Drupal classes, one would just need to not print {{classes}}. And to remove Drupal classes selectively one would need preprocess functions, but that seems OK for me. In particular as due to the change to twig, chances are higher that themers need to dive into preprocessing anyhow.

Cottser’s picture

@valderama - the problem that we've seen with template code like that is it can easily result in "sloppy" markup like <article class=" my-classes"> when classes is empty for example. But sure you can still do it with something like this if you don't want to use addClass, though we will likely be replacing any code like this in core with addClass:

<article class="{{ attributes.class }} my-classes"{{ attributes|without('class') }}>

We are now taking advantage of the fact that most of the attributes in templates are being printed from an Attribute object and adding useful methods to those objects to allow manipulating them from the template (or preprocess) level.

davidhernandez’s picture

Seconding Cottser's comment. Also, we are avoiding creating the classes in preprocess altogether. That is the main point of this change. It is much more difficult to remove dynamically added classes when they are created in preprocess. Also, it lets themers avoid preprocess, and using PHP functions, altogether. At least for something as simple as classes. That is a big win for the themer experience. We know that many people are comfortable navigating preprocess functions and variable arrays to get the markup they want, but not everyone is.

This is only phase 1. In phase 2, we'll move most of these class assignments out of core, and into the Classy base theme. Core itself will be very clean. In the end, you will be able to take any approach you want with how you add or manage your classes. You will not have any particular approach, or set of classes, forced upon you.

xjm’s picture

Priority: Normal » Major
Jeff Burnz’s picture

Posted issue for search block, seems to be have been overlooked: #2358037: Add search form block Twig template file

davidhernandez’s picture

search_preprocess_block was covered in #2328913: Move block classes out of preprocess and into templates See comment #19. Thanks for checking though, we probably need to do another sweep as the templates and theme functions have been changing.

Jeff Burnz’s picture

@36 bummer, would have been nice for this sort of thing to be very consistent.

davidhernandez’s picture

The problem is search does not have its own template file for this. That would probably be good addition, but it was really outside the scope of these particular changes.

Jeff Burnz’s picture

Point taken, I suppose I look at it from a different perspective.

I have to add a template override, to add .removeClass('container-inline'), or do some preprocess hack, or a CSS override. Which ever way you look at this I have to do something to remove what core is providing. This is regardless of whether I use the Classy theme or not.

When we say "move classes to templates", I don't think that precludes adding new templates, we're doing that all the time e.g. menus, because thats how the new system is supposed to work. At least thats how I see it.

OK, so aside from the ideological discussion of those points, what really gets my goat is that container-inline class actually does something, i.e. display: inline, which I don't want core deciding for me. Benign classes that have no styles is one thing (meh), classes that impart layout are quite another.

FWIW I am more than happy with the current progress and decision making, this is outstanding work and I am forever grateful. The new menu template is great and I'm having lots of fun with it right now, so please don't take my moaning toooo seriously, but I do get a tad annoyed at exceptions to rules, cause we have too many already in Drupal, its makes the DX a bit of chore.

davidhernandez’s picture

I updated the search issue and removed it as a child of this meta since it is more about the template file conversion. Please discuss there. Patch added.

#2358037: Add search form block Twig template file

Jeff, regarding your concerns about container-inline, please discuss them in the phase 2 meta, or the bi-weekly Twig hangouts. (#2348543: [meta] Consensus Banana Phase 2, transition templates to the Classy theme) These are exactly the kinds of things we are trying to figure out. I'm discovering a lot of classes that are not clear cut whether we consider them functional or style. It will take some deciding to figure out how to handle them across the board.

mortendk’s picture

check and fwiw inline stuff etc where drupal have an idea that it have to change the way markup works must be burned ;)
(inline whatever etc.)
ill put it on the agenda tonight & fight the good fight

Wim Leers’s picture

Looking at the spreadsheet, I see the *_preprocess_html() functions aren't mentioned. Can anybody explain why? :) Thanks!

mortendk’s picture

cause we forgot ;) ?

davidhernandez’s picture

_html is in the spreadsheet, though no one has probably updated it in a while. The only class left in template_preprocess_html was db-offline. It is so rarely used, it didn't seem like something we wanted to move to the template.

If you are looking at Bartik's and Seven's _preprocess_html, those don't need to be changed as part of this meta. Most of their preprocess functions were out of scope because they are actual themes. Any changes to them was to maintain existing functionality.

Cottser’s picture

Another question: Can we close this meta now? As far as I can see there is just the cleanup issue left. Despite that I would consider this done! :D

davidhernandez’s picture

I was waiting for the cleanup to get finished before closing this one. I don't like loose ends. Someone pleeease review it. Then I can throw the ceremonial "closed (fixed)" switch and toss confetti in the air and have a party! :DD

Wim Leers’s picture

#43: It was an honest question, I thought there might be a reason :)

#44: Oh, right, this issue is only about removing classes. I was actually asking about removing *_preprocess_html() (and other preprocess functions) completely. Can you point me to the issue for that? Or was I just imagining that that was ever being done? :P

davidhernandez’s picture

Wim, you might be looking for this, #2035055: Introduce hook_theme_prepare[_alter]() and remove hook_preprocess_HOOK(), and its related issues.

It looks like these have all been pushed to 9.x.

joelpittet’s picture

@Wim Leers there is no plan in removing those hooks altogether for D8 for come consistency. Though if all they are doing is adding CSS classes, we will likely remove those specific preprocesses;)

Wim Leers’s picture

Many thanks to both of you! That clears things up :)

davidhernandez’s picture

Status: Active » Closed (fixed)

*tosses confetti*

All issues and the follow up are done!