Move field classes out of preprocess and into templates

YES Thanx for dragging this out of the what classes do we want inside of a fields div :)
i especially love the {{ attributes.class }} this will make it easy for a module to add in a required class - best of both wolds.

This also forces the field.html.twig file & makes it easy for the themer to understand what goes on.

I think it adds complexity. I mean, if I look at markup and see a class, I expect to see it in hook_preprocess_template.
Of course next thing I check is the template. But if someone wants to see what classes my theme is removing, it'd be better if he/she only needed to check one place.

Let's test it! :)

The reason that we wants to expose the variables directly in the template is to make it clear & visibile to the themer to work with it - we dont want to "hide" thes things inside php, out in twig with it, so the themers dont even have to know its php ;)

we discussed today at the codesprint in stockholm, that it would be a good idea to have the names as a keyed array instead and save a bit of memory.
class="{{ attributes.class.foo }} {{ attributes.class.bar }} {{ attributes.class.baz }} {{ attributes.class }}"

I'd like to hear more about that, I didn't know the attributes class variable was keyed.

They are not keyed at the moment, but afaik that would be a huge win & not freak out the developers that were gonna transport each var as a single variable and not just 1 array :)

So instead of $attributes['class'][] = 'new-thingie'; I'd have to write $attributes['class']['new_thingie'] = 'new-thingie';? Ugh.

if you want to add new-thingie class you would do

<div class="new-thingie {{ attributes.class }}">....

easier for the themer :)

yeah, but I'm a module writer half of the time ;)

we have had discussions about this for 2 years - im gonna see if i can find all the documents & minuts that was written down from the many many meetings that have been done. -but yes one objective was to make it as awesome for the themer as possible.

Thats one of the core values of shifting to twig, its about empowering the themer. :)

Would class="{{attributes.class.foo}}" break if foo was removed from a preprocess?

3: drupal-field-classes-templates-2217731-3.patch queued for re-testing.

I rerolled the patch by adding classes inside ['class'] variables to make them available by separate and I added the comments on twig template. I hope it was the expected result.

Some changes:

Maybe $variables['attributes']['class']['entity_type_css'] = 'field-' . strtr($element['#entity_type'], '_', '-'); should be $variables['attributes']['class']['entity_type_css'] = strtr($element['#entity_type'], '_', '-');

Maybe $variables['attributes']['class']['field_name_css'] = $variables['attributes']['class']['entity_type_css'] . '-' . strtr($element['#field_name'], '_', '-'); should be $variables['attributes']['class']['field_name_css'] = 'field-name-' . strtr($element['#field_name'], '_', '-');

Rerolled adding @rteijeiro modifications and considering the new preprocess_field function location.

21: drupal-field-classes-templates-2217731-21.patch queued for re-testing.

Documentation included in few indented lines and I recovered the example classes removed from the #3 patch.

1. +++ b/core/includes/theme.inc
   @@ -2421,20 +2421,15 @@ function template_preprocess_field(&$variables, $hook) {
   -    $variables['attributes']['class'][] = 'clearfix';
   +    $variables['attributes']['class']['clearfix'] = 'clearfix';
   

   can we please remove clearfix completely ?
   it will be done with sass in the future & theres no reason that drupalcore puts that out (sorry im ranting but i hate it)

2. +++ b/core/modules/system/templates/field.html.twig
   @@ -16,20 +16,24 @@
   +<div class="field field-{{attributes.class.entity_type_css}}--{{attributes.class.field_name_css}} field-name-{{attributes.class.field_name_css}} field-type-{{attributes.class.field_type_css}} field-label-{{attributes.class.label_position}}{{attributes.class}}" {{ attributes }}>
   

   im thinking that we should do this with the set command in twig instead makes it easier to look at :

   in the twig template:

   {% set cssclass %}
   field 
   field-{{attributes.class.entity_type_css}}--{{attributes.class.field_name_css}} 
   field-name-{{attributes.class.field_name_css}} 
   field-type-{{attributes.class.field_type_css}} 
   field-label-{{attributes.class.label_position}}
   {% endset %}
   

   then we can call that cssclass (or whatever we call it)

   <div class="{{ cssclass }} {{attributes.class}}" {{ attributes }}> 
   ...
   </div>

I have remove the clearfix, but to avoid that the feature of the inline breaks the field display I have added to the system.theme.css the same lines that we got in the .clearfix selector to the .field-position-inline selector.

Right now we are not going to put the class 'hidden' neither 'above' because we think that it's not neccesary to show that information.

Sorry for my previous patch, here is again.

1. +++ b/core/includes/theme.inc
   @@ -2421,20 +2421,13 @@ function template_preprocess_field(&$variables, $hook) {
   -    $variables['attributes']['class'][] = 'clearfix';
   

   This issue is about moving the classes and not about changing them at the same time. Let's add or remove classes in a follow-up and focus on the actual move here.

2. +++ b/core/modules/system/templates/field.html.twig
   @@ -16,20 +16,28 @@
   + *   - class: CSS Styles.
   

   These are not styles, but classes.

3. +++ b/core/modules/system/templates/field.html.twig
   @@ -16,20 +16,28 @@
   + *     - label_position: The position of the label in relation to the field value.
   

   I believe this value is one of a set, so it's probably a good idea to document this set here so people know what values to expect.

@mortendk no, we can't act as if SASS/Compass is in core. We can make it easy to remove classes you don't use (which is usually accomplished with preprocess). If you move the classes to the templates, then you have to copy over the templates and modify them.

#facepalm wrong filename

added in clearfix to the template - were removing it in a follow up issue :)

36: 2217731-drupal-field-classes-templates-34.patch queued for re-testing.

I have delete the part of the clearfix from this patch, so it can be worked in another issue.

Here is the new patch and the interdiff.txt

Remove extra whitespace?

<div class="{{ cssclass }} {{ attributes.class }}" {{ attributes }}>

Change to:

<div class="{{ cssclass }}{{ attributes.class }}"{{ attributes }}>

I don't think we need those extra spaces, based on my understanding of Twig whitespace handling. Always possible I'm wrong.

A question here woudnt it be better for the theme if the [class] variables is a regular variables so we can test on that inside of the field ?

preprocess:

$variables['entity_type'] = $element['#entity_type'];
  $variables['field_name'] = $element['#field_name'];
  $variables['field_type'] = $element['#field_type'];

template

{% set cssclass %}
field field-{{entity_type|replace('_','-')}}--{{field_name|replace('_','-')}} 
field-name-{{field_name|replace('_','-')}} 
field-type-{{field_type|replace('_','-')}} 
clearfix
{% endset %}

The good thing here would be that the theme then could do test on the field_type etc so giving the theme more power.
im gonna check if we can grap that data out somewhere else of the string though

I looked a bit more on the patch and we actually have all the data available right now - you just have to look for it the right place.
This patch graps the data fx {{ element['#entity_type'] }} and used that instead & then in the template do a replace on the new lines & the _ to -

{% set cssclass %}
{% filter replace({'\n': ' ', '_': '-'}) %}
field
field-{{ element['#entity_type'] }}--{{ element['#field_name'] }}
field-name-{{ element['#field_name'] }}
field-type-{{ element['#field_type'] }}
clearfix
{% if element['#label_display'] == "inline" %}
field-label-inline
{% endif %}
{% endfilter %}
{% endset %}

<div class="{{ cssclass }} {{ attributes.class }}" {{ attributes }}>
...
</div>

thats pretty awesome imho & we dont end up having to do a lot of preprocess stuff now that we alreayd have the data + its easy to read as well with all the classes + removing what you dont want :)

