Remove magic printing from Attribute class to make Twig 'without' filter work for attributes

I'm not sure we have tests to see if classes are printed twice or not. Maybe it won't fail tests?

I love this idea. less magic is good. Explicitly stating when you don't want something makes sense.

Joel pinged me about this in IRC and I asked for an "after" example, since the "before" is obviously confusing and dumb.

Unfortunately, the "after" would turn node.html.twig from:

<article id="node-{{ node.id }}" class="{{ attributes.class}} clearfix"{{ attributes }} role="article">

to:

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

Which... ugh. :( I wouldn't mind this as much if it were in some wackadoo templates like aggregator-foo or similar, but it's happening in node, comment, page, taxonomy, etc. which are very 'core' templates, and basically step 0 of any themer's journey into how Drupal theming works. I'd really rather not make those more verbose and "PHP-like" than we need to. It works against the stated benefit of Twig, which is to remove all of that convoluted stuff in favour of {{simplicity}}.

I'm also concerned that it's a very deliberate reversion of a very deliberate feature we added in D7, which was print render($foo['bar']); ... print render($foo); // everything but bar This was to help facilitate the theming of dynamic elements such as forms, nodes, etc. which could have additional properties added to them at any time that weren't originally accounted for in the design. While this is still supported with "without," (so this wouldn't qualify as a regression), now you need to know about the existence of that and how to use it properly. It won't happen for you automagically just by simply having {[attributes}} (or {{content}} or whatever) there. This also breaks compatibility with other places we use the render API.

The following things would make me feel better here:

a) Research into how other Twig-utilizing CMSes handle this issue, if any. Like if "without" is used everywhere else, and is as common a sight as "print" is for PHP people, I'm probably freaking out over nothing. :D

b) Research into how often print render($foo['bar']); ... print render($foo); is actually being used "in the wild." This is going to be hard to quantify, because a lot of it I'm sure is/isn't happening in custom themes to which we have no insight. I do have a checkout of all contrib themes if someone has a bright idea on how to query for this info. Else, maybe we'll start with anecdotal data from a few people on how often they use this feature. It was conceived by developers IIRC so it's possible that the answer is "never," in which case great! But I simply don't know.

@webchick

ok without beeing to ranty ... (and he started ranting)

Themers that comes into drupal the first time are more wtf where did this come from & why cant i change the class / markup where i want to, and simply dont understand why its all hidden over in a preprocess/process/funtion/thingie/deep-in-a-dungeon-where-you-need-the-secret-handshake to be able to modify the actual output.
TBH themers dont care about alle the smart things you can do with those functions, they just wanna change the classes & markup to what they need, without having to understand all of drupals magic.

During on of the many many many talks we have had we came up with the principles to guide us when we got into it with Drupal8 https://drupal.org/node/2008464#principles

one of the princples was to make it visible what was going on, so fx instead of just having {{ attributes }} then splitting them up like <div class="{{ attributes.class}}"{{ attributes }}> then its sweet n clear whats what.

right now were working with pure magic - that will create a possbiel problem down the road if you wanna change something in drupal that we didn think about inside of the attributes
lets say that role="article" but i mighty themer want to make it a nav instead:
old:

  <article  class="node-{{ node.id }} {{ attributes.class}}"{{ attributes }}>

new:

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

that gives me some badass flexibility in what we output, and i dont have to figure out where the stuff came from & how to grap it with a regex ;)

In #10, webchick's before example was this:

<article id="node-{{ node.id }}" class="{{ attributes.class}} clearfix"{{ attributes }} role="article">

I think it's worth pointing out that in this example, if attributes has id and role defined, then the output could be:

<article id="node-42" class="meaning-of-life clearfix" id="node-42 role="article" role="article">

Using node.id won't stop attributes.id from printing. And since role is being explicitly added to the markup without stopping attributes from printing role, that gets spit out twice too.

I understand the desire for simple template files, but this example is actually a good reason to allow for without to work on attributes to fix this sort of problem.

This is a prime example of the problems caused by removing attribute children once they're printed. https://api.drupal.org/comment/49398#comment-49398

This boils down to a consistency thing for me.

Right now, you can use the without filter on variables in Twig templates... unless they are attributes, and then without doesn't work. That's not intuitive.

Right now, for most variables in the templates, if you print foo.bar and then print foo, you also get foo.bar the second time unless you explicitly remove it using a without filter. So that ship has already sailed.

What this does is to make attributes consistent with all of the other variables in Twig templates.

It's not that complicated. If you want to print out all info in an attributes variable, you just add attributes to your template, and away you go. If you want to explicitly use a sub-element inside attributes, no problem, but then you need to explicitly prevent that sub-element from being used if you want to print attributes, and you don't want duplication. Just like you need to do with every other variable in a template.

This makes things less magical, yes, but way more consistent.

Talked about this with @joelpittet irl - yes this is more verbose but we should be moving to the following in core themes...

  <article{{ attributes }}>

Rather than...

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

In short I'm +1 to this change since we have less magic and themers get more flexibility to print things multiple times.

Additionally in future this could allow us to change the attribute object's children to be an array of values rather then objects which will be more performant.

Ok, thanks for the extra feedback here, folks! I talked to Joel and Scott tonight and they said they'd also been running this past some first-time Twiggers in D8 training and seemed good to them too. Joel also showed me a patch with like a 12:500 diffstat. ;)

Marking this to RTBC. I'll take a look at this tomorrow if no one else gets to it first. Time to pass out now. :)

Looks like we missed a couple:

core/themes/bartik/templates/maintenance-page.html.twig
19:<body class="{{ attributes.class }}"{{ attributes }}>

core/themes/bartik/templates/comment.html.twig
65:<article class="{{ attributes.class }} clearfix"{{ attributes|without('class') }} role="article">

This one needs 'role' added.

Also this should definitely have a change record, and we can update the existing Attribute change record: https://drupal.org/node/1727592

Draft change record: https://drupal.org/node/2240005

I can update https://drupal.org/node/1727592 after commit.

Re-titling.

Committed f122f87 and pushed to 8.x. Thanks!

Thanks! I just updated https://drupal.org/node/1727592.

…and published the draft change record (oops) :)