Split out of #2214249: Fields classes - what do we actually need & want in Drupal8 twig:

Problem/Motivation

All classes are produced in a preprocess that makes it hard for a themer to manipulate these into whatever you need.
I propose we remove them all from the preprocess and move them into the template file, so its easy to find and manipulate.
(twig template at the end)

Proposed resolution

Remove class creation from the preprocess functions and add them in the Twig templates using the addClass() attribute method and the clean_class filter.

Preprocess Functions Modified

core/includes/theme.inc: template_preprocess_field
core/modules/comment/comment.module: comment_preprocess_field
core/themes/bartik/bartik.theme: bartik_preprocess_field

Twig Templates Modified

core/modules/system/templates/field.html.twig
core/modules/system/templates/field--comment.html.twig
core/modules/system/templates/field--node--title.html.twig
core/themes/bartik/templates/field--taxonomy-term-reference.html.twig

Test instructions

Create a new node.
Add a comment.
Make a note of the markup that a field produces.
Apply the patch.
Now check the markup of the field again it should match the markup from before the patch was applied.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it improves the Themer Experience.
Issue priority Normal because it should not affect many people.
Unfrozen changes Unfrozen because it changes markup.
Prioritized changes The main goal of this issue is usability of the new Twig theming layer, and since it adjusts markup it must be completed prior to RC.
Disruption Disruptive for core/contributed and custom themes because it is part of the Consensus Banana to move CSS classes from preprocess to twig templates. (#2322163)
Files: 
CommentFileSizeAuthor
#224 2217731-224.patch12.94 KBcrowdcg
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,782 pass(es). View
#218 2217731-218.patch12.92 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2217731-218.patch. Unable to apply patch. See the log in the details link for more information. View
#218 interdiff.txt4.67 KBlauriii
#207 move_field_classes_out-2217731-207.patch11.84 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,380 pass(es). View
#201 interdiff.txt642 byteslauriii
#201 move_field_classes_out-2217731-201.patch11.69 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,915 pass(es). View

Comments

mortendk’s picture

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.

LewisNyman’s picture

This patch only works if you apply it on top of the patch in #1987398: field.module - Convert theme_ functions to Twig. Once that's committed this will be ready for testing. Feel free to give it a code review though.

LewisNyman’s picture

FileSize
2.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,676 pass(es). View

Oops wrong patch

barraponto’s picture

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.

rteijeiro’s picture

Status: Active » Needs review

Let's test it! :)

Status: Needs review » Needs work

The last submitted patch, 3: drupal-field-classes-templates-2217731-3.patch, failed testing.

mortendk’s picture

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

mortendk’s picture

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 }}"

barraponto’s picture

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

mortendk’s picture

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

barraponto’s picture

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

mortendk’s picture

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

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

easier for the themer :)

barraponto’s picture

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

LewisNyman’s picture

I think it's easier to decide what is best if we focus on a certain user objectively. Does something like that exist for the Twig initiative?

I'd imagine it would be for the benefit of non-module developers who don't use php, which makes the decision here simple.

mortendk’s picture

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

barraponto’s picture

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

pakmanlh’s picture

Status: Needs work » Needs review
pakmanlh’s picture

FileSize
2.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-field-classes-templates-2217731-18.patch. Unable to apply patch. See the log in the details link for more information. View

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.

rteijeiro’s picture

Status: Needs review » Needs work

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'], '_', '-');

The last submitted patch, 18: drupal-field-classes-templates-2217731-18.patch, failed testing.

pakmanlh’s picture

Status: Needs work » Needs review
FileSize
2.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,732 pass(es). View

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

Status: Needs review » Needs work

The last submitted patch, 21: drupal-field-classes-templates-2217731-21.patch, failed testing.

pakmanlh’s picture

Status: Needs work » Needs review
LewisNyman’s picture

+++ b/core/modules/system/templates/field.html.twig
@@ -23,6 +23,10 @@
+ * - attributes.class.entity_type_css: The entity type in a CSS friendly string.
+ * - attributes.class.field_name_css: The field name in a CSS friendly string.
+ * - attributes.class.field_type_css: The field type in a CSS friendly string.
+ * - attributes.class.label_position: The position of the label in relation to the field value.

I think there's a simpler way to document keyed items in an array using indentation.

pakmanlh’s picture

FileSize
3.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,724 pass(es). View

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

mortendk’s picture

  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>
jjcarrion’s picture

