Move node classes out of preprocess and into templates

In an attempt to make this prettier this won't work but is a bit of pseudo code for how I'd propose this looks in the template. Need to add a few methods to AttributeArray object to accomplish this.

Inline like you have it.

<article class="{{ attributes.class.add('node', 'node--type-' ~ node_type, node_promoted ? 'node--promoted', node_sticky ? 'node--sticky', node_unpublished ? 'node--unpublished', preview ? 'node--preview', 'node--view-mode-' ~ view_mode) }}"{{ attributes|without('class') }}>

Easier to read but same difference.

{% set classes = [
  'node',
  'node--type-' ~ node_type,
  node_promoted ? 'node--promoted',
  node_sticky ? 'node--sticky', 
  node_unpublished ? 'node--unpublished',
  preview ? 'node--preview',
  'node--view-mode-' ~ view_mode
]
%}
<article class="{{ attributes.class.add(classes) }}"{{ attributes|without('class') }}>

If you think this is worth while open up a follow up and ping me with the issue and I'll jump on that.

I'm unclear on what's going on with the syntax here. I'm guessing that the question mark is an evaluator to see if a property exists, then setting a value if true, while the ~ always add a value? But I'm not clear, and I'm not sure others will be clear on it either. Is there a way to write this in a way where it's more obvious what's going on?

http://twig.sensiolabs.org/doc/templates.html#other-operators

for ~ concatenation and ? for ternary on a boolean expression.

Oh I should have added, the only think I added that doesn't exist in that code was the add method

Coming from #2217731: Move field classes out of preprocess and into templates.

If we're generalizing this as a pattern across our templates (which I'm fine with), would be good to standardize on the way we're going to name those "variables specifically massaged with drupal_html_class() or strtr() so that they can be placed in a 'class' attribute".

See discussion in #2217731-66: Move field classes out of preprocess and into templates and below.

In short : $variables['view_mode'] or $variables['css_view_mode'] ?

Copy/pasting my arguments from that other issue :

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

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

Doesn't it make harder for modules/theme frameworks updates? If a module template changed the class name, should I pay for coders again for the changes? Imagine that handling 100 sites, it's crazy to update 100 custom themes.

Generating css class in the preprocess makes it impossible to later change them.

I'm not understand this point. Why can't ?

Can't we do something like:

{{ classes|add('new-class') }}

{{ classes|remove('node-promoted') }}

{{ attributes|without('class') }}
If everywhere are code in this way, shouldn't take the CLASS away from attributes by default?

Or said... why not just:
{{ attributes }}