I don't understand the fear here - we have as a core value that we dont wanna dump it down ;) - and honestly what is it thats not to understand here.

{{ element['#entity_type'] }}
{{ element['#entity_type'] }} is the value that drupal puts out, the only reason that we dont write is as {{ element.entity_type }} is because of the issue of not beeing able to drill into #, as twig understands this as a comment - this is a general issue that we have, which is sucky.

After thinking a bit about i dont think it make sense to have a specific attributes.class.foo ( $variables['attributes']['class']['entity_type_css'] = strtr($element['#entity_type'], '_', '-');) it makes the strings very long {{ attibutes.class.entity_type_css }} & we adding this data in, when we already have it in {{ element['#entity_type'] }}.

if we do the strstr in the preprocess (and i can see the reason for that), we should not hide it down in {{ attibutes.class.entity_type_css }} then lets add them at a higherlevel instead fx {{ entitytype_safe }} if we wanna make it easier to read

Doing a filter on the class that we have set, is normal twiging, thats a feature in the language:
{% filter replace({'\n': ' ', '_': '-'}) %} the replace \n is to make the {{ cssclass }} more readable by having each part of the class on its own line, to make it very easy to read & understand whats the values are available instead of one long string.

ok after sleeping a bit on it it comes down to the problem, that we have a lot of classet atm thats added to a field, and we still havent taken the disucssion about what we actually want there (and that will be a seperate issue - so dont start it here) so it quickly gets confusion whats actually going on here

i see 2 approaches to this :
set class
inside the template - here were using what fields already provide to us form fields & everythings is done with twig. It do look a bit more scary, but its epic power, each class is split into a new line to make it easy to figure out whats going on.

{% set cssclass %}
{% filter replace({'\n': ' ', '_': '-'}) %}
field
field-{{ element['#entity_type'] }}--{{ element['#field_name'] }}
field-name-{{ element['#field_name'] }}
field-type-{{ element['#field_type'] }}
clearfix
{% if element['#label_display'] == "inline" %}
field-label-inline
{% endif %}
{% endfilter %}
{% endset %}

Preprocess some & add vars
The other approach would be to preprocess the _ to - to make it a bit cleaner and put them out into vars - not the {{attributes.class.entitype}} it gets way to long imho & is not easier to read.

preprocess:

 $variables['entitytype_css'] = strtr($element['#entity_type'], '_', '-');
  $variables['fieldname_css'] = strtr($element['#field_name'], '_', '-');
  $variables['fieldtype_css'] = strtr($element['#field_type'], '_', '-');
  $variables['label_display_css'] = strtr($element['#label_display'], '_', '-');
  // Add a "clearfix" class to the wrapper since we float the label and the
  // field items in field.module.css if the label is inline.
  if ($element['#label_display'] == 'inline') {
    $variables['label_display'] = $variables['label_display'] . ' clearfix';
  }

then the template would be something like this:

<div class="field field-{{ entitytype_css }} field-{{ entitytype_css }}--{{fieldname_css}} field-name--{{ fieldname_css }} field-type-{{ fieldtype_css }} {{ label_display_css }} {{ attributes.class }}" " {{ attributes }}>

or do a combination

{% set cssclass %}
{% filter replace('\n': ' ') %}
field field-{{ entitytype_css }} 
field-{{ entitytype_css }}--{{fieldname_css}} 
field-name--{{ fieldname_css }} 
field-type-{{ fieldtype_css }} 
{{ label_display_css }} 
{% endfilter %}
{% endset %}

<div class="{{ cssclass }} {{ attributes.class}}" {{attributes }}>
...
</div>

Its 3 different approaaches that gives the themer a ton of flexibility & we dont break modules ability to add in classes if its neede

Personally, I prefer the preprocess approach. That still gives us variables we can use to construct classes within the template, it's just that the messy work with strings isn't right out in the open.

I don't see a compelling use case for being able to handle that string manipulation within the template. If there are specific examples of how that epic power would be wielded, I'm glad to listen. I do think that one of the other principles is trying to solve problems for 80% of the people, and I think that creating the variable names in preprocess gets much closer to that sweet spot.

I'm fairly familiar with how Twig works now, and I find that string manipulation code intimidating. I think that new people are even more likely to be nervous when they see that and get worried they might break something.

The people who wouldn't be intimidated by that are also probably people who wouldn't be intimidated by looking at preprocess functions, so I think they'll still be able to do powerful things if they'd like to do so.

I'd suggest tweaking the variable names to be a little more friendly. I agree that the css isn't needed: that's a bit confusing, as what these are is elements of class names. We don't want people getting confused and thinking that somehow some css is in the variable.

Here's my suggestion for the approach:

Preprocess

 $variables['entity_type'] = strtr($element['#entity_type'], '_', '-');
  $variables['field_name'] = strtr($element['#field_name'], '_', '-');
  $variables['field_type'] = strtr($element['#field_type'], '_', '-');
  $variables['label_display'] = strtr($element['#label_display'], '_', '-');
  // Add a "clearfix" class to the wrapper since we float the label and the
  // field items in field.module.css if the label is inline.
  if ($element['#label_display'] == 'inline') {
    $variables['label_display'] = $variables['label_display'] . ' clearfix';
  }

Template