Assigned: Unassigned » jjcarrion
jjcarrion’s picture

FileSize
2.45 KB
2.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2217731-drupal-field-classes-templates-28.patch. Unable to apply patch. See the log in the details link for more information. View

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.

Status: Needs review » Needs work

The last submitted patch, 28: 2217731-drupal-field-classes-templates-28.patch, failed testing.

jjcarrion’s picture

Status: Needs work » Needs review
FileSize
3.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,728 pass(es). View
3.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,730 pass(es). View

Sorry for my previous patch, here is again.

jjcarrion’s picture

Xano’s picture

Status: Needs review » Needs work
  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.

barraponto’s picture

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

mortendk’s picture

#facepalm wrong filename

mortendk’s picture

Status: Needs work » Needs review

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

mortendk’s picture

FileSize
3.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,806 pass(es). View

Status: Needs review » Needs work

The last submitted patch, 36: 2217731-drupal-field-classes-templates-34.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
jjcarrion’s picture

FileSize
3.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,807 pass(es). View
1.96 KB

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

jjcarrion’s picture

Assigned: jjcarrion » Unassigned
mdrummond’s picture

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.

mortendk’s picture

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

mortendk’s picture

FileSize
2.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,888 pass(es). View

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

LewisNyman’s picture

+++ b/core/modules/system/templates/field.html.twig
@@ -16,20 +16,32 @@
+{% 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 %}

Wasn't this issue supposed to make it really easy and simple to change classes? This is no longer easy to understand and change without fear of breaking something. We might as well keep this in a preprocess if this is what we have to do to put it in a twig file.

mortendk’s picture

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.

mortendk’s picture

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

LewisNyman’s picture

&lt;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 }}&gt;

I think we could remove the "_css" after each variable name. I don't think it's important to know how they are formatted. Themers should assume all variables we pass to tempaltes are CSS safe, right?

mdrummond’s picture

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

mortendk’s picture

FileSize
3.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,793 pass(es), 15 fail(s), and 18 exception(s). View

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'], '_', '-');
mortendk’s picture

FileSize
2.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,826 pass(es). View

new patch removed entity view stuff ;)

The last submitted patch, 49: 2217731-drupal-field-classes-templates-49.patch, failed testing.

aboros’s picture

Assigned: Unassigned » aboros
Status: Needs review » Needs work
+++ 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.

aboros’s picture

Assigned: aboros » Unassigned
Status: Needs work » Needs review
FileSize
728 bytes
2.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,752 pass(es), 3 fail(s), and 0 exception(s). View

This is just putting spaces where they were missing from.

emma.maria’s picture

Issue summary: View changes
Issue tags: +Novice, +frontend
joshua.boltz’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
65.04 KB

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 53: 2217731-drupal-field-classes-templates-53.patch, failed testing.

aczietlow’s picture

Assigned: Unassigned » aczietlow
aczietlow’s picture

I'm going to take an attempt at this.

aczietlow’s picture

Assigned: aczietlow » Unassigned
Status: Needs work » Needs review
FileSize
959 bytes
2.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,760 pass(es). View

Fixed trailing white space in twig file for simpletest.

LewisNyman’s picture

Issue tags: +Needs manual testing
LewisNyman’s picture

Status: Needs review » Needs work
Issue tags: -Needs manual testing

Good news, I tested it manually and the markup is identical. I would like to mark this RTBC but we still need to document these new variables in the template.

aczietlow’s picture

Assigned: Unassigned » aczietlow
aczietlow’s picture

FileSize
2.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,848 pass(es). View
768 bytes

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

dcam’s picture

Status: Needs work » Needs review

Go, Testbot!

sarahjean’s picture

Status: Needs review » Reviewed & tested by the community

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

yched’s picture

Status: Reviewed & tested by the community » Needs review

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

jstoller’s picture

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.
aczietlow’s picture

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?

yched’s picture

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

jstoller’s picture

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.

Ec1ipsis’s picture

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.

yched’s picture

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.

Ec1ipsis’s picture

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.

aczietlow’s picture

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

yched’s picture

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 ?

joelpittet’s picture

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.

jstoller’s picture

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.

mortendk’s picture

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.

LewisNyman’s picture

One of the points that really hit home with me with the "classy" theme implementation was that if we ever want to completely change the mark up in Drupal core again we won't have to change all the module template files. We just create a new theme. That's mind blowing.

