Problem/Motivation

The new without filter in twig works great, though we'd also like it to work with attributes. Currently it does remove the attribute, but since the referenced object is not cloned it's deleted instead of copied.

In addition Attribute objects still have a printed state. This means:

You can only print attributes once and they are done.

{{ attributes }} will print attributes.
{{ attributes }} will print nothing! :(

You *must* print attribute values in order or they won't print.

<tag class="extra {{ attributes.class }}"{{ attributes }}> will print <tag class="extra red green blue" id="header"> Magic!
<tag{{ attributes }} class="extra {{ attributes.class }}"> will print <tag class="red green blue" id="header" class="extra "> WTF!

Proposed resolution

For objects with ArrayInterface: clone the object before printing it.
Remove printed state so attributes can be printed as many times as needed.
Use without twig filter to explicitly separate attributes from the Attribute object and allow for before and after ordering of attribute objects.

After, to pull out attributes twice:

{{ attributes }} will print attributes.
{{ attributes }} will print again! :) 

After, to pull out attributes, use the without filter:

<tag class="extra {{ attributes.class }}"{{ attributes|without('class') }}> will print <tag class="extra red green blue" id="header"> Magicless!
<tag{{ attributes|without('class') }} class="extra {{ attributes.class }}"> will print <tag id="header" class="extra red green blue"> FTW!

Remaining tasks

  • Write tests for without attributes.
  • Write patch for without attributes.
  • Remove printed state from Attribute objects
  • Replace all the magic separated attributes with explicit without filtering to avoid double print.

User interface changes

n/a

API changes

Attributes no longer magical, less WTFs with explicit printed exclusions.

Files: 
CommentFileSizeAuthor
#24 interdiff.txt1.66 KBjoelpittet
#24 2227869-without-attributes-24.patch13.8 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,692 pass(es). View
#11 interdiff.txt2.42 KBjoelpittet
#11 2227869-without-attributes-11.patch12.84 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,651 pass(es). View
#5 interdiff.txt5.52 KBjoelpittet
#5 2227869-without-attributes-5.patch10.59 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,009 pass(es). View
#3 2227869-without-attributes-3.patch7.03 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,925 pass(es), 7 fail(s), and 0 exception(s). View
#3 interdiff.txt2.54 KBjoelpittet
#1 2227869-1-without-attributes-tests.patch6.41 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,796 pass(es), 8 fail(s), and 0 exception(s). View

Comments

joelpittet’s picture

Status: Active » Needs review
Issue tags: +Twig, +DX (Developer Experience), +sprint
FileSize
6.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,796 pass(es), 8 fail(s), and 0 exception(s). View

Here's the tests I'm looking to have pass. But I must be missing something because not even the first attributes are printing. I'd expect that at the least. Unfortunately attributes have a printed state which prevents them from being printed multiple times. This in my mind is a DX issue.

Status: Needs review » Needs work

The last submitted patch, 1: 2227869-1-without-attributes-tests.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
2.54 KB
7.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,925 pass(es), 7 fail(s), and 0 exception(s). View

This will still fail but it should only fail 7 times at most. That is because twig_without doesn't clone the incoming arrayobject AND because attributes can't be printed twice.

Status: Needs review » Needs work

The last submitted patch, 3: 2227869-without-attributes-3.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
10.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,009 pass(es). View
5.52 KB

This will make those 7 tests pass, though will likely break others because now printing attributes isn't magic and you can print them in multiple times and in any order you'd like.

joelpittet’s picture

Issue summary: View changes
jenlampton’s picture

Issue summary: View changes

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.

joelpittet’s picture

Issue summary: View changes
joelpittet’s picture

Issue summary: View changes
webchick’s picture

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.

joelpittet’s picture

Assigned: joelpittet » Unassigned
Issue summary: View changes
FileSize
12.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,651 pass(es). View
2.42 KB

Here are the fixes for the duplicate attribute value printings.

joelpittet’s picture

Re #10 @webchick forgot to mention |without filter is already in core, we are using it here to apply to attributes too. And we found it elsewhere here http://buildwithcraft.com/docs/templating/filters#without
Original issue is #2114563: Remove TwigReference with the help of 'without' filter replacing 'show'/'hide' functions.

mortendk’s picture

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

mdrummond’s picture

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.

joelpittet’s picture

#10 @webchick show()/hide() and render() forced us to hack the way twig works to pass things around by reference and broke lots of things on the way like it's dump() with it's nodevisitor priority and auto-escaping. Also personally they were really hard to know what they were doing even though they were documented and there are examples in core.

When themers need those features they hunted them down but day to day I'd bet they rarely use show/hide(show especially). And render() is called automatically by printing with twig. So now these features still exist, it's more flexible and in context of what you intend to do. This issue is just to extend it's flexibility to apply to Array like Objects and treat Attributes the same as renderable arrays are treated for consistency.

Have a look at in the change record for how the without filter makes the exclusion of elements being printed in context to what you are dealing with and not forcing you to scroll up and down the page looking for a hide() call that may not be right above your parent element you are applying it to.
https://drupal.org/node/2212845

Saying all that, I do agree it's more verbose and don't have a magic solution to make that go away and did expect that reaction to come from some people though most themers seem to prefer it that I've talked to so far. It seems way easier than show/hide to understand. And I hope it's only needed often, I agree it shouldn't be used in core templates(and if I can help it I will remove them), only for examples of how to do this in Bartik and your custom theme. For all other core templates we can just have pretty and simple everywhere.

This patch allows for multiple printing of attributes too, so if you are in a loop or debugging you can print the attributes out twice. And if you don't want to print them out twice... in this case just don't, that simple.

joelpittet’s picture

Also wanted to point out people are already having issues with not being able to print out attributes twice:
https://drupal.org/comment/7455088#comment-7455088

joelpittet’s picture

I've got a crazy idea (rehashed from last year when I tried this). If there is no printed state on attributes either, there doesn't need to be 4 PHP classes that represent the values of the attributes, you don't have to clone it's values because they would be class = array, everything else just bool or string instead of AttributeArray and AttributeBoolean. This would make for merging classes possible with stock twig, this would make this issue go away https://api.drupal.org/comment/49398#comment-49398 and likely make things faster.

So another reason I really hope this issue gets support.

mdrummond’s picture

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

mdrummond’s picture

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.

joelpittet’s picture

alexpott’s picture

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.

webchick’s picture

Status: Needs review » Reviewed & tested by the community

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

Cottser’s picture

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

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

joelpittet’s picture

Status: Needs work » Needs review
FileSize
13.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,692 pass(es). View
1.66 KB

Thanks for catching those @Cottser, here's the quick fixes for #23.

And thanks for the RTBC! (please re-set if green)

Cottser’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

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

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

Cottser’s picture

Title: Twig 'without' filter to work with Attributes. » Remove magic printing from Attribute class to make Twig 'without' filter work for attributes

Re-titling.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed f122f87 and pushed to 8.x. Thanks!

  • Commit f122f87 on 8.x by alexpott:
    Issue #2227869 by joelpittet: Remove magic printing from Attribute class...
Cottser’s picture

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

Cottser’s picture

Issue summary: View changes

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

Status: Fixed » Closed (fixed)

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