Problem/Motivation

Sometimes you have an array of attributes and it would be nice if you could addClass, removeClass, setAttribute, etc. rather than trying to do array manipulation.

Proposed resolution

Add a function to our Twig extension to allow creating an Attribute object from an array of attributes.
allow_instantiating-2616756-16.patch

Remaining tasks

All of them.

User interface changes

n/a

API changes

API addition: New Twig function added (name TBD).

Data model changes

n/a

Comments

Cottser created an issue. See original summary.

LittleCoding’s picture

The Attribute class already includes the functions: addClass, removeClass, hasClass, setAttribute, removeAttribute.

Using this: /themes/my-theme/templates/html.html.twig

{% set attr = 'data-test' %}
{% set val = 'foobar' %}
{%
  set classes = [
    'top',
  ]
%}
<!DOCTYPE html>
<html{{ html_attributes }}>
  <head>
...
  </head>
  <body{{ attributes.addClass(classes).setAttribute(attr, val) }}>
    <ul class="skip-to-content">
...

the body element is rendered something like:

<body class="top toolbar-tray-open toolbar-fixed toolbar-horizontal" data-test="foobar">

So is this a case for documentation?

Cottser’s picture

attributes in that case is already an Attribute object, attributes, content_attributes, and title_attributes are special cased inside \Drupal\Core\Theme\ThemeManager::render() - see related issue. Elsewhere you should be able to find where html_attributes is turned into an Attribute object in preprocess.

You might want to test instead by creating an attribute array entirely in a template or you can create an array of attributes in a preprocess function. Either way, take that variable and then try to manipulate that in Twig (or even just print it), it won't work because it's not an Attribute object.

Hope that makes sense :)

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

nicholasThompson’s picture

I agree that this would be useful.

Use case:
You have a field with many items, say images, and you're making a bootstrap carousel from them. You may want to define a default attributes object for each item that you can easily addClass for the "active" one to.

joelpittet’s picture

@nicholasThompson any chance you'd like to take a stab at creating this as a proof of concept?

nicholasThompson’s picture

I'd love to if I had the faintest idea of where to start... ;-)

joelpittet’s picture

Start by adding a function or filter in the TwigExtension class.
/core/lib/Drupal/Core/Template/TwigExtension.php

My guess would be a function that would create a new instance of an Attribute object.

That would likely be all that is need I guess.

hctom’s picture

I'd totally appreciate this in order to be able to include templates with a completely new Attribute() object set in the include-statement. This is for example needed when building templates within the atomic design process, to be able to add modifier classes while including a subcomponent template.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

heddn’s picture

Issue tags: +Novice

+1 on the idea here. I'm going to mark this as novice, because creating a small helper function in TwigExtension per comment #8 is super simple to do.

ytl’s picture

I'll try work on this at Drupalcon Dublin

ytl’s picture

Issue tags: +Dublin2016
FileSize
1.03 KB
heddn’s picture