What's going on with this meta? I feel kind of anxious that, after we had over 20 frontend devs holding a big thumbs up over an openly discussed and developed plan, there's more discussion going on behind closed doors.

aczietlow’s picture

FileSize
2.77 KB
2.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2217731-drupal-field-classes-templates-80.patch. Unable to apply patch. See the log in the details link for more information. View

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.

mortendk’s picture

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

LewisNyman’s picture

andypost’s picture

jwilson3’s picture

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

yched’s picture

@jwilson3: see #72.

jwilson3’s picture

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.

mdrummond’s picture

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

Cottser’s picture

jwilson3’s picture

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

Cottser’s picture

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

aczietlow’s picture

Status: Needs review » Active

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

Status: Active » Needs review
aczietlow’s picture

FileSize
3.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,780 pass(es), 3 fail(s), and 4 exception(s). View
Ec1ipsis’s picture

Functionally tested patch #94 and passed.

joelpittet’s picture

Status: Needs review » Needs work

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.

The last submitted patch, 80: 2217731-drupal-field-classes-templates-80.patch, failed testing.

The last submitted patch, 94: 2217731-drupal-field-classes-templates-94.patch, failed testing.

crowdcg’s picture

Issue summary: View changes
FileSize
5.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,581 pass(es), 830 fail(s), and 14 exception(s). View
4.3 KB

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.

crowdcg’s picture

Status: Needs work » Needs review

Test please!!!

joelpittet’s picture

Status: Needs review » Needs work

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.

The last submitted patch, 99: 2217731-99.patch, failed testing.

crowdcg’s picture

Status: Needs work » Needs review
FileSize
5.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,101 pass(es), 3 fail(s), and 0 exception(s). View
567 bytes

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

Status: Needs review » Needs work

The last submitted patch, 103: 2217731-103.patch, failed testing.

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
5.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,115 pass(es). View
2.79 KB

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.

joelpittet’s picture

Assigned: aczietlow » Unassigned
Status: Needs review » Reviewed & tested by the community

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!

mdrummond’s picture

This is very nice. Great work.

davidhernandez’s picture

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.

dead_arm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

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.

davidhernandez’s picture

What would the documentation be on?

tim.plunkett’s picture

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.

mdrummond’s picture

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.

tim.plunkett’s picture

Status: Needs work » Needs review

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.

davidhernandez’s picture

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.

mdrummond’s picture

Sounds good if you want to write that up David.

davidhernandez’s picture

Status: Needs review » Needs work

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

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

crowdcg’s picture

Assigned: Unassigned » crowdcg

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

Cottser’s picture

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.

davidhernandez’s picture

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

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

davidhernandez’s picture

Also, added links to documentation and previous issues.

Cottser’s picture

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

jwilson3’s picture

Removed period.

joelpittet’s picture

Issue tags: -Needs change record

@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

crowdcg’s picture

Assigned: crowdcg » Unassigned
Status: Needs work » Needs review
FileSize
8.69 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,108 pass(es), 3 fail(s), and 0 exception(s). View
6.58 KB

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>

Status: Needs review » Needs work

The last submitted patch, 124: 2217731-124.patch, failed testing.

jwilson3’s picture

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

joelpittet’s picture

@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

crowdcg’s picture

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

davidhernandez’s picture

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

crowdcg’s picture

FileSize
8.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,824 pass(es), 3,103 fail(s), and 207 exception(s). View
4.06 KB

Changes incorporated from 126-129.

crowdcg’s picture

Status: Needs work » Needs review

Always forget to change status...

Status: Needs review » Needs work

The last submitted patch, 130: 2217731-130.patch, failed testing.

joelpittet’s picture

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

davidhernandez’s picture

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"
    }
crowdcg’s picture

Status: Needs work » Needs review
FileSize
8.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,240 pass(es). View
4.7 KB
568 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,114 pass(es). View

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.

The last submitted patch, 135: 2217731-135.patch, failed testing.

crowdcg’s picture

Status: Needs review » Needs work

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

Cottser’s picture

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>
Cottser’s picture

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

Cottser’s picture

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

davidhernandez’s picture

davidhernandez’s picture

Cottser queued 135: 2217731-135.patch for re-testing.

Cottser’s picture

Status: Needs work » Needs review
davidhernandez’s picture

Status: Needs review » Needs work

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.

davidhernandez’s picture

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

crowdcg’s picture