<div class="field field-{{ entity_type }} field-{{ entity_type }}--{{field_name}} field-name--{{ field_name}} field-type-{{ field_type}} {{ label_display}} {{ attributes.class }}" " {{ attributes }}>

It's entirely possible that I'm missing something, and that it isn't okay to have a variable key within $variables that has the same name as a key within $elements. If not, though, I think this is pretty clear.

Overall, this is a great idea. I'd be very happy to across this template when working on a theme, as it would make it super easy to delete all those awful class names as quickly as possible. :)

a reroll and back to simpler times

<div class="field field-{{ entity_type }}--{{ field_name}} field-name-{{ field_name }} field-type-{{ field_type}} {{ attributes.class }}" {{ attributes }}>

theme.inc:

  $variables['entity_type'] = strtr($element['#entity_type'], '_', '-');
  $variables['field_name'] = strtr($element['#field_name'], '_', '-');
  $variables['field_type'] = strtr($element['#field_type'], '_', '-');
  $variables['label_display'] = strtr($element['#label_display'], '_', '-');

new patch removed entity view stuff ;)

+++ b/core/modules/system/templates/field.html.twig
@@ -16,20 +16,22 @@
+<div class="field field-{{ entity_type }}--{{ field_name}} field-name-{{ field_name }} field-type-{{ field_type}} {{ attributes.class }}" {{ attributes }}>

missing space after field_name and field_type.

This is just putting spaces where they were missing from.

When I apply this patch, the markup is slightly different that the before-patch markup.
1. class .field-label-hidden is no longer showing in the string of classes on the main wrapper div.field.
2. There may be some minor whitespace-type cleanup to do (2 whitespaces, instead of just one)
Hope that helps.

I'm going to take an attempt at this.

Fixed trailing white space in twig file for simpletest.

How's this? Added an interdiff to make it easy to review.

Go, Testbot!

Tested this manually, the markup and the documentation of the variables in the file comment both look good.

Sorry for kicking this back, but :

+++ b/core/includes/theme.inc
@@ -2371,16 +2371,10 @@ function template_preprocess_field(&$variables, $hook) {
+  $variables['entity_type'] = strtr($element['#entity_type'], '_', '-');
+  $variables['field_name'] = strtr($element['#field_name'], '_', '-');
+  $variables['field_type'] = strtr($element['#field_type'], '_', '-');
+  $variables['label_display'] = strtr($element['#label_display'], '_', '-');

+++ b/core/modules/system/templates/field.html.twig
@@ -16,20 +16,23 @@
+ * - entity_type: The entity type to which the field belongs.
+ * - field_name: The name of the field
+ * - field_type: The type of the field.
+ * - field_label: The display of the label for the field.

That's not true, strtr($field_name, '_', '-') is not "the field name".

If we put the "CSS friendly strings with _ replaced by -" in the $variables available in the template, we have to give them non-misleading names, and we shouldn't lie in their description in the template doc :-)

Any problem with this?

• css_entity_type: The entity type to which the field belongs, in a CSS friendly format.
• css_field_name: The name of the field, in a CSS friendly format.
• css_field_type: The type of the field, in a CSS friendly format.
• css_field_label: The display of the label for the field, in a CSS friendly format.

I'm not a fan of prepending the variables with 'css_'. I can't see many cases where you would need the variable name with the '-' and the css friendly variable name with the '_' in the twig file. I think we should leave the variable names as is.

I do like the proposed descriptions though.

• entity_type: The entity type to which the field belongs, in a CSS friendly format.
• field_name: The name of the field, in a CSS friendly format.
• field_type: The type of the field, in a CSS friendly format.
• field_label: The display of the label for the field, in a CSS friendly format.

Can someone else confirm this?

Just using $entity_type, $field_name ... as var names would still be lying. +1 on #67

I'm not actually sold on the clarification in the variable name being necessary, but it doesn't bother me either way. I would just hate to see this issue held up by a minor semantic issue.

Personally I think that the variables would be better named without the prefix. The prefix doesn't really add any clarification in this context (there isn't an unfriendly version here to get it confused with), and detailed documentation is immediately above their usage. As a very minor point, keeping variable names concise and unique is a win for being able to quickly read over the template file and knowing precisely what is going on.

Sorry, I still disagree. If we start applying the pattern of "print classes directly in templates" to other templates than field.html.twig, then naming the CSS-friendly variants of "things" the same name as the "thing" would end up very confusing.

We prepare $variables['some_var'] = strtr($element['#field_name'], '_', '-'); only because $some_var is specifically intended to end up in a "class" attributes. It's not "the field name" as a generic-purpose string, and it's not "the field name" as the rest of the code flow uses it. It's "the field name specifically massaged to be usable as a CSS class". Accuracy of non-misleading var names wins over saving 4 chars in a var name.

I think you make a good point regarding consistency in other areas. Consistency is good for sanity, and sanity is good for programmers. Point conceded, I think your suggested naming is the correct path forward.

I see your point. I will re-roll the patch with the suggested changes in #67. Thanks for the feedback.

Well, also, #2004252: node.html.twig template just went in, which did this to node.html.twig :
http://cgit.drupalcode.org/drupal/diff/core/modules/node/templates/node....

-<article id="node-{{ node.id }}" class="{{ attributes.class }} clearfix"{{ attributes|without('id', 'class') }}>
+<article{{ attributes }}>

So do we really want to go in the opposite direction here ? What's the global strategy regarding classes in templates ?

There is definately some merit to this idea.
Though for consistency sake, clean templates I'm not on board.
There is a lot more power and control in the preprocess to generate these classes and attributes. Also rdf and many contrib modules rely on manipulating them. As well

One key idea in twig is you aren't really supposed to create data , rather get data in and format it's output..

This brings to light two things to me, the attributes API Needs some more helpers methods to make it easier to manipulate the data. And to a lesser extent it seems also that empty attributes is not a real concern for classes.
@webchick mentioned in passing maybe we should always treat classes as the special flower they are and by looking at this approach you've all taken maybe she is right.

There was a big meeting of frontenders at DrupalCon and we hashed out a plan for dealing with classes and CSS in Core. There's a picture somewhere of mortondk and JohnAlbin shaking over the plan—it was all very official—and there was supposed to be a meta issue going up here to explain it all, but it looks like that may not have happened yet.

