[meta] Consensus Banana Phase 1, move CSS classes from preprocess to twig templates.

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.

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

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.

• 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;)

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

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?

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?

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.

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?

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.

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

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

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

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.

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

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

cause we forgot ;) ?

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

#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

@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;)

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