Status: Needs work » Needs review
FileSize
8.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,239 pass(es). View
909 bytes

Updated per @davidhernandez's comments.

davidhernandez’s picture

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

mdrummond’s picture

This is looking good to me.

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

Cottser’s picture

Category: Feature request » Task
mortendk’s picture

Issue tags: +frontendunited

gonna get some bodys on this from copenhagen :)

rteijeiro’s picture

Issue tags: -frontendunited +FUDK

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

lauriii’s picture

Assigned: Unassigned » lauriii
rteijeiro’s picture

Assigned: lauriii » Unassigned
Status: Needs review » Needs work

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 }}"

davidhernandez’s picture

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.

jwilson3’s picture

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

davidhernandez’s picture

Issue summary: View changes
davidhernandez’s picture

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.

davidhernandez’s picture

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()

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
8.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,257 pass(es). View
726 bytes
Cottser’s picture

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

davidhernandez’s picture

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.

mdrummond’s picture

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?

Cottser’s picture

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.

Cottser’s picture

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.

davidhernandez’s picture

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.

andypost’s picture

Related issues: +#2229355: Field template suggestions are colliding

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

yched’s picture

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.

Cottser’s picture

Issue tags: +Needs profiling
davidhernandez’s picture

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.

davidhernandez’s picture

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.

yched’s picture

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

yched’s picture

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

davidhernandez’s picture

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%
yched’s picture

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

davidhernandez’s picture

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.

joelpittet’s picture

@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

davidhernandez’s picture

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.

mdrummond’s picture

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

karolus’s picture

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

yched’s picture

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 ?

Cottser’s picture

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.

davidhernandez’s picture

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.

davidhernandez’s picture

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

davidhernandez’s picture

lauriii’s picture

Status: Needs work » Needs review
FileSize
11.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,015 pass(es), 6 fail(s), and 0 exception(s). View

Rerolled

Status: Needs review » Needs work

The last submitted patch, 187: move_field_classes_out-2217731-187.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
744 bytes
11.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,028 pass(es), 1 fail(s), and 0 exception(s). View

Some variables were missing

Status: Needs review » Needs work

The last submitted patch, 189: move_field_classes_out-2217731-189.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
11.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,033 pass(es). View
767 bytes

Testbot didnt like my variable inside html for some reason..

LewisNyman’s picture

Status: Needs review » Needs work
+++ b/core/modules/node/templates/field--node--created.html.twig
@@ -11,12 +11,24 @@
 <span{{ attributes }}>

+++ b/core/modules/node/templates/field--node--uid.html.twig
@@ -11,12 +11,24 @@
 <span{{ attributes }}>

In some template files we aren't added the classes variable to attributes.

lauriii’s picture

Status: Needs work » Needs review
FileSize
906 bytes
11.72 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,508 pass(es). View
LewisNyman’s picture

Perfect. I diffed the output of the html and it looks good. Looks like this still needs some performance review

joelpittet’s picture

First set of performance tests on #193

50 nodes on the homepage using teasers of articles with all the fields showing in their viewmode (comments block too).

=== 8.0.x..8.0.x compared (544333b05173f..544337f542f97):

ct  : 898,030|898,030|0|0.0%
wt  : 3,764,162|3,767,531|3,369|0.1%
cpu : 3,738,272|3,746,080|7,808|0.2%
mu  : 57,286,648|57,182,552|-104,096|-0.2%
pmu : 57,378,768|57,279,088|-99,680|-0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=544333b05173f&...

=== 8.0.x..2217731-193 compared (544333b05173f..54433ad6a7755):

ct  : 898,030|899,902|1,872|0.2%
wt  : 3,764,162|3,851,937|87,775|2.3%
cpu : 3,738,272|3,820,533|82,261|2.2%
mu  : 57,286,648|57,226,768|-59,880|-0.1%
pmu : 57,378,768|57,322,296|-56,472|-0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=544333b05173f&...

~87ms slower which isn't all that great.

davidhernandez’s picture

I ran some tests using 100 nodes, with 5 fields each plus titles. This setup is similar to what I originally did in #174.

Baseline

=== 8.0.x..8.0.x compared (5443da27ae6d4..5443dcd48e976):

ct  : 1,125,068|1,125,068|0|0.0%
wt  : 4,551,401|4,557,970|6,569|0.1%
cpu : 4,543,031|4,549,435|6,404|0.1%
mu  : 63,453,376|63,453,376|0|0.0%
pmu : 63,755,496|63,755,496|0|0.0%