In any case, phase 1 of the plan was to move all the default classes for all the core modules out of preprocess functions and into their respective twig templates. However, we also noted that in doing this we still need to support the classes variable, so we don't break things like RDF. There was widespread consensus that this alone would be a good thing for themers. This step is also required for the rest of the plan to work, but I'm not sure here is the best place to get into that meta issue.

The meta issue wil be up in a couple of days (hopefully in about 24 hours)

But yes first thing is to make sure that we move classnames out of the preprocess, where we don't have any control over them (unless we use regex) and move them into the template files instead.

Theres no reason to not have classnames inside the templates besides of "we used to have them in the preprocess".
It gives us flexibility & makes it very easy for us to build new frontend architecture where mistakes we do now, like fx using a bem naming scheme, that probably will be outdated in 18 months, is very easy to correct.

Modules will still have the power to add in whatever class the need as we keeps the {{ attributes.class }} so to me this is a no brainer & will make sure that we dont lock us self down in mistakes we think are good now (like fx having 960 grid in core, or bootstrap or whatever is the hot thing today)

Honestly this is the only way we can make the frontend flexible + this have been discussed for 5 years now, everybody have been heard.

Unless somebody can show a usecase where moving classnames into the template isnt a good idea, i really think we need to move forward and get it done.
I we dont, we can just as well drop twig and just go for headless drupal.

Adding the last agreed upon changes to the patch, and marking it for testing. Though I feel this is blocked until meta issue mentioned in #78 gets posted.

no talk behind closed doors - john is taking the lead on it & he had to fly home , and sleep first then it gets out :)

Related, comment field rendered differently #1962846-32: Use field instance name for header of comment list, drop comment-wrapper template

+++ b/core/includes/theme.inc
@@ -2371,16 +2371,10 @@ function template_preprocess_field(&$variables, $hook) {
+  $variables['css_entity_type'] = strtr($element['#entity_type'], '_', '-');
+  $variables['css_field_name'] = strtr($element['#field_name'], '_', '-');
+  $variables['css_field_type'] = strtr($element['#field_type'], '_', '-');
+  $variables['css_label_display'] = strtr($element['#label_display'], '_', '-');
+++ b/core/modules/system/templates/field.html.twig
@@ -16,20 +16,23 @@
+ * - css_entity_type: The entity type to which the field belongs, in a CSS friendly format.
+ * - css_field_name: The name of the field, in a CSS friendly format.
+ * - css_field_type: The type of the field, in a CSS friendly format.
+ * - css_label_display: The display of the label for the field, in a CSS friendly format..

This probably wont be a popular observation, but I wonder what does the "css_" prefix add to themers experience? The original variables (the ones with the underscores) aren't accessible in the twig file and for all intents and purposes aren't very useful to themers, so what's the use to distinguish this in the variable name? Yes, the "css_" prefix is used to signify that we've converted underscores to hyphens, but this is already made clear in the description in the comment section of the template. Seems important to discuss this now, because this could set the precedent for use in other to-be-created issues for migrating classes into templates.

@jwilson3: see #72.

What if we add a twig filter a la {{ entity_type|c }}, that could turn underscores into hyphens on the backend. Twig engine could replace c with a call to drupal_html_class. We could even progressively add support to the "c" filter to handle objects and arrays automatically using _toString() or similar.

I know I've had needs where I want to turn a string value into a class name in the past. Would be uber-cool to have a shortcut to do this in the template itself.

Just wanted to note that the way we're going to be adding classes to templates is being worked on in #2285451: Create addClass() and removeClass() methods on Attribute object for merging css class names..

@jwilson3 see #2283301: Add Twig filters for drupal_html_class() and drupal_clean_id_identifier() for that functionality.

@cottser Awesome! So if that patch were to get in, would there be any reason to NOT move the "css_" prefix stuff in this patch out of preprocess into the template?

The hash keys (#) are kinda yucky because you can't use Twig's dot syntax to access them.

With the meta issue #2322163 now posted I think I see a path forward with this.

Functionally tested patch #94 and passed.

Looking good, here's my review. Thanks for the patch @aczietlow

1. +++ b/core/modules/system/templates/field.html.twig
   @@ -16,22 +16,37 @@
   - * - attributes: HTML attributes for the containing element.
   ...
   + * - attributes: HTML attributes for the containing element.
   

   You can leave this at the top.

2. +++ b/core/modules/system/templates/field.html.twig
   @@ -16,22 +16,37 @@
   + *  - entity_type: The entity type to which the field belongs.
   + *  - field_name: The name of the field.
   + *  - field_type: The type of the field.
   + *  - label_display: The display of the label for the field.
   

   These variables shouldn't be indented. Seem to have one space infront.

3. +++ b/core/modules/system/templates/field.html.twig
   @@ -16,22 +16,37 @@
   +    'field-label-' ~ label_display|clean_class
   

   Don't think #label_display needs |clean_class as it didn't before.

   clean_class doesn't map to strtr() either so make sure the output is as expected with _ converted to -.

4. +++ b/core/modules/system/templates/field.html.twig
   @@ -16,22 +16,37 @@
   +<div {{ attributes.addClass(classes, attributes.class) }}>
   

   No space needed in front of the twig print statement. And don't need to add attributes.class because it already is there and it's just going to merge classes.

5. +++ b/core/modules/system/templates/field.html.twig
   @@ -16,22 +16,37 @@
   -    <div class="field-label{% if title_attributes.class %} {{ title_attributes.class }}{% endif %}"{{ title_attributes|without('class') }}>{{ label }}:&nbsp;</div>
   +    <div class="field-label"{{ title_attributes }}>{{ label }}:&nbsp;</div>
   

   This is a good change because there doesn't need that if statement, thank you. Though you should just use addClass() here too.
   

Updated this per @joelpittet comments in 96 and the Google doc from the meta. Also updated the issue summary to include the preprocess functions and twig templates modified.

Test please!!!

Thanks @crowdcg those I think are almost perfect accept my first comment below of the missing.

+++ b/core/modules/comment/templates/field--comment.html.twig
@@ -22,7 +22,19 @@
+    'field-type-' ~ field_type|clean_class,
+    'field-label-' ~ label_display
+    'comment-wrapper'

Needs a comma after the label_display, will likely fail.

+++ b/core/modules/system/templates/field.html.twig
@@ -23,17 +23,32 @@
-    <div class="field-label{% if title_attributes.class %} {{ title_attributes.class }}{% endif %}"{{ title_attributes|without('class') }}>{{ label }}:&nbsp;</div>
+    <div{{ title_attributes.addClass('field-label') }}>{{ label }}:&nbsp;</div>

Thank god for this cleanup! All those if statements are to remove the space! Completely unnecessary a ternary would have done nicely even.

@joelpittet thanks for seeing that. Added the missing comma.

The test failed because of the whitespace introduced between the array and the markup. I'm uploading a patch with it removed to verify. Also, there were a few other spots with classes to convert.

Nice sleuthing. I've no problems with this patch. It looks like a very nice cleanup/improvement. And gives a strong example for the rest of the banana movement.

All good things:

1. +++ b/core/modules/system/templates/field.html.twig
   @@ -43,14 +42,13 @@
   -      <div class="field-item"{{ item_attributes[delta] }}>{{ item }}</div>
   +      <div{{ item_attributes[delta].addClass('field-item') }}>{{ item }}</div>
   

   Good catch! That will fix a bug if item attributes are added they'd get duplicate class attribute. Without|without:)

2. +++ b/core/themes/bartik/templates/field--taxonomy-term-reference.html.twig
   @@ -26,14 +26,13 @@
   -  <ul class="links field-items {{ content_attributes.class }}"{{ content_attributes|without('class') }}>
   +  <ul{{ content_attributes.addClass('links', 'field-items') }}>
   

   Love it!

3. +++ b/core/themes/bartik/templates/field--taxonomy-term-reference.html.twig
   @@ -26,14 +26,13 @@
   -      <li class="taxonomy-term-reference-{{ delta }}"{{ item_attributes[delta] }}>{{ item }}</li>
   +      <li{{ item_attributes[delta].addClass('taxonomy-term-reference-' ~ delta) }}>{{ item }}</li>
   

   Awesome!

This is very nice. Great work.

Cottser, mdrummond and I talked about this in IRC. If everyone likes the patch this one can get committed straight away, to at least serve as a good example for changing the other preprocess functions.

I read through this issue and it seems like a good thing but I think there should be a change notice and succinct documentation for people to read before it goes in.

What would the documentation be on?

Same as any change notice.
"This is how things were done in D7"

"This is how we do things now in D8"

Also helpful links to twig basics, like what the ~ does.

Most importantly, it lets people who follow the change notice twitter account know that there is something new to learn.

There are going to be a couple dozen similar issues to this one, so I guess the question if we want a change notice for each one. That could make the change notices account a bit noisy. I agree that it's good to explain this new approach, just not sure if a change notice for each of these template changes would make sense.

Well, isn't this first one? I wasn't implying this would be done for EVERY template. Just one for the new approach of doing things for templates in general.

Yeah, definitely do not want to have to create a change record for every one of these changes. I was assuming we'd just make a change record to go along with the meta issue, but this is going in first, so I guess one here makes sense. I'll make one, unless someone would like to beat me to it.

Sounds good if you want to write that up David.

I started drafting a change record. In the process I discovered there are couple more class changes needed.

https://www.drupal.org/node/2325067

Ok apparently, there are a number of things to fix in this still after a recent IRC conversation. Working on a patch now.

Nice work on this one folks!

If we only want to do one change record for this entire shift, I don't think it should talk about field classes in the title. Something more like "Most CSS classes moved from preprocess to Twig templates" would be a more accurate description except it's not yet true. Or maybe something like "Default CSS classes will be defined in Twig templates instead of preprocess"? Either way I guess we can make it future tense and then edit the change record after we are "done".

I think @tim.plunkett is talking about https://twitter.com/drupal8changes by the way, and I think it's helpful to think of the title of the change record as something that can be made sense of on its own.

In addition to @tim.plunkett's note about linking to Twig's concatenation documentation, the change record should also link to the addClass/removeClass change record to explain that part.

I fleshed out the change record, removed Field form the title, and made the title more present tense.

https://www.drupal.org/node/2325067

Also, added links to documentation and previous issues.

Looks really good, thank you so much @davidhernandez! My only nitpick is that the CR title shouldn't end in a period.

Removed period.

@crowdcg is on some changes still. We missed a couple class attributes in the preprocess_field for title_attributes and content_attributes. I'll do more thorough pass over the preprocess's related to this to see once he's got that patch in here.

Removed tag for needs change record.
@tim.plunkett and @dead_arm want to give the change record a boo to see if it seems it will do the trick?
https://www.drupal.org/node/2325067

Just when you think your done...

Did a bunch of clean up and think I got everything. However, I'm getting a trailing whitespace when doing logic. Not sure how to get rid of it, or not get it there in the first place. Please see example below.

field.twig.html

{%
  set title_classes = [
    'field-label',
    label_display == 'visually_hidden' ? 'visually-hidden'
  ]
%}
...
<div{{ title_attributes.addClass(title_classes) }}>{{ label }}:&nbsp;</div>

Output

<div class="field-label ">Body:&nbsp;</div>

+++ b/core/modules/node/templates/field--node--title.html.twig
@@ -15,4 +15,13 @@
-<span{{ attributes }}>{{ items }}</span>
+{%
+  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|clean_class
+  ]
+%}
+<span{{ attributes.addClass(classes) }}>{{ items }}</span>

I think this syntax would be better in the form:

{%
  attributes.addClass([
    'field',
    'field-' ~ entity_type|clean_class ~ '--' ~ field_name|clean_class,
    ...
  ])
%}
<span{{ attributes }}>{{ items }}</span>

Why? Because if you want to remove all the classes, all you have to do is copy the template to your theme and delete everything between the {% %}. The way it's coded now you'd have to do that plus also remove the .addClass(classes) to have clean and logical code.

@jwilson3's idea sounds good, maybe you guys can discuss that a bit on IRC?

1. +++ b/core/themes/bartik/templates/field--taxonomy-term-reference.html.twig
   @@ -22,13 +22,20 @@
   +    field_type == 'taxonomy_term_reference' ? 'field-label',
   

   This check is not needed, bartik preprocess did that because there are no suggestions for preprocess functions.

2. +++ b/core/modules/comment/templates/field--comment.html.twig
   @@ -28,21 +28,21 @@
   -    'field-label-' ~ label_display,
   +    'field-label-' ~ label_display|clean_class,
   
   +++ b/core/modules/node/templates/field--node--title.html.twig
   @@ -21,7 +21,7 @@
   -    'field-label-' ~ label_display
   +    'field-label-' ~ label_display|clean_class
   
   +++ b/core/modules/system/templates/field.html.twig
   @@ -39,12 +39,19 @@
   -    'field-label-' ~ label_display
   +    'field-label-' ~ label_display|clean_class,
   
   +++ b/core/themes/bartik/templates/field--taxonomy-term-reference.html.twig
   @@ -22,13 +22,20 @@
   -    'field-label-' ~ label_display,
   ...
   +    'field-label-' ~ label_display|clean_class,
   

   None of these label_display need the clean_class filter because they weren't using them before. I'd avoid making that change in this patch.

3. +++ b/core/modules/system/templates/field.html.twig
   @@ -39,12 +39,19 @@
   +{%
   ...
    %}
   

   You can use whitespace modifiers to remove the space. {% becomes {%- to remove the whitespace before. @see http://twig.sensiolabs.org/doc/templates.html#whitespace-control

@jwilson3 I can totally see the benefit of that, great idea!

@joelpittet

1. Thanks, so just changing that to add the 'field-label' since we're already in the taxonomy term reference.

2. Fair enough, just seemed inconsistent in the use of - over _ in the classes. Currently in D8 the attributes div displays the class as field-label-visually_hidden and in the title_attributes div displays as the class visually-hidden.

3. Awesome, will add those where needed. @davidhernandez also probably need to explain this in and update the change record.

I will have time to post an updated patch this afternoon, unless someone is feeling eager and wants to beat me to it!

@crowdcg, regarding 2, just make sure the class names come out the same as they were previously. We aren't changing any CSS or JS, so if something is already expecting it with the underscore we can leave it for now. Let me know if you need help with the whitespace. It looks like that is the only reason the tests failed.

If the whitespace is caused by the ternary IF in the array, that means an empty array element is being added when it evaluates to false and that results in a blank space?. Is there any way to stop that? @joelpittet, I think the whitespace problem may also go back to when addClass was added, and the array_filter was removed. I am concerned that if there is an IF in the middle, or beginning, of the array, will an extra blank space show up there as well? I'll try to test that.

Regarding #126 I don't quite agree with the reasoning that it would make things easier for people; however, it does get rid of a variable, which I am totally onboard with.

Changes incorporated from 126-129.

Always forget to change status...

@crowdcg I agree we should be using the clean_class a bit more consistently. But not in this issue, it becomes scope creep and the I'd rather the bare necessities now and the cleanup in a quick follow up issue that can bikeshed or deal with the test failures, etc.

We may need to axe #126 unless we're doing something wrong or missing some other operator. It doesn't seem you can manipulate a variable separate from printing it. I could only get the following to work:

{%
  set attributes = attributes.addClass([

Not sure we would want to do that over just using the 'class' variable.

Also, this whitespace problem isn't going away. The modifiers are being applied to the whole attribute, not specifically to just the output of the class attribute. I think we need an array_filter in addClass or somewhere to remove the empty array elements. Unless there is a way to stop the array from adding empty elements for ones that evaluate false.

Here is a dump of the title_attribute:

["class"]=>
    object(Drupal\Core\Template\AttributeArray)#1755 (2) {
      ["value":protected]=>
      array(3) {
        [0]=>
        string(11) "field-label"
        [1]=>
        string(6) "inline"
        [2]=>
        string(0) ""
      }
      ["name":protected]=>
      string(5) "class"
    }

Ok, undoing the suggested change per @jwilson3, sorry but as @davidhernandez shows above, this seems too messy.

Also, added trailing commas in arrays for @cottser, and cleaned up inaccurate comments.

Lastly, in order to get rid of the trailing spaces we really need adding-arrayfilter.patch to get in, which was pulled out before #2285451 was committed. After many attempts, we were still unsuccessful in getting rid of the extra space. This issue clearly shows the reason we need this added.

Just ran the 2 failing tests locally after applying adding-arrayfilter.patch and they pass.

Oh awesome, I'm really happy the trailing commas work, thank you @crowdcg! I know we did an upstream Doctrine patch earlier in the cycle to fix that with annotations and make those work with trailing commas.

Do the trailing commas work with the syntax proposed in #126 as well? I guess it should but I'm curious if this would work (passing multiple arguments rather than passing an array as the first argument):

{%
  attributes.addClass(
    'field',
    'field-' ~ entity_type|clean_class ~ '--' ~ field_name|clean_class,
    ...,
  )
%}
<span{{ attributes }}>{{ items }}</span>

Oh I missed the part where it wouldn't work, I had suspicions about that. So nevermind :)

I think we need a separate issue for adding-arrayfilter.patch please. Then we can add tests and get it RTBC and committed.

Added another issues #2326397: Add filtering to AttributeArray

The filter is in.

#2326397: Add filtering to AttributeArray

field--node--title.html.twig needs the visually-hidden logic, which it normally inherits from template_preprocess_field. It is also missing the inline label logic, but I can't think of why it would need it. You can't make the comment label inline. Everything else looks good.

Sorry, I meant field--comment.html.twig, not node title.

Updated per @davidhernandez's comments.

Ok, anything else we can find wrong? If not, I'm inclined to RTBC.

This is looking good to me.

Maybe we should do some manual testing to make sure we're getting the source code output?

gonna get some bodys on this from copenhagen :)

It seems official tag is FUDK (kinda weird, BTW)

Should we take the proposed solution to review this patch?

It seems that these are not the classes included in the patch:

"field-{{ name }} field-type-{{ fieldtype }} .field-label-{{ label-position }} {{ attributes.class }}"

No, the issue summary needs changing. You may not want to tackle this one at front end united, as this issue has a rather long history to it.

So what is needed is issue summary update per #155, and manual testing per #149.

I manually checked the markup for this in Bartik and Stark with a term reference, text field, and comments, pre and post patch. The only thing I noticed is in Bartik's field--taxonomy-term-refernce.html.twig. The clearfix class is applied always, not just when label_display = inline.

Pre:
<div class="{{ attributes.class }} clearfix"{{ attributes|without('class') }}>

Post:
label_display == 'inline' ? 'clearfix'

It is not an inconsequential difference. There is a noticeable spacing difference.

Also, this can be removed from the comment block in field--taxonomy-term-refernce.html.twig, since the function was removed completely.

* @see bartik_preprocess_field()

+++ b/core/includes/theme.inc
@@ -2064,29 +2064,11 @@ function template_preprocess_field(&$variables, $hook) {
-  // Add default CSS classes. Since there can be many fields rendered on a page,
-  // save some overhead by calling strtr() directly instead of
-  // drupal_html_class().
-  $variables['entity_type_css'] = strtr($element['#entity_type'], '_', '-');
-  $variables['field_name_css'] = strtr($element['#field_name'], '_', '-');
-  $variables['field_type_css'] = strtr($element['#field_type'], '_', '-');

I'm wondering whether we should just keep and use these _css variables to "save some overhead" instead of using |clean_class, because there can indeed by many fields rendered on a page.

RE: #161, if we do that, we hard code them. If someone doesn't want them, and removes them from the template, they still get generated. And when we move these out of the core template, and into the Classy template, they would still be there even though the core/Stark themes don't use them.

I think what Cottser is saying, and correct me if I'm wrong, is that we do the cleanup of the variable in preprocess, but still send it to the template to get added with addClass. I guess it is more efficient to do the cleanup in preprocess than in the template with clean_class?

It should be more efficient from a performance perspective because strtr() is doing a lot less things than drupal_clean_css_identifier() - which is doing a more complex strtr() and two preg_replace().

@davidhernandez is the concern over the variables being irrelevant to the core templates in phase 2? If so we could consider moving the variable setup to a classy preprocess function.

So I'm basically proposing this:

+++ b/core/modules/comment/templates/field--comment.html.twig
@@ -22,17 +22,34 @@
+{%
+  set classes = [
+    'field',
+    'field-' ~ entity_type_css ~ '--' ~ field_name_css,
+    'field-name-' ~ field_name_css,
+    'field-type-' ~ field_type_css,
+    'field-label-' ~ label_display,
+    label_display == 'inline' ? 'clearfix',
+    'comment-wrapper',
+  ]
+%}

Instead of:

+++ b/core/modules/comment/templates/field--comment.html.twig
@@ -22,17 +22,34 @@
+{%
+  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,
+    label_display == 'inline' ? 'clearfix',
+    'comment-wrapper',
+  ]
+%}

Having said that, it's probably worth doing some actual profiling before making this decision. I can take care of that soon.

And also I'm not proposing we do this for all templates, but field can really be used a lot so it may need some additional optimization.

Yes, that is the approach I was first thinking of in the node issue, to avoid the isSticky stuff by adding it as a variable. But, as you noted, after phase 2, these variables would be of no use to core. Unless you think it is worth having them there just in case someone might want to use them, but I don't think we do that with anything else, do we?

I could see doing it in the Classy preprocess to optimize it. They will definitely be used there, though I could argue the same problem. If someone doesn't want them, they are still being generated.

Once field classes are involved, let's get more eyes on #2229355: Field template suggestions are colliding

Some profiling would be nice here, yes. IIRC we replaced drupal_html_class() with strtr() here in D7 because there was a non-negligible performance impact - see #652246: Optimize theme('field') and use it for comment body, where this was done.

Maybe things are different in D8, but let's make sure before reverting that.

I gave the profiling a shot. These are my results using the patch in #160 and then modifying it to use the variable class names in preprocess, as HEAD is currently doing. This is on my MacBook Pro with MAMP and PHP 5.4.26.

8.0.x

=== 8.0.x..8.0.x compared (5405db0a4f9c5..5405dc66ec861):

ct  : 134,921|134,921|0|0.0%
wt  : 569,165|567,780|-1,385|-0.2%
cpu : 562,618|561,326|-1,292|-0.2%
mu  : 26,645,064|26,645,768|704|0.0%
pmu : 26,730,512|26,731,184|672|0.0%

8.0.x versus the patch in 160

=== 8.0.x..2217731-160-profiling compared (5405db0a4f9c5..5405dcaed1fb7):

ct  : 134,921|137,873|2,952|2.2%
wt  : 569,165|579,316|10,151|1.8%
cpu : 562,618|572,786|10,168|1.8%
mu  : 26,645,064|26,705,552|60,488|0.2%
pmu : 26,730,512|26,791,256|60,744|0.2%

8.0.x versus the patch in 160 with _css variables in preprocess

=== 8.0.x..2217731-160-cssvar-profiling compared (5405e1ee57a86..5405e2c78e5bb):

ct  : 134,921|137,541|2,620|1.9%
wt  : 570,027|579,452|9,425|1.7%
cpu : 563,315|572,916|9,601|1.7%
mu  : 26,645,768|26,695,568|49,800|0.2%
pmu : 26,731,184|26,780,840|49,656|0.2%

I kept getting APC warning messages. I don't know if there is something wrong with my setup, so I disabled APC and ran it again.

8.0.x

=== 8.0.x..8.0.x compared (5405e58c7d324..5405e6115329d):

ct  : 145,958|145,958|0|0.0%
wt  : 874,291|874,368|77|0.0%
cpu : 859,368|859,513|145|0.0%
mu  : 45,359,816|45,359,816|0|0.0%
pmu : 45,388,384|45,388,384|0|0.0%

8.0.x versus the patch in 160

=== 8.0.x..2217731-160-profiling compared (5405e58c7d324..5405e66149743):

ct  : 145,958|148,910|2,952|2.0%
wt  : 874,291|887,407|13,116|1.5%
cpu : 859,368|872,615|13,247|1.5%
mu  : 45,359,816|45,394,192|34,376|0.1%
pmu : 45,388,384|45,422,632|34,248|0.1%

8.0.x versus the patch in 160 with _css variables in preprocess

=== 8.0.x..2217731-160-cssvar-profiling compared (5405e58c7d324..5405e7a9b8059):

ct  : 145,958|148,578|2,620|1.8%
wt  : 874,291|884,342|10,051|1.1%
cpu : 859,368|869,438|10,070|1.2%
mu  : 45,359,816|45,392,824|33,008|0.1%
pmu : 45,388,384|45,421,432|33,048|0.1%

I'll try to profile php 5.5 as well, but I need to compile a binary for it first.

I forgot to explain the page. I added every type of field to my content type and multiple values for ones that allow. So this is rendering about 20 fields with over 70 items.

@davidhernandez: Thanks ! 20 fields is not enough though IMO.

The most critical case are listing pages. So we should try a view with a simple query, displaying 20 node teasers, each teaser displaying 5 fields - that's realistic use case ?

(Using a custom page callback that displays 20 node teasers rather than a view would actually allow a more accurate comparison, by not diluting the perf cost into the general Views overhead, actually)

Not sure whether devel_generate currently works on HEAD, but #2320157: Generate placeholder content for Field types would help generating the nodes.

No need to go with various field types, simple number fields would be enough.

Also, we don't want to measure the case of render cache hits, of course :-)

Ok, I tried this again. A created a content type with five fields and generated 100 nodes. I then created a custom page displaying teasers for all 100 nodes. Including the node title, that should be 600 field items rendered.

8.0.x

=== 8.0.x..8.0.x compared (5409bad376a91..5409bc8f45fb0):

ct  : 1,077,489|1,077,489|0|0.0%
wt  : 4,000,885|4,005,178|4,293|0.1%
cpu : 3,980,636|3,984,993|4,357|0.1%
mu  : 34,484,056|34,484,056|0|0.0%
pmu : 35,337,224|35,337,224|0|0.0%

8.0.x versus the patch in #160

=== 8.0.x..2217731-160-profiling compared (5409bad376a91..5409be024b88e):

ct  : 1,077,489|1,113,859|36,370|3.4%
wt  : 4,000,885|4,132,650|131,765|3.3%
cpu : 3,980,636|4,111,596|130,960|3.3%
mu  : 34,484,056|34,520,736|36,680|0.1%
pmu : 35,337,224|35,419,872|82,648|0.2%

8.0.x versus #160 with the _css variables

=== 8.0.x..2217731-160-cssvar-profiling compared (5409bad376a91..5409c2f3ac41f):

ct  : 1,077,489|1,111,596|34,107|3.2%
wt  : 4,000,885|4,122,616|121,731|3.0%
cpu : 3,980,636|4,102,081|121,445|3.1%
mu  : 34,484,056|34,474,712|-9,344|-0.0%
pmu : 35,337,224|35,361,968|24,744|0.1%

I then compared the branches against each other just to double-check.

_css vars

=== 2217731-160-cssvar-profiling..2217731-160-cssvar-profiling compared (5409c85d44761..5409ca699d546):

ct  : 1,111,596|1,111,596|0|0.0%
wt  : 4,118,573|4,114,749|-3,824|-0.1%
cpu : 4,098,179|4,094,080|-4,099|-0.1%
mu  : 34,474,712|34,474,712|0|0.0%
pmu : 35,361,968|35,361,968|0|0.0%

_css vars versus the patch in #160 with no changes

=== 2217731-160-cssvar-profiling..2217731-160-profiling compared (5409c85d44761..5409ccb88f0ba):

ct  : 1,111,596|1,113,859|2,263|0.2%
wt  : 4,118,573|4,120,783|2,210|0.1%
cpu : 4,098,179|4,100,486|2,307|0.1%
mu  : 34,474,712|34,520,736|46,024|0.1%
pmu : 35,361,968|35,419,872|57,904|0.2%

Thanks @davidhernandez!

So yeah, 3% is not really minor... and it's 3% of [more than in D7] too...

I'm not too familiar with that area and cannot really evaluate the approaches that would let us keep strtr() here.

Worst case, if it's really not doable or has too ugly consequences, we can always consider that it's below the entity render cache and will only be an overhead on cache misses, but of course it's best to keep the performance of cache misses in mind too...

The 3% seems to mostly come from moving things to the template, and I assume all the added addClass() calls. What we're really looking at is the difference between using strtr() in the preprocess function with variables passed to the template versus using the clean_class filter in the Twig template. (What Cottser mentioned in #161) That is seen in the last group I posted ("_css vars versus the patch in #160 with no changes",) which has been showing a 0.1-0.3% difference. (I probably included more data than I needed to and made it confusing.)

The other possible mitigating factor, if we get all the "Classy" theme changes in, is this will move out of the core template and into the "Classy" theme. The performance hit should only be seen there. Core should actually become slightly faster, shouldn't it? Since all the preprocess and template css code will be gone.

@yched and @davidhernandez FYI the twig filter |replace() maps directly to strtr, so that should be much faster(guessing...).
http://twig.sensiolabs.org/doc/filters/replace.html

@davidhernandez any chance you could get a ubench key from from @Fabianx or @Cottser and post your xhprof results to Fabian's server? That would help see bulk and we can inspect it a bit further?
We have some documentation on that here @see https://drupal.org/contributor-tasks/profiling

Thinking about this a little more today. If we keep the strtr() ($variables['entity_type_css'] = strtr($element['#entity_type'], '_', '-');) we have to do it in preprocess. Option 1 is to move that code to the Classy theme when the template moves. In general, I'm opposed to adding functions to Classy. But, this will also leave us with the scenario where the template in Classy is dependent on code that lives there. If someone were to copy the template from Classy, which we likely encourage, but doesn't use Classy as their base, that template will break. They'll have to copy the preprocess code with it to maintain functionality.

Option 2 is to keep the preprocess code in the field module (core.) The template moved to Classy will receive those variables, and anyone that uses that same template will also receive those variables. However, these variables will not be used by the core template, and the (not insignificantly expensive) processing will still happen. That doesn't sound like a good idea either.

If we keep the clean_class method, the processing stays with the class assignment, the work is removed from core, nothing can break, and if any of those classes get removed, the work gets removed with it. If, ultimately, the difference is 0.1-0.4% difference based on my (I won't claim to be perfect) profiling, it seems reasonable to use the Twig filter.

That approach seems to make a lot of sense given the effects in those different situations.

Just tested the patch at our local sprint. Everything checked out fine. Screen grabs are attached to show the results.

Sorry for not being able to follow more closely (afk, following with my phone...). Not clear to me what's the 3% perf impact that wat mentioned earlier. If that's an effect of the patch itself (rather than the clean_css / strtr thing), isn't that a bit worrying ?

addClass will have at least slightly better performance after #2336355: Refactor Attribute rendering so that class="" is not printed, which is good because in these templates it can be called up to 4 times.

Yeah, what Cottser said. The big issue here is all the calls to addClass, not the difference between using strtr and clean_class. We are looking for ways to speed up addClass and I suppose to attribute printing, as well..

I had a conversation with Cottser about this on IRC yesterday, and he brought up using the Twig replace filter. People were using that earlier in this thread, before we ever had clean_class to use. I forgot all about that. If I have time this week I'll try it out and see if I can find any performance improvement over clean_class.

#2229435: Clean up the way attributes are printed in field.html.twig This will likely get in first, so we'll have to reroll if it does.

#2229435: Clean up the way attributes are printed in field.html.twig is in.

Rerolled

Some variables were missing