+++ b/core/modules/node/node.module
@@ -677,22 +677,21 @@ function template_preprocess_node(&$variables) {
+  $variables['node_type'] = $node->bundle();
...
   if ($node->isPromoted()) {
...
   if ($node->isSticky()) {
...
   if (!$node->isPublished()) {
...
   if (isset($variables['preview'])) {

FYI you don't need to make special variables in preprocess for these things.
If node is passed as a variable you can just do the following.

{{ node.promoted ? 'node--promoted' }}
{{ node.sticky ? 'node--sticky' }}
{{ node.published ? 'node--published' }}
{{ preview ? 'node--preview' }}

getPromoted and isPromoted method naming conventions will get mapped automatically by twig. You can see @Cottser and my talk at Austin and nice camp if you want more details.

@yched you're right too, there is no need for those 'css_' prefixed names, we can use the variables that are provided off the node to get the data we want. And we may want to consider moving drupal_html_class() into the AttributeArray class but that class is generic and rdf uses it for a couple of things so we will have to be a bit careful there(ie check for the name 'class' before running things through that filter)

@droplet we could do something like that, but the solution I proposed will keep the api consistent as we've made all attributes work off the one object. Technically you could just instantiate an AttributeArray() with those new methods I proposed above and pass it to twig and it will do just as you mentioned above if you'd like. Though in core we are trying to aim for consistency by manipulating the attributes using one object and if you just print {{ attributes }} you can avoid printing empty attribute names if there are no values. eg class="{{ attributes.class }}" = class="" when there are no classes provided.

You are totally right though, if we are always excluding class, there may be no reason to include it in attributes and instead make it a special flower that it is... though I'm not sure how many of those we are doing... maybe you can do a check on core and see?

This seems to make templates more complex without a gain that I can see.

@nevets the gain that I believe they are looking for is giving control over the attributes to the template developers and not hiding them in the preprocess layer. Yes it is more complex but with a bit of syntax sugar and helpers that I was proposing above, the management within the template won't look as complex and maybe even easier to understand than the preprocess. That's my goal anyways. If it looks as ugly as that first patch, I'm -1 too.

FYI my POC works just adding some tests.

Add Classes
-----------
Classes: {{ attributes.class.add(classes) }}

Other attributes: {{ attributes|without('class') }}

Remove Classes
-----------
Classes: {{ attributes.class.remove(classes) }}

Add Comma Separated Classes
-----------
Classes: {{ attributes.class.add('one', 'dos', 'trois') }}

Chain Classes
-----------

{% set preview = true %}
Classes:

{{ attributes.class
  .remove('one', 'dos')
  .add(['cat', 'dog'])
  .add('fish', preview ? 'node--preview')
}}

produces

Add Classes
-----------
Classes: red green blue node node--type-article node--promoted node--sticky node--unpublished node--view-mode-teaser

Other attributes: id="quotes" checked

Remove Classes
-----------
Classes: red green blue


Add Comma Separated Classes
-----------
Classes: red green blue one dos trois

Chain Classes
-----------
Classes: red green blue trois cat dog fish node--preview

It removes the ease of common classes across themes and in my mind puts logic in a template that should not be there.

@nevets I share that concern, though I was walked through the plan in Austin by @JohnAlbin and @mortendk and they discussed having a "core base theme" that would provide those common classes and structure in it's overridden templates. And remove all the conventions and most classes from core, so stark is even starker but bartik and seven would extend from this "core base theme" (called "Classy" but name is WIP) with all the best practice(for the moment) classes and structure.

I believe @JohnAlbin is writing up something, on an issue some place which @mortendk can fill you in on that when it shows up, but that's the gist of the idea.

This is in an attempt to future proof by removing provided classes and conventions that may change in 2 years to something wildly different, and less things to remove and fix for contrib base theme maintainers who will likely want to use their own naming conventions and/or classes and structure.

So yes I do agree that convention does aid in consistency between drupal themers and it seems they've tried to address that at the same time. The logic in templates though, that's debatable. Adding a class to a div is hardly business logic it's presentational, and we are still pumping in the values provided by contrib as long as the themer doesn't rip that out(which some may).

Does that help clarify things or make things worse?

I still do not like this proposal, it means modules can no longer counter on any classes the add to actually appear in the output since the theme is may not display them. This really seems like a large step backwards.

As for

This is in an attempt to future proof by removing provided classes and conventions that may change in 2 years to something wildly different,

this means instead of updating classes in one spot, hundreds of themes would have to update their classes.

this means instead of updating classes in one spot, hundreds of themes would have to update their classes.

No it actually means the opposite, themes who want the current classes can opt-in by using the classy theme as the base. Themes that don't want Drupal's default classes no longer have to have template files longer than your arm trying to regex out all the mark up in preprocesses.

We did talk about preserving the ability the add classes via preprocess, to cover cases in contrib, like display suite or block class.

I think we came to a strong agreement in Austin that classes (aka design logic) in templates is a good direction. What's the way forward in this patch? Decide a consistent implementation?

This issue feels postponed until we have that meta

Old parent has left the issue queue, so #2289511: [meta] Results of Drupalcon Austin's Consensus Banana is adopting this and taking it into its loving home.

Un-postponed!

tldr;
+1000 to this issue.

Personally I would much rather see:

{% attributes.class.add([
  'node',
  'node--type-' ~ node.bundle|class,
  node.promoted ? 'node--promoted',
  node.sticky ? 'node--sticky',
  not node.published ? 'node--unpublished',
  preview ? 'node--preview',
  'node--view-mode-' ~ view_mode|class
]).remove([
  'some-module-class'
]) %}
<article {{attributes}}>

Also, I know that there has been talk in IRC about the "ugliness" of this approach. I would like to remind everyone though that this has been discussed (in great length) amongst most of the front-end community as the most sensible course of action #2289511: [meta] Results of Drupalcon Austin's Consensus Banana:

1. We need an easy way to add and remove classes inside the template. This is very, very similar to how jQuery works (which most themers are familiar with).
2. One syntax: Twig. A (new to Drupal) themer does not need to know PHP in order to understand the preprocess layer to manipulate classes. This also means we do not have to debug the attributes.class variable first to see what it contains.
3. Classes are not variables. They are attributes manipulated by variables (tested expressions) that affect the presentation of a template via CSS.
4. Thus, this helps further our ultimate goal of the separate of "backend/data logic" vs. "frontend/display logic".

Classes belong in a template. This does not prevent contrib from adding classes via preprocess when appropriate (display suite, panels, fences, etc.), however core itself should not. I also suspect that the reason this would look "ugly" to some is due to the shear number of classes that have been added over time. Once all of these classes are in the templates and we further consolidate them into a said "base-theme" (#2289511: [meta] Results of Drupalcon Austin's Consensus Banana), it will be much easier to see how over-engineered (and likely completely unnecessary) these classes have evolved.

I think agree at this point that classes should be added in the template, what I really need is a consistent way of doing things so we can create all the child issues for #2289511: [meta] Results of Drupalcon Austin's Consensus Banana with the same instructions.

@mortendk what do you think about Mark Carver's suggestion above? Is that the better approach? It does look tidier.

Please make sure you are clear when implementing the move classes from preprocess to templates that you indicate how to deal with template suggestions. ie. Suggest using a twig block or something or there will be a bunch of duplicate template logic creating a maintenance nightmare for themers.

Right now, the preprocess deals with the classes being passed to all the template suggestions, by moving the classes to the template that *arguably display logic* now becomes a template maintenance issue. twig blocks may be the only solution to alleviate this logic duplication but only if you follow that as a best practice and thus I recommend discussing solutions to that effect and making those practices clear for people making the transition to this pattern.

Maybe also putting a bit too much emphasis on preprocess as the *obvious culprit*, also be cognizant of other areas that classes may and do come from.

1. hook_theme default
2. renderable array pass through
3. admin UI additions or overrides
4. pre_render
5. _alter() hooks
6. preprocess
7. last stop template

#wetblanket

Working up a patch using https://www.drupal.org/node/2315471.

Here's a new patch that uses addClasses. This was very easy to setup. Really nice to have this option. Very easy to add the clearfix class in the Bartik node template for example. Nice vindication for this approach.

Okay, the example above had a class filter. I was wondering if that was a thing or not. It appears not so much. Need to figure out what that was supposed to do, and what alternative we can use. I would bet that's what's causing the nearly 2200 fails.

@mdrummond morten mentioned in #22 this puppy: #2283301: Add Twig filters for drupal_html_class() and drupal_clean_id_identifier()

Just removing the class filters to make sure it will pass tests. Do we need to wait on the filter to proceed? I'm assuming not having it is just status quo, and not taking a step back. (...though it looks like that issue has a reviewed patch)

Also, I added an empty array in the node preprocess instead of an empty string. Isn't that more appropriate?

1. +++ b/core/modules/node/node.module
   @@ -654,24 +654,8 @@ function template_preprocess_node(&$variables) {
   -  $variables['attributes']['class'][] = drupal_html_class('node--type-' . $node->bundle());
   ...
   -    $variables['attributes']['class'][] = drupal_html_class('node--view-mode-' . $variables['view_mode']);
   

   These drupal_html_class() filters need to be done in preproces until that clean_class filter is added.

2. +++ b/core/modules/node/node.module
   @@ -654,24 +654,8 @@ function template_preprocess_node(&$variables) {
   +  // Add an empty class for modules
   +  $variables['attributes']['class'] = array();
   

   This shouldn't be necessary.

3. +++ b/core/modules/node/templates/node.html.twig
   @@ -77,7 +77,20 @@
   +  set add_classes = [
   +    'node',
   +    'node--type-' ~ node.bundle,
   
   +++ b/core/themes/bartik/templates/node.html.twig
   @@ -71,7 +71,20 @@
   +  set add_classes = [
   +    'node',
   +    'node--type-' ~ node.bundle,
   

   This duplication is my main maintenance fear and banana blocker. Please look at trying to remove the duplication.

1. We need to add the filter first then, because otherwise we're not actually removing this class from preprocess.

3. I strongly disagree that this needs to be abstracted out. The entire point of banana is to put classes into the template so they're easy to add and remove for themers. If you put those classes in some master template rather than the actual template, they are not right there, and it gets rid of making that easy. This is exactly the same thing as having similar markup in the system template and a Bartik template. We don't abstract out that HTML markup that is the same between the two, and I don't think we should do so for classes either.

Re #41:

2. This is only added because other preprocess function are expecting the 'class' array to already exist when they add to it. Not to say this is the best solution.

3. Any duplication to Bartik should only be temporary. Bartik would made a child of the 'classy' theme and inherit these class additions from it. I'm also not seeing the maintenance issue. Once a version of core is released, do templates even change? Wouldn't that essentially be an api break? I checked the git log for 7, and it looks like node.tpl.php has been changed in years.

re #43 and #42
1. Ok fine, though I'd rather not halt momentum on this on yet another issue... maybe just add that patch in here.
2. Oh yeah, l'll likely have to make a follow-up to move things from using the array access to addClass() because the array access may still expect the AttributeArray be instantiated before adding more where as addClass() doesn't have that limitation.
3. When one change needs to be made in two or more places it's a maintenance issue. Try to look at the bigger picture on this, there is not just one theme in core and not solving this now could take a lot of work fixing later for a 'temporary' change. I've put in a temporary change once, the real fix didn't come a year later and it had to be reverted. So I'm a bit sensitive to 'temporary' in core. If there is a proven solution in the issue queue that proves this won't be a problem (this could be that issue for the others) I wouldn't have as much of a problem with it.

Yes, it's possible that these class definitions could get out of sync between system and Bartik (or Seven or 'Classy'), but the exact same thing is true in HTML in all of those templates. If you want to make the setting of HTML in multiple templates DRY, then fine, but we're not doing that, and I don't see how class names are any different than an HTML element in a template.

Let's try to talk this out in IRC sometime soon. I know we talked about this in the Twig hangout last week.

#2283301: Add Twig filters for drupal_html_class() and drupal_clean_id_identifier()

The clean_class and clean_id filters are in.

Not sure if I'll be able to get on the Twig call or not, so just in case, wanted to share another thought on this.

Let's say that we did keep everything in preprocess. Those preprocess additions would need to go into the classy.theme file for the Classy theme. We couldn't just keep them in system or core modules.

So the consequence is that the first step would be to add the classy theme to core, have it inherit all of its templates from system/core modules, and set up Bartik and Seven to be based off of Classy. Then once it's in, you could start moving preprocess functions for each template to Classy rather than system/core, along with the template itself.

That sort of plan is possible, but it relies upon getting people to add a new theme to core that doesn't actually do anything useful to start with, on the premise that we can get everything done before launch.

The reverse plan is that we move classes out of preprocess and into templates. Once that is done, we can add the Classy theme, probably along with the templates? Then you'd have to go through system/core and decide on what basic markup you want for each of those templates once they no longer have classes in them.

It's probably messy either way. I think classes in templates is a better plan, even though it's less DRY. But I think to make that decision, you need to get Morten and John Albin involved, as they were two of the key people involved with the Banana plan, and keeping classes in preprocess is a huge change to that plan.

Okay, here's a revised patch using clean_class where appropriate.

In our Twig discussion today, we agreed that moving class decisions to the template was the best option.

What we need to discuss with a core committer is one of two paths forward.

Plan A: The Classy Three-Step

• Step one: Go through all the Twig templates where class attributes are added in from preprocess and move those decisions into the template itself, like above.
• Step two: Create the Classy theme. Change Bartik and Seven to use Classy as their base theme. Put out a change notice advising other contrib themes to do the same thing.
• Step three: Go through all the Twig templates with classes in them and move them into the Classy theme. The core/system version of that template would have all classes removed from it at this time.

Plan B: The Classy Two-Step

• Step one: Create the Classy theme. Change Bartik and Seven to use Classy as their base theme. Put out a change notice advising other contrib themes to do the same thing.
• Step two: Go through all the Twig templates where class attributes are added in from preprocess and move those decisions into the template itself, like above. At the same time, move those templates into the Classy theme. The core/system version of that template would have all classes removed from it at this time.

Part of our concern is that when we strip out all the classes from the core templates in step three of Plan A or step two in Plan B, there could be an argument over what markup should be left behind in the template: what HTML elements should get matched up with the data variables.

Ultimately I think we just need to make a decision that there can be no markup changes besides class removals in A3 or B2. Once the classes have been stripped out in core, there could be a separate dreammarkup issue opened up to play around with the core markup. But we don't want that to hold up the plan.

It should also be noted that with either plan, we would likely be moving the relevant CSS in steps A3 or B2.

What it comes down to is that plan A will have far, far more issues opened up, because we will have to one set of issues for moving classes from preprocess to template, and another set for moving templates to classy. With plan B, that all happens in one step, so there will be fewer issues, but bigger patches.

I also think it would be good to get the Classy theme added as early as possible, even if there is no visible benefit at first, because that gives more time for contrib themes to set Classy as their base theme. Technically, with Plan A we could still get step two in right away, even if we make no use of it.

What it really comes down is if committers would rather deal with twice as many small patches or half as many larger patches. We can go either way.

Attaching the actual patch and interdiff.

Also, if for some reason people don’t want us to name the new theme Classy, I vote that we name it Ace. Because nearly every contrib theme, even base themes, will then have a line in their info.yml file that says:

Base theme: Ace

And that way, our new theme could be quite literally the Ace of Base.

@mdrummond I'm +1 on Ace because I love corny jokes and I named my family dog Ace.

Re #43 Thought about 2) again, and this is an array, (not Attribute object until it gets through _theme(). So you can just do $var['attrs']['class'][] = 'blah'; and it won't complain, also doesn't cobber any variables coming through a render array like

$renderable = array(
  '#theme' => 'node', 
  '#attributes' => array('class' => array('default-node-class')))

.
So it's even harmful (kinda) to do that initialization in preprocess.

Also setting the variable to classes instead of add_classes because it's familiar to views and other D7y things so there a bit of a precedence and don't want it to confuse with the method name.

Cool?

And that way, our new theme could be quite literally the Ace of Base.

I got out of bed, and opened up my laptop just to say Boooo!

A3: Just to clarify, not all the classes would be removed. Just classes deemed non-critical.

There's probably also a step 4 somewhere, which is to go through Stark, Bartik and Seven and evaluate what might need cleanup?

Ultimately I think we just need to make a decision that there can be no markup changes besides class removals in A3 or B2.

Agreed, no markup changes.

What it really comes down is if committers would rather deal with twice as many small patches or half as many larger patches. We can go either way.

Regardless of what we do, I don't think we'll have committers deal with lots of patches. If we use the sandbox or not, we should still have each phase done with a mega patch. One for the preprocess change, one for copying/modifying the templates. This will also help with rerolling, if needed.

I'm curious to see if it will pass test with the removal of the empty class attribute. I think that was added because things were breaking.

I support the add_classes to classes change. Let's just make sure we are consistent as we go through all the templates.

@davidhernandez Those 2 tests are just attributes out of order fails. I rewrote them as xpath so they don't need to care about the order.

They check the existance or lack of some lang/dir attributes + article tag with a "node" class.

I tested the patch. Made various changes in custom preprocess functions, and everything came through as expected. Woo!

Since this is the test case of these preprocess changes we a proposing to make, I'd prefer another person give it a look and RTBC it.

Changing parent to Phase 1

Just a suggestion but why not go one better and use cssSelect for the revised tests? https://www.drupal.org/node/2276689

Otherwise:

1. +++ b/core/modules/node/templates/node.html.twig
   @@ -77,7 +77,20 @@
    #}
   -<article{{ attributes }}>
   +
   +{%
   
   +++ b/core/themes/bartik/templates/node.html.twig
   @@ -71,7 +71,20 @@
    #}
   -<article class="{{ attributes.class }} clearfix"{{ attributes|without('class') }}>
   +{%
   

   Spacing is inconsistent, the second version seems better.

2. +++ b/core/modules/node/node.module
   @@ -653,25 +653,6 @@ function template_preprocess_node(&$variables) {
   -  if ($variables['view_mode']) {
   -    $variables['attributes']['class'][] = drupal_html_class('node--view-mode-' . $variables['view_mode']);
   -  }
   
   +++ b/core/modules/node/templates/node.html.twig
   @@ -77,7 +77,20 @@
   +    'node--view-mode-' ~ view_mode|clean_class,
   
   +++ b/core/themes/bartik/templates/node.html.twig
   @@ -71,7 +71,20 @@
   +    'node--view-mode-' ~ view_mode|clean_class,
   

   This doesn't seem entirely consistent with what was happening in preprocess, what if view_mode is falsey?

3. +++ b/core/modules/node/templates/node.html.twig
   @@ -77,7 +77,20 @@
   +    preview ? ' node--preview'
   
   +++ b/core/themes/bartik/templates/node.html.twig
   @@ -71,7 +71,20 @@
   +    preview ? ' node--preview',
   

   Why is there an extra leading space on this one only?

And I'm guessing trailing/serial comma doesn't work for the list of classes like we'd normally do for PHP arrays. That could get annoying.

Working on an update to this after all the fun things we learned in #2217731: Move field classes out of preprocess and into templates. One strange thing @wheatpenny and I are noticing is the fact that node.sticky ? 'node--sticky' is alway true with the latest patch applied. Will post a new patch tomorrow morning.

Cleaned this up to be consistent with what was done in field. One thing I didn't tackle was updating the LocaleContentTest to use cssSelect, although I am on board with that change.

@davidhernandez @wheatpenny and I all looked into the node.sticky ? 'node--sticky' issue from my previous comment, but to no avail yet.

That is because twig checks for a sticky property first, and that always exists because it is a field.

Call the method (isSticky) directly or use sticky.value

Why do node.published and node.promoted behave differently? Those don't require using *.value.

Of the two options, isSticky versus .value, I'm not sure which is more kosher. .value seems odd to have to use, but I don't know. Alternatively, should we just set a variable in preprocess for sticky, promoted, published, etc?

Because published and promoted are no fields. published and promoted map directly to isPublished() and isPromoted(), the fields that hold the actual value are called promote and status. for sticky, it matches both the field and the isSticky() method, and twig uses the propery if it exists and not the method.

I like #61
This will help totally get rid of frontent's code from php

+++ b/core/modules/node/templates/node.html.twig
@@ -77,34 +77,45 @@
-  {{ title_suffix }}
+    {{ title_prefix }}

I think things got a bit further indented than normal here. Should be 2 spaces like we use everywhere.

@crowdcg we can use isSticky() method directly too. @Berdir why doesn't the public property map to the method? re #64 that sounds a bit confusing to why one set of methods match and others don't, is this a bug?

No, it's not a bug, that's how twig works. I've been thing about custom integration for content entities but I don't quite know what we'd actually want to do.

The properties do not actually exist, they go through __get() and so on on ContentEntityBase. They can't map to the methods because they hold the actual data, the methods use them, not the other way round.

Berdir, thanks for the explanation.

How about we just make these all variables then? I'm not liking too much this mix of variable and method use in the node template. Since it is likely to be one of the most used templates by themers, it would be nice to have it reasonably clean. Plus, node already has variables for things like is_front, readmore, view_mode, author_name, etc.

We could just have:

{%
  set classes = [
    'node',
    'node--type-' ~ bundle|clean_class,
    promoted ? 'node--promoted',
    sticky ? 'node--sticky',
    not published ? 'node--unpublished',
    'node--view-mode-' ~ view_mode|clean_class,
    preview ? 'node--preview',
  ]
%}

That is a nicer experience than node.isSticky

@Berdir I still feel like I'm missing something from the Entity API regarding sticky/isSticky(). Why aren't they working the same as promoted? Or is promoted busted as well?

From \Drupal\node\Entity\Node

  public function isSticky() {
    return (bool) $this->get('sticky')->value;
  }

VS

  /**
   * {@inheritdoc}
   */
  public function isPublished() {
    return (bool) $this->get('status')->value;
  }

I guess you can do node.isSticky or node.isSticky() or node.sticky() and all should work. Just kinda seems strange that they would be picked up by the __get(). I guess it's tricky to use __get() for properties and fields?

Oh properties are fields in D8, sorry I thought they were object properties for some reason... disregard.

Ok, working on an updated patch, where promoted, sticky, and published are variables and all the spacing is correct.

I'm figuring this would mean the comment block needs to be updated to reflect this change as well, but my question is should we leave in or take out the following under the node variable:

 *   - promoted: Whether the node is promoted to the front page.
 *   - sticky: Whether the node is 'sticky'. Sticky nodes are ordered above
 *     other non-sticky nodes in teaser listings
 *   - published: Whether the node is published.

@crowdcg I'm partial to keeping as much as possible out of preprocess as possible (and leaving that for contrib to play with). So'd recommend using the full method names.

{{ node.isSticky() }}
{{ node.isPublished() }}
{{ node.isPromoted() }}

and I just realized why 'sticky' and 'promoted' and 'published' are different. 'sticky' is the field name, where promoted='promote' and published = 'status'. So that's why they are different.

I know we don't use camelCasing much but we already are for addClass() method. And maybe we call those methods directly as we do in PHP, it' would be a bit less work on Twig to find out what we mean anyway...

Thoughts?

I've been thinking about this, and I may be willing to retract my previous comment. If people who take the "Stark" method of theming aren't going to care about sticky, promoted, etc, then adding the extra variables would be a waste for them, so might as well just keep it in the template. I am still concerned about making sure we take care in keeping node clean. Whatever the final result, it will end up being one of D8's most visible templates.

Joel, aren't the parentheses after the method names not needed?

@davidhernandez correct no need for parentheses. Here is an updated patch.

@davidhernandez They aren't needed, though there is a small caveat to that. The parenthesis that say it's a method call and not a property will effect how Twig finds the value. If for some silly reason someone makes a field called issticky, ispromoted, ispublished for whatever usecase they may have, you will run into the same problem as above where it uses magic __isset and __get methods and look for that field first. If you put the parenthesis in, it will find it if that edge case happens.

So slim chance... but possible nonetheless.

@joelpittet thanks for being an eagle eye and catching the extra spaces. Leaving the () out for now until others comment or we discuss on the Twig call.

+++ b/core/modules/node/templates/node.html.twig
@@ -76,7 +76,18 @@
+    'node--view-mode-' ~ view_mode|clean_class,

just a really general thought. Did we considered to call the clean_class functionality from inside the attributes? This would safe us from having to care about it.

@dawehner I was giving that some thought but we don't really have a method to override that and clean_class may do something we don't intend it to do all the time. Like BEM __ would get converted to --. Though I think that example was fixed other patterns from other CSS naming schemes may not be accounted for, so unless it's not as destructive and/or not another performance hit we should likely avoid that. I wouldn't rule it out as an option though, just off the cuff my mind goes to let's do it manually.

Now if we had transparent class extension in PHP, maybe some kind of Attribute Decorator, we could do all sorts of things like that I guess:) *wishful thinking*

1. Need to change the following from:
<div{{ content_attributes.addClass('node__content clearfix') }}>
to:
<div{{ content_attributes.addClass('node__content', 'clearfix') }}>

2. Use the parentheses for example isPublished() etc.

3. Also need to change test to use cssSelect.

1 & 2 definitely fixed, 3 passed locally, but probably needs a good review to see if I did this right.

+++ b/core/modules/locale/src/Tests/LocaleContentTest.php
@@ -177,26 +177,26 @@ public function testContentTypeDirLang() {
-    $pattern = '|class="[^"]*node[^"]*"[^<>]*lang="en"|';
...
+    $element = $this->cssSelect('article.node[lang="en"]');
...
-    $pattern = '|class="[^"]*node[^"]*"[^<>]*dir="ltr"|';
...
+    $element = $this->cssSelect('article.node[dir="ltr"]');
...
-    $pattern = '|class="[^"]*node[^"]*"[^<>]*lang="ar" dir="rtl"|';
...
+    $element = $this->cssSelect('article.node[lang="ar"][dir="rtl"]');
...
-    $pattern = '|class="[^"]*node[^"]*"[^<>]*lang="es"|';
...
+    $element = $this->cssSelect('article.node[lang="es"]');
...
-    $pattern = '|class="[^"]*node[^"]*"[^<>]*lang="es" dir="ltr"|';
...
+    $element = $this->cssSelect('article.node[lang="es"][dir="ltr"]');

The previous pattern looked for the node class + an attribute of lang with a value of "en."

This adds in an article element selector that didn't exist previously.

Is the article selector necessary? If so why?

@mdrummond not sure why @joelpittet added the article selector in the #55 patch, but you're right that it doesn't seem to really need to be there.

re: #82 I added it because that is the first tag in the node template, considering you are rendering an entire page and you could have a french block and a spanish node or something else along those lines I wanted to target the node template more specifically to avoid any false positives.

Okay, well that was my only concern. Everything else looks good.

Looking back, though it appears that item 2 in this comment hasn't been addressed: https://www.drupal.org/node/2254153#comment-9065333

What if we changed that from:

'node--view-mode-' ~ view_mode|clean_class,

to:

view_mode ? 'node--view-mode-' ~ view_mode|clean_class,

I think if we get that in, this could be RTBC at least from me.

Fixed issue noted by @mdrummond in #85, originally reported by @Cottser in #58!

Green means go.

We've gone through this extensively, and it looks like all the bases are covered.

Please note that the change notice that explains this is available at https://www.drupal.org/node/2325067.

Just a couple questions, otherwise looks good to me!

1. +++ b/core/modules/node/node.module
   @@ -651,25 +651,6 @@ function template_preprocess_node(&$variables) {
   -  if (isset($node->preview)) {
   -    $variables['attributes']['class'][] = 'node--preview';
   -  }
   
   +++ b/core/modules/node/templates/node.html.twig
   @@ -76,7 +76,18 @@
   +    preview ? 'node--preview',
   

   This doesn't seem consistent. Are we sure there is a preview variable? The way previews work changed very recently: #1510544: Allow to preview content in an actual live environment

2. +++ b/core/modules/node/templates/node.html.twig
   @@ -11,10 +11,10 @@
   - *   - promoted: Whether the node is promoted to the front page.
   - *   - sticky: Whether the node is 'sticky'. Sticky nodes are ordered above
   + *   - isPromoted(): Whether the node is promoted to the front page.
   + *   - isSticky(): Whether the node is 'sticky'. Sticky nodes are ordered above
     *     other non-sticky nodes in teaser listings
   - *   - published: Whether the node is published.
   + *   - isPublished(): Whether the node is published.
   

   These aren't "variables" in the strict sense. I'm not completely against adding them here like this but I think it's worth mentioning.

@Cottser so #1 seems like it is completely removed now. When I run D8 at HEAD and hit preview I never see that class applied.

#2 not sure where else we would put them, logically then need to be under node to work. Maybe make the comments something like:
"isPublished(): Method defining whether the node is published."

Thoughts?

RE: #1, it looks like that is gone with the changes to preview because it now shows the preview in a full page instead of embedding the node inside the edit form. So now we get 'page-node-preview' applied to the body class.

RE: #2, I think the issue is that the top comment line says "Available variables". We don't comment every method usage in the template, just variables that exist, because those aren't easily discernible. Technically, we could just not add a comment for them.

I removed the node preview class since it looks like it has no use any more. I also checked the CSS and it does not appear anywhere.

I updated the comment blocks removing the lines with the method explanations. There is actually a block below that explains the logic behind the classes. It think that is sufficient.

*   The following are controlled through the node publishing options.
 *   - node--promoted: Appears on nodes promoted to the front page.
 *   - node--sticky: Appears on nodes ordered above other non-sticky nodes in
 *     teaser listings.
 *   - node--unpublished: Appears on unpublished nodes visible only to site
 *     admins.

I also copied the node comment block into Bartik's template. There were slight language differences. I assume one was updated and the other not.

I approve of those changes :) thanks!

+++ b/core/themes/bartik/templates/node.html.twig
@@ -11,15 +11,11 @@
- *   {{ content|without('field_example') }} to exclude the printing of a
- *   given child element.
+ *   {{ content|without('field_example') %} to temporarily suppress the printing
+ *   of a given child element.

Oh no! This changes the example code to {{ foo %}, so that needs to be fixed here and in the base node.html.twig. Looks like this was missed before, probably when |without got committed in the first place.

Does the older line have the more correct text then too? to exclude the printing

"temporarily suppress" is probably slightly more accurate (because it doesn't actually alter the render array/Attribute object/whatever), but we have both wordings in core now.

Changed the percent.

I think this resolves the outstanding issues.

+++ b/core/modules/node/templates/node.html.twig
@@ -33,8 +29,8 @@
+ *   - node--type-[type]: The current node type. For example, if the node is a
+ *     "Article" it would result in "node--type-article". Note that the machine

It should read, in two places: if the node is an "Article". I realize this patch didn't introduce the grammatical error but it would be nice to fix it.

Feel free to throw up a patch with an interdiff. Easy quick fix.

Good work on this issue everyone. Please don't credit me for this drive-by, but somebody let me know about the other module issues that are opened and I will try to help.

I jut realized crowdcg removed the original sticky, etc, docs because he replaced them with the method descriptions. I then removed the method the descriptions. I assume we should put the original ones back in?

*   - promoted: Whether the node is promoted to the front page.
*   - sticky: Whether the node is 'sticky'. Sticky nodes are ordered above
*     other non-sticky nodes in teaser listings
*   - published: Whether the node is published.

Hmm, but didn't we find some/most of them no longer worked as advertised? Tough call.

I don't think we're passing in those original variables now. And we decided no docs for methods since other methods aren't documented. So I think we're fine.

The variables should still be there as part of node, so I think node.published.value, etc, would still be viable.

@all I think that because of the unreliable nature of sticky we should just remove them. Although @davidhernandez is correct about the node.published.value working, this isn't a pattern we are looking to promote. Now that we've switched to using the methods isSticky(), etc., we are saying this is the recommended way to use these in the Twig templates and therefore shouldn't confuse the matter.

I think the comments are more about listing what is available to the user (themer) not just describing what is being done in the template. We don't want them to have to dig around to know certain variables are available to them in a template. But I don't want to bikeshed over it if others feel they don't belong.

Manually tested both Bartik and Stark (frontpage view and node page), no real differences other than some nice fixes like:

Before

<div class="node__content ">

After

<div class="node__content">

Reroll. It wasn't clear what caused the merged conflict, it got stuck on line 97 of node/templates/node.htm.twig so have a quick check just in case.

Setting this back to RTBC. There was just one small change in a line of code outside of the lines being changed in this patch. So this should still be good. Checked and patch still applies.

Committed 8c38be0 and pushed to 8.0.x. Thanks!

Since this is in before Field (#2217731: Move field classes out of preprocess and into templates,) which we were expecting to be the first big one to go in, and Field had a draft change record (https://www.drupal.org/node/2325067,) I think we should attach that change record to Node and publish it. (Making text changes needed.)

If no one objects I'll do it today.

Sounds good, thanks @davidhernandez!

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