Compared to #193

=== 8.0.x..2217731-193 compared (5443da27ae6d4..5443df3213304):

ct  : 1,125,068|1,134,752|9,684|0.9%
wt  : 4,551,401|4,612,688|61,287|1.3%
cpu : 4,543,031|4,603,815|60,784|1.3%
mu  : 63,453,376|63,481,672|28,296|0.0%
pmu : 63,755,496|63,789,520|34,024|0.1%

I'm not completely sure why these are faster, other than the performance changes that were made to addClass() in a previous issue. A month has gone by, with many commits to core.

I was more interested in testing the difference between clean_class and using replace('_', '-'), since that might be the only alternative within the template.

Baseline for #193

=== 2217731-193..2217731-193 compared (5443ed9310e2a..5443fbe0ae3f9):

ct  : 1,134,752|1,134,749|-3|-0.0%
wt  : 4,611,640|4,610,796|-844|-0.0%
cpu : 4,602,744|4,602,713|-31|-0.0%
mu  : 63,481,672|63,479,344|-2,328|-0.0%
pmu : 63,789,520|63,787,360|-2,160|-0.0%

Using replace filter

=== 2217731-193..2217731-193-withreplace compared (5443ed9310e2a..5443fc10d98da):

ct  : 1,134,752|1,132,296|-2,456|-0.2%
wt  : 4,611,640|4,627,036|15,396|0.3%
cpu : 4,602,744|4,617,841|15,097|0.3%
mu  : 63,481,672|63,505,328|23,656|0.0%
pmu : 63,789,520|63,802,184|12,664|0.0%

I ran these repeatedly, and replace was always slightly slower, though never hugely different.

I don't know how many performance gains can be found in how we handle these classes in the template, if any significant detriment is from addClass.

On the brightside, these will likely all be removed from the core module and put into Classy. Also, being in the template, if anyone has a performance concern, they can override the template and remove them. That is not something you can do if it were to stay in the preprocess function.

joelpittet’s picture

@davidhernandez can you post your xhprof results, ask @Fabianx or maybe @Cottser for a key for uploading the bench marks. That way we can dig in and scout the bottle necks.

From the results I posted this is what I got for the bits:
drupal_html_class (|clean_class filter)
Excl. Wall Time (microsec)
before: 1,257
after: 3,805
diff: 2,548
percent diff: 202.7%

addClass()
Excl. Wall Time (microsec)
before: 7,048
after: 9,688
diff: 2,640
percent diff: 37.5%

Both of those are fairly negligible at a difference of 2.5ms.

I'm wondering where the rest of the 50-80ms is coming from?

davidhernandez’s picture

FileSize
317.64 KB

I tried getting the remote posting of the results to work before, but couldn't get it to work. I'll have to try again.

I lost my last runs after rebooting, but ran it a few more times. This is what I'm seeing when sorted by the function call diff. (sorry for the crappy formatting.)

function|calls diff|call diff %|incl. wall diff|incl. wall diff %|excl. wall diff|excl. wall diff %

drupal_html_class|3,200|33.0%|11,686|25.4%|7,513|16.4%
getClass|3,200|33.0%|4,192|9.1%|3,303|7.2%
strtr|-2,388|-24.7%|-2,319|-5.0%|-2,319|-5.0%
function_exists|-800|-8.3%|-834|-1.8%|-834|-1.8%
call_user_func_array@1|800|8.3%|22,497|49.0%|3,309|7.2%
getAttribute|800|8.3%|38,755|84.4%|12,800|27.9%
func_get_args|800|8.3%|1,210|2.6%|1,210|2.6%
bartik_preprocess_field|-800|-8.3%|-801|-1.7%|-801|-1.7%
addClass|800|8.3%|19,799|43.1%|8,308|18.1%
is_object|800|8.3%|-630|-1.4%|-630|-1.4%
get_class|800|8.3%|481|1.0%|481|1.0%
strtolower|800|8.3%|889|1.9%|889|1.9%
array_merge|800|8.3%|1,723|3.8%|1,723|3.8%
hasExtension|800|8.3%|796|1.7%|796|1.7%

These seem to be the biggest differences in call count and time.

The only other thing I see with majorly different numbers each time is load::service_container. Changing by 10ms an each run. Is that even something to be controlled? Is it introducing too much randomness into the tests?

I sorted the results by Excl. Wall Time and made a screenshot (required some editing to remove the crud.)

lauriii’s picture

Issue tags: +banana
lauriii’s picture

FileSize
11.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,696 pass(es). View

Rerolled

lauriii’s picture

FileSize
11.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,915 pass(es). View
642 bytes

Added missing docblock

mdrummond’s picture

Tried to look back at the discussion to see the forest for the trees. Given the xhprof testing, would it be reasonable to say that we'd be okay with going with the clean_class filter in the template rather than cleaning things up in preprocess? clean_class seems like the most stable option since it doesn't rely upon the existence of a preprocess function, which would be useless if kept in core, and which might be somehow missed if Classy doesn't get properly set as a base theme. Do I have that right?

davidhernandez’s picture

@mdrummond, can you clarify what you mean by:

which might be somehow missed if Classy doesn't get properly set as a base theme

Do you mean if the classes in preprocess were moved to a preprocess function in Classy? If so, yes, I would be concerned about someone copying the field template from Classy, but not using Classy as a base. This would break the template. We would also repeat the same problem of things happening in preprocess, just localizing the problem to Classy.

rteijeiro’s picture

Issue tags: +Needs bikeshedding
rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -FUDK, -

It looks good for me. Let me know if you need screenshots.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/node/templates/field--node--created.html.twig
@@ -11,13 +11,25 @@
+ * - entity_type: The entity type to which the field belongs.
+ * - field_name: The name of the field.
+ * - field_type: The type of the field.
  *

+++ b/core/modules/node/templates/field--node--uid.html.twig
@@ -11,13 +11,25 @@
+ * - entity_type: The entity type to which the field belongs.
+ * - field_name: The name of the field.
+ * - field_type: The type of the field.
  *

Missing label_display

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
11.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,380 pass(es). View
3.43 KB

I updated the label_display comment in all the templates.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Only reviewed the change and it looks good. Gonna RTBC because of #205

alexpott’s picture

The performance concerns feel a bit glossed over. Reading all the profiling in the comments it looks like this patch introduces a 3% regression - is that the case?

davidhernandez’s picture

The last test Joel did looks to be about 2.1% and the last one I did was about 1.2%; obviously different tests. I'd have to do a much larger test (a lot more loops) to get a more accurate test. The regression would carry over to Classy, but removed from the core field module once we, hopefully, get the classes removed.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update
+++ b/core/modules/comment/templates/field--comment.html.twig
@@ -17,22 +17,43 @@
+    'field-' ~ entity_type|clean_class ~ '--' ~ field_name|clean_class,
+    'field-name-' ~ field_name|clean_class,

+++ b/core/modules/node/templates/field--node--created.html.twig
@@ -11,13 +11,26 @@
+    'field-' ~ entity_type|clean_class ~ '--' ~ field_name|clean_class,
+    'field-name-' ~ field_name|clean_class,

+++ b/core/modules/node/templates/field--node--title.html.twig
@@ -11,13 +11,26 @@
+    'field-' ~ entity_type|clean_class ~ '--' ~ field_name|clean_class,
+    'field-name-' ~ field_name|clean_class,

+++ b/core/modules/node/templates/field--node--uid.html.twig
@@ -11,13 +11,26 @@
+    'field-' ~ entity_type|clean_class ~ '--' ~ field_name|clean_class,
+    'field-name-' ~ field_name|clean_class,

+++ b/core/modules/system/templates/field.html.twig
@@ -24,15 +24,35 @@
+    'field-' ~ entity_type|clean_class ~ '--' ~ field_name|clean_class,
+    'field-name-' ~ field_name|clean_class,

+++ b/core/themes/bartik/templates/field--taxonomy-term-reference.html.twig
@@ -12,14 +12,34 @@
+    'field-' ~ entity_type|clean_class ~ '--' ~ field_name|clean_class,
+    'field-name-' ~ field_name|clean_class,

Calling this twice on the same string feels wasteful. How about an additional set so we don't have too.

Also this issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary. Obviously as part of the banana consensus it can go in but it's nice to have the justification in a standard way on every issue.

crowdcg’s picture

Issue summary: View changes
FileSize
11.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,391 pass(es). View
4.14 KB

Updated patch with additional set, so clean_class doesn't run twice on field_name. Also, took a stab at the issue summary per #211. @davidhernandez feel free to revise. Once we have it for this we should add it to any others that are outstanding.

crowdcg’s picture

Status: Needs work » Needs review

Forgot to send the patch for testing...

crowdcg’s picture

FileSize
12.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,909 pass(es), 1 fail(s), and 0 exception(s). View
4.38 KB

Per a conversation on IRC with @davidhernandez I've changed field_name = field_name|clean_class to field_name_class = field_name|clean_class as to not cause issues with using field_name elsewhere in templates.

lauriii queued 214: 2217731-214.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 214: 2217731-214.patch, failed testing.

davidhernandez’s picture

I think it is this commit that changed things. #2358529: Right-aligned images in CKEditor appear to the right of other fields Some extra field templates were introduced and the test that is failing is checking for exact markup. The difference being an added clearfix on body text.

lauriii’s picture

Status: Needs work » Needs review
FileSize
4.67 KB
12.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2217731-218.patch. Unable to apply patch. See the log in the details link for more information. View

Test failed because of the class sort has changed.

lauriii’s picture

Issue tags: -Needs reroll
davidhernandez’s picture

Profiling the patch in #218

My setup was 100 nodes with a title, body field and four integer fields. That should be six fields per node, giving 600 total on the page.

### FINAL REPORT

=== 8.0.x..8.0.x compared (54811b6aec43d..54811c7da26f8):

ct  : 1,262,917|1,262,917|0|0.0%
wt  : 3,557,584|3,556,141|-1,443|-0.0%
mu  : 42,490,344|42,490,344|0|0.0%
pmu : 45,436,280|45,436,280|0|0.0%

=== 8.0.x..2217731-218 compared (54811b6aec43d..54811e8abf892):

ct  : 1,262,917|1,271,008|8,091|0.6%
wt  : 3,557,584|3,627,956|70,372|2.0%
mu  : 42,490,344|42,561,904|71,560|0.2%
pmu : 45,436,280|45,511,816|75,536|0.2%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

### XHPROF-LIB REPORT

+-------------------+------------+------------+------------+------------+------------+
| namespace         |        min |        max |       mean |     median |       95th |
+-------------------+------------+------------+------------+------------+------------+
| Calls             |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2217731-218       |  1,271,008 |  1,274,933 |  1,271,047 |  1,271,008 |  1,271,008 |
| 8_0_x             |  1,262,917 |  1,266,842 |  1,262,956 |  1,262,917 |  1,262,917 |
|                   |            |            |            |            |            |
| Wall time         |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2217731-218       |  3,627,956 |  4,145,188 |  3,665,010 |  3,654,802 |  3,679,872 |
| 8_0_x             |  3,556,141 |  4,071,021 |  3,606,145 |  3,596,047 |  3,627,338 |
|                   |            |            |            |            |            |
| Memory usage      |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2217731-218       | 42,561,904 | 65,691,360 | 43,001,708 | 42,561,904 | 42,561,904 |
| 8_0_x             | 42,490,344 | 65,629,712 | 42,930,458 | 42,490,344 | 42,490,344 |
|                   |            |            |            |            |            |
| Peak memory usage |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2217731-218       | 45,511,816 | 68,378,128 | 45,948,813 | 45,511,816 | 45,511,816 |
| 8_0_x             | 45,436,280 | 68,302,960 | 45,873,526 | 45,436,280 | 45,436,280 |
|                   |            |            |            |            |            |
+-------------------+------------+------------+------------+------------+------------+

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=54811b6aec43d&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=54811b6aec43d&...

davidhernandez’s picture

Status: Needs review » Reviewed & tested by the community

#218 has the changes requested in #211

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 218: 2217731-218.patch, failed testing.

davidhernandez’s picture

Issue tags: +Needs reroll
crowdcg’s picture

Status: Needs work » Needs review
FileSize
12.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,782 pass(es). View

Rolling, rolling, rolling, field patch is rerolling...

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs profiling, -Needs reroll

A quick manual test shows all is well

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed a081397 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.

  • alexpott committed a081397 on 8.0.x
    Issue #2217731 by crowdcg, lauriii, davidhernandez, aczietlow, jjcarrion...
davidhernandez’s picture

*falls to knees and weeps*

It's a Christmas miracle!

mortendk’s picture

it only took 9 months

Jeff Burnz’s picture

Indeed, its a christmas miracle, I am celebrating, well done guys, hats off and merry christmas.

Status: Fixed » Closed (fixed)

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