Status: Active » Needs work
  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -600,4 +601,17 @@ public function safeJoin(\Twig_Environment $env, $value, $glue = '') {
    +   * @param array $attributes
    +   *   Array (key, value) for attributes.
    

    This should match the parameter arguments provided by Attribute.

  2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -600,4 +601,17 @@ public function safeJoin(\Twig_Environment $env, $value, $glue = '') {
    +  public function addAttributes($attributes) {
    

    This doesn't add multiple attributes. Maybe calling this addAttribute would be better.

    No type hinting or default values so if someone doesn't pass anything, this is going to fail miserably. Take a look at the default constructor arguments on Attribute and copy/paste them into here.

  3. Also, after uploading a patch, the status is normally changed to 'needs review'.
Charlotte17’s picture

Issue summary: View changes
FileSize
1.12 KB
685 bytes

Called an array in a function __construct with a class Attribute

lauriii’s picture

Issue tags: +Needs tests

Thanks for working on this issue! This would be very useful improvement.

  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -134,6 +134,7 @@ public function getFunctions() {
    +      new \Twig_SimpleFunction('addAttributes', [$this, 'addAttributes']),
    

    I think we should use underscore writing compound since all the other Twig functions are following that pattern

  2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -594,4 +595,20 @@ public function safeJoin(\Twig_Environment $env, $value, $glue = '') {
    +   * Create an Attribute object from an array.
    

    s/Create/Creates

  3. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -594,4 +595,20 @@ public function safeJoin(\Twig_Environment $env, $value, $glue = '') {
    +   * @return Attribute
    

    This should include the full namespace

  4. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -594,4 +595,20 @@ public function safeJoin(\Twig_Environment $env, $value, $glue = '') {
    +    if (!empty($attributes)) {
    

    I think this should return new Attributes object even if $attributes is empty

  5. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -594,4 +595,20 @@ public function safeJoin(\Twig_Environment $env, $value, $glue = '') {
    +        new Attribute($attribute);
    

    The attributes object should be returned in the end

Charlotte17’s picture

Version: 8.3.x-dev » 8.2.x-dev
Status: Needs work » Needs review
FileSize
1.41 KB
1.78 KB

This patch include all items comments into #15 and #17

Charlotte17’s picture

The last submitted patch, 18: allow_instantiating-2616756-18.patch, failed testing.

lauriii’s picture

I iterated that a little further

lauriii’s picture

FileSize
2.94 KB

Here's interdiff

joelpittet’s picture

  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -54,6 +54,13 @@ class TwigExtension extends \Twig_Extension {
    +   * An empty attributes object that can be cloned.
    

    nit: An empty Attribute object that can be cloned.

  2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -600,4 +608,25 @@ public function safeJoin(\Twig_Environment $env, $value, $glue = '') {
    +    if (empty($attributes)) {
    

    This outside if is not needed. It should always check attributes !isset($this->attributes) and if it's not there create the empty new object.

    After clone if it's ! empty, maybe something like this. (like the constructor but not in the constructor)

    foreach ($attributes as $name => $value) {
    $cloned_attributes[$name] = $value;
    }

  3. If that's getting to messy we can look at that in a follow-up issue for a slight performance gain.

heddn’s picture

#23 makes sense. I went a little further and renamed the property from $this->attributes => $this->attribute. Interdiff attached.

heddn’s picture

Version: 8.2.x-dev » 8.3.x-dev

Since this is a new feature, also moving to 8.3. Although (hint, hint) this would be awful nice in 8.2 as well.

joelpittet’s picture

Version: 8.3.x-dev » 8.2.x-dev
Status: Needs review » Reviewed & tested by the community

I think this looks very well put together. I'll add this to the twig_extensions if it takes a while to get in.

joelpittet’s picture

Version: 8.2.x-dev » 8.3.x-dev

Didn't mean to change the version.

heddn’s picture

@joelpittet, that seems to just be a wrapper around twig upstream. Maybe opening an issue in twig_tweaks or some other twig++ contrib module is a better fit. And given this is slotted for 8.3, it will be a few months before folks start seeing it generally available.

joelpittet’s picture

Hmm good point, don't want to scope creep my own project, lol. Yeah, I can do something like that too, hopefully in a non-disruptive to when the filter gets into core.

Cottser’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php
@@ -299,6 +299,27 @@ public function testRenderVarWithGeneratedLink() {
+      ['class' => ['kittens']],
+      [],
+      []

Nice kittens. Can we test adding multiple attributes, please?

Charlotte17’s picture

In this patch add multiple attributes #30

Status: Needs review » Needs work

The last submitted patch, 31: allow_instantiating-2616756-31.patch, failed testing.

Charlotte17’s picture

Here I applied correctly patch #24 and add attributes asked in comment #30

heddn’s picture

  1. +++ b/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php
    @@ -309,14 +309,15 @@ public function testCreateAttribute() {
    +  ¶
    ...
    +    ¶
    

    Whitespace.

  2. +++ b/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php
    @@ -309,14 +309,15 @@ public function testCreateAttribute() {
    +      ['class' => ['puppies']],
    +      ['class' => ['dollies']],
    

    Let's add some attributes that aren't class, maybe try a data-* attribute. And also, let's leave some empty ones too, just for completeness.

Charlotte17’s picture

Status: Needs work » Needs review
FileSize
1.31 KB
3.36 KB

Fixed 2 items to comments #34
1.Whitespace
2. Add some attributes

Status: Needs review » Needs work

The last submitted patch, 35: allow_instantiating-2616756-35.patch, failed testing.

T&#039;Bell Hernandez’s picture

Status: Needs work » Needs review
FileSize
3.05 KB
  1. +++ b/core/lib/Drupal.php
    @@ -81,7 +81,7 @@ class Drupal {
    -  const VERSION = '8.2.1-dev';
    

    fixed

  2. +++ b/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php
    @@ -299,6 +299,26 @@ public function testRenderVarWithGeneratedLink() {
    +  ¶
    

    space fixed

  3. +++ b/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php
    @@ -299,6 +299,26 @@ public function testRenderVarWithGeneratedLink() {
    +    $expected = '<div class="kittens" data-toggle="modal" data-lang="es"></div><div id="puppies" data-value="foo" data-lang="en"></div>';
    

    added empty div

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

#30 is addressed

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -600,4 +608,26 @@ public function safeJoin(\Twig_Environment $env, $value, $glue = '') {
+    if (!isset($this->attribute)) {
+      $this->attribute = new Attribute();
+    }
+    $cloned_attributes = clone $this->attribute;

Why the cloning dance?

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

@alexpott it's what we do in core for attributes that get created often for performance instead of re-instantiating the same class.

References:

core/includes/theme.inc:1571
core/lib/Drupal/Core/Theme/ThemeManager.php:364

We can do without it too, it's just to continue the pattern.

joelpittet’s picture

There is a missing space here that could be fixed on commit:
'data-value' =>['foo']

Or I'll fix it in a re-roll if we are going to remove the performance clone stuff.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I don't think that that micro-optimisation is worth in a method with only a single test call in core. If it becomes an issue then we could change it. Also I doubt that this test result still holds for PHP7... https://stackoverflow.com/questions/18040137/which-is-more-efficient-whe...

vaplas’s picture

Sorry, but it slower than just "return new Attribute($attributes);" in my test like

$renderer = \Drupal::service('renderer');
$extension = new TwigExtension($renderer);
$count = 10000;
$attrs = [];
$attrs =["class" => ['123']];

$before = microtime(true);
for ($i=0 ; $i<$count ; $i++) {
//    $extension->createAttribute($attrs);
    $extension->createAttribute2($attrs);
}
$after = microtime(true);
$result = $after - $before;

# result createAttribute vs createAttribute2
# $attrs = []
0.062520027160645 vs 0.055322885513306 (diff 0.007197141647339);

# $attrs = ["class" => ['123']]
0.19546914100647 vs 0.18453502655029 (diff 0.01093411445618);

In any case, it will not hit performance, in contrast to the simplicity and the beauty in this case.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
2.24 KB
2.51 KB

Removed the micro-optimization, thanks @alexpott. Also fixed the array space, added a description to the test and removed the extra arrays in the attributes that aren't needed(only class needs to be an array)

joelpittet’s picture

@vaplas Nice script, what version of PHP were you using and which method was which approach(could you edit your comment)?, I tried it and couldn't get a big diff either way which was closer to what @alexpott mentioned.

vaplas’s picture

@joelpittet, thank you for correctness to me and patch #44. I'm tested it - works well!

Also I know, that my script not nice and looks clumsily. I just wanted to quickly compare "new object" vs "cloning". I tested it with php 7.0.8 on Windows in theme_hook (e.g. hook_preprocess_node). With different order and different arguments. Without warm opcache and other special things. Every case "return new Attribute()" won by time. But i'm not saved the script and closed the editor already. I hope this information is enough and will be able to repeat it, if the desire remains :)

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Latest patch works and looks good!

alexpott’s picture

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

We also need a CR to tell people about this new functionality.

lauriii’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record

Created CR

vaplas’s picture

I know that my next words may irritate after all this work. But my heart will not feel comfortable if I did not ask about it: create_attribute - it is real pretty name? I know that "attributes" exist in Twig. But may be:

  • attrs
  • attribs
  • tag_attributes
  • create_attributes

or other words "params", "pairs". Although the attributes - the official term HTML.

After fixing this issue the name will forever. If there is a chance to use the most lovely name, we must to use it.

<div{{ create_attribute({..}) }}>
vs
<div{{ attribs({..}) }}>

In any case, thank you for the realization of this issue. Templates certainly become better with it.

lauriii’s picture

After fixing this issue the name will forever

That is not correct. We can change it later with BC layer in case we feel like changing it.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

I'm not against the name we chose, the short hand ones are a no go at all, as we try to use full words and not abbreviations in core.

joelpittet’s picture

Also this has some sense, it's clear what it's doing. It creates an Attribute object, so follows the name and doesn't obfuscate that fact, even though that name singular has thrown me a few times, it makes sense.

heddn’s picture

+1 on create_attribute. It was what I naturally was planning to call this thing when I first looked at this issue.

vaplas’s picture

Ok. Now I look at the "create_attribute" as "new Attribute" and see the sence too. Thanks for your callbacks and your brains :)

Cottser’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -600,4 +601,17 @@ public function safeJoin(\Twig_Environment $env, $value, $glue = '') {
+   *   An associative array of key-value pairs to be converted to attributes.

This should be prefixed with (optional) per https://www.drupal.org/node/1354#param.

Related to this, maybe we should test that calling create_attribute with no arguments still returns an empty attribute object because that would be useful I'd say.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.64 KB
2.79 KB

This should work as a test, uses the method on the object without any args added to the create_attribute function.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Looks great

  • Cottser committed 99a55f0 on 8.3.x
    Issue #2616756 by Charlotte17, joelpittet, lauriii, heddn, ytl, T'Bell...
Cottser’s picture

Status: Reviewed & tested by the community » Fixed
FileSize
889 bytes

Fixed a minor grammar thing on commit, interdiff attached.

Also made some tweaks to and published the change record.

Committed 99a55f0 and pushed to 8.3.x. Thanks!

Cottser’s picture

FileSize
889 bytes

Oops wrong interdiff (wrong folder). This one.

Status: Fixed » Closed (fixed)

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