Problem/Motivation

#2285451: Create addClass() and removeClass() methods on Attribute object for merging css class names. added methods on the Attribute class to add and remove CSS classes. This is super useful and a key part of #2322163: [meta] Consensus Banana Phase 1, move CSS classes from preprocess to twig templates..

It would also be useful to have methods for adding/removing non-class attributes so that this could be done cleanly from within Twig templates.

Proposed resolution

Add methods to the Attribute object for adding/removing attributes, some proposed method names (we would only add one set, we need to pick from these or come up with another set):

set/remove
setAttribute/removeAttribute

setAttribute would accept the attribute name and value.
removeAttribute would accept the attribute name only.

Example syntax within a Twig template:

set/remove:

{% set classes = [
  'test1',
  'test2',
  'test3',
  'test4',
]
%}
<div {{ attributes.addClass(classes).set('data-foo', 'bar').remove('role') }}></div>

setAttribute/removeAttribute:

{% set classes = [
  'test1',
  'test2',
  'test3',
  'test4',
]
%}
<div {{ attributes.addClass(classes).setAttribute('data-foo', 'bar').removeAttribute('role') }}></div>

Remaining tasks

Patch
Tests

User interface changes

n/a

API changes

API addition to be able to manipulate non-class attributes on Attribute objects.

Files: 
CommentFileSizeAuthor
#63 interdiff.txt1.05 KBlauriii
#63 add_methods_for-2325517-63.patch5.17 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,080 pass(es). View
#58 interdiff-53-58.txt477 bytesTom Verhaeghe

Comments

joelpittet’s picture

I'm in favour of the API addition and just 'set/remove' pair added. addClass/removeClass do array manipulation. set() will add or replace.

Duplicate attributes aren't ideal so, add may give the wrong impression. 2¢

Cottser’s picture

Issue summary: View changes

To be clear I'm not proposing adding both sets, just thinking about what will be intuitive for people. I don't have a strong opinion right now. Tweaked the issue summary to indicate that we need to pick.

davidhernandez’s picture

So it is set/add versus addAttribute/removeAttribute? (or maybe add is just setAttribute) setAttribute/removeAttribute seems clearer, and is following the same naming convention.

At first glance, this line might be confusing:

<div {{ attributes.addClass(classes).remove('role') }}></div>

Is it removing a class?

But this should be clear:

<div {{ attributes.addClass(classes).removeAttribute('role') }}></div>
crowdcg’s picture

@Cottser I agree with @davidhernandez, I think it is important that the syntax be consistent and also easy to understand. So +1 to addAttribute/removeAttribute.

joelpittet’s picture

You need to know that you are working on an Attribute object for those methods to work in the first place so it seems redundant to add that to the method names as well.

Also addClass(class1, class2, class3) takes and array and merges onto one attribute.
@see http://api.jquery.com/addclass/

addAttribute(name, value)/set(name, value) would not merge it would set the attribute to a value or override it's value.
@see http://api.jquery.com/attr/

Description: Set one or more attributes for the set of matched elements.

Which could do this nice bit:

attributes.set({
  alt: "Beijing Brush Seller",
  title: "photo by Kelly Clark"
});

or we could just copy them directly and call it attr()/removeAttr() though that doesn't seem normal naming conventions for Drupal.

davidhernandez’s picture

Sure, if you have a good understanding of how this works, you would know you are dealing with an Attribute object, but will your average themer know that or even understand what it means? Most people don't think about the behind the scenes, they are only thinking about what is directly exposed to them. My concern is that if the naming convention isn't consistent, it makes it harder to remember and intuit.

I agree that set over add makes sense, if there isn't anything to actually add.

Cottser’s picture

I like @davidhernandez's setAttribute/removeAttribute suggestion. In this case I think a bit of verbosity is for the best.

Cottser’s picture

I just remembered that removing attributes can already be done now via the |without filter, but no such luck with |merge of course. Looking at the jQuery stuff I wonder if adding or removing should support arrays/hashes as shown in @joelpittet's #5:

{{ attributes.setAttribute({
  'alt': "Beijing Brush Seller",
  'title': "photo by Kelly Clark",
}) }}

{{ attributes.removeAttribute(['alt', 'title']) }}

Otherwise you'd have to chain a bunch of set/remove to do multiple manipulations.

Edit: For remove you could just pass multiple arguments so this is more about adding but I still think remove should work either way, and I think the addClass/removeClass works like that already.

Cottser’s picture

Adding banana as related so this is more findable.

Wim Leers’s picture

While working on #1869476: Convert global menus (primary links, secondary links) into blocks, I stumbled upon needing to add an attribute in a Twig template, but it's sadly impossible AFAICT. Hoping this issue will move forward, because it's indeed going to be pretty common.

This will also allow us to remove most of system_preprocess_block() for example, which sets the role attribute on several blocks (amongst others).

Cottser’s picture

Issue tags: +sprint

Thanks @Wim Leers, that's great to know. Bumping this up at least in focus.

Cottser’s picture

Issue summary: View changes

Updating the issue summary to use replace addAttribute with setAttribute.

lauriii’s picture

Assigned: Unassigned » lauriii
lauriii’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
1.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,749 pass(es). View
lauriii’s picture

Assigned: lauriii » Unassigned
FileSize
3.13 KB
2.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
3.88 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,751 pass(es). View

The last submitted patch, 15: setattributes-2325517-15-tests.patch, failed testing.

lauriii’s picture

FileSize
420 bytes
3.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,080 pass(es). View

Fixed one documentation problem I found

joelpittet’s picture

Issue tags: -Needs tests

this is looking really good, it also has tests:) Just a couple of questions @lauriii

  1. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -161,6 +161,54 @@ public function addClass() {
    +    if ($attribute && $value) {
    

    This seems like it would prevent something like <img{{ attributes.setAttribute('alt', '') }}>

    Is this ok?

  2. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -161,6 +161,54 @@ public function addClass() {
    +      // If attribute exists no need to create new attribute.
    +      if (isset($this->storage[$attribute]) && $this->storage[$attribute] instanceOf AttributeArray) {
    

    The doc's may need to indicated this only referes to AttributeArray instances because they hold arrays, but otherwise this makes sense.

lauriii’s picture

FileSize
2.44 KB
3.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,380 pass(es). View

#18.1 actually makes sense so now we add even empty Attributes. Not sure if we should test if $value is FALSE and then we wouldnt set attribute. It would make sense but I didnt find any use case for that.

I think it actually makes more sense if setting class, we replace the whole attribute. I dont think setAttribute should be used as a replacement for addClass.

Status: Needs review » Needs work

The last submitted patch, 19: setattributes-2325517-19.patch, failed testing.

Status: Needs work » Needs review
lauriii’s picture

FileSize
846 bytes
3.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,612 pass(es). View
joelpittet’s picture

Status: Needs review » Needs work

@lauriii could you add a test in there to make sure this works with boolean attributes too?
like checked/selected?

I think that may be the only thing missing from this.

rteijeiro’s picture

Assigned: Unassigned » rteijeiro

Working on this one from DrupalCamp Novi Sad.

rteijeiro’s picture

Assigned: rteijeiro » Unassigned
Status: Needs work » Needs review
FileSize
4.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
1.74 KB

There is still a test that is not passing. Lauri is going to teach me how to fix it using his drunken coding skills.

Status: Needs review » Needs work

The last submitted patch, 25: setattributes-2325517-25.patch, failed testing.

rteijeiro’s picture

FileSize
4.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,562 pass(es). View
880 bytes

There is a problem when creating a boolean attribute using the constructor. Maybe we need a follow-up to dig into the problem. Anyway I fixed the test with a workaround.

rteijeiro’s picture

Status: Needs work » Needs review

Go testbot, go!!

Status: Needs review » Needs work

The last submitted patch, 27: setattributes-2325517-27.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
drifter’s picture

Status: Needs review » Needs work

Tested this manually in a TWIG template. Works well. One thing I found confusing was passing an array to setAttribute() - the syntax in #8 does not work (set multiple attributes at once). Passing an array as a value works

{{ attributes.setAttribute('class', ['class1', 'class2', 'class3']) }}

and results in joining the value array with spaces, which is basically only useful for the "class" attribute, I can't think of another use case. So maybe needs better documenting of the array passing version? Otherwise RTBC.

drifter’s picture

Discussed with lauriii. What mislead me was:

  /**
   * Adds attributes or merges them on to array of existing attributes.
   *
   * @param string|array ...
   *   Attributes to add to the class attribute array.
   *
   * @return $this
   */

If you're using the array syntax, you're not adding multiple attributes (as in alt, title, href), you're adding multiple attribute values (as in class1, class2, class3). Maybe change documentation to say something like:

  /**
   * Adds attribute or merges values on to array of existing attribute values.
   *
   * @param string $attribute
   *   Attribute name
   * @param string|array $value
   *   Value(s) to add or merge to the given attribute
   *
   * @return $this
   */
lauriii’s picture

Thanks, I missed that. The documentation is completely wrong!

davidhernandez’s picture

Issue tags: +Needs change record

Should get a change record with this API addition.

lauriii’s picture

Status: Needs work » Needs review
FileSize
666 bytes
4.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,753 pass(es). View

Fixed the documentation on setAttribute

drifter’s picture

Status: Needs review » Reviewed & tested by the community

Looks ready to me.

lauriii’s picture

Interdiff was 666 bytes, not sure if its a good thing to but this into core ;)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -161,6 +161,50 @@ public function addClass() {
+    // If attribute key exists we can set attribute.
+    if ($attribute) {

The if does not appear to be tested and i'm not sure why we have it. Do we anticipate code calling this with $attribute set to FALSE or NULL?

Also #35 asks for a CR and we don't have one.

alexpott’s picture

re #38 and on halloween too!

davidhernandez’s picture

Issue tags: -Needs change record

I added a draft change record.

Cottser’s picture

Thanks all!

I agree we should remove the if @alexpott mentioned in #39, it feels like babysitting.

  1. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -161,6 +161,50 @@ public function addClass() {
    +   *   Name of the Attribute.
    

    s/Attribute/attribute, we are not referring to the Attribute object.

  2. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -161,6 +161,50 @@ public function addClass() {
    +   * Removes argument from array of existing attributes.
    

    Maybe "Removes an attribute from an Attribute object" here? Internally it's an ArrayObject anyway, not just an array... :)

lauriii’s picture

Issue tags: +Novice
Alienpruts’s picture

Assigned: Unassigned » Alienpruts
Issue tags: +DrupalCamp Ghent 2014
Alienpruts’s picture

Assigned: Alienpruts » Unassigned
Status: Needs work » Needs review
FileSize
1.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,975 pass(es). View
498 bytes

As suggested by Alexpott at #39 : removed the if-check and tested.

Be gentle : first sprinter here :)

Alienpruts’s picture

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

Working in #42, sorry Cottser, forgot about it :)

Alienpruts’s picture

Assigned: Alienpruts » Unassigned
Status: Needs work » Needs review
FileSize
1.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,984 pass(es). View
975 bytes

Changed lines as commented by Cottser #42, removed a forgotten comment line in previous patch.

Learning as we go :)

roderik’s picture

Status: Needs review » Needs work

@Alienpruts: learning as you go is the first objective :)

The change to a/core/tests/Drupal/Tests/Core/Template/AttributeTest.php fell out of your uploaded patch.

Also, general question:

index cebe699..2c0874b 100644
--- a/core/lib/Drupal/Core/Template/Attribute.php

@@ -161,6 +161,47 @@ public function addClass() {
+   * Adds attribute or merges values on to array of existing attribute values.
+   *
+   * @param string $attribute
+   *   Name of the attribute.
+   * @param string|array $value
+   *   Value(s) to add or merge to the given attribute.
+   *
+   * @return $this
+   */
+  public function setAttribute($attribute, $value) {
+      $this->offsetSet($attribute, $value);

looking into offsetSet(), it doesn't look to me this actually merges stuff, it completely overwrites any existing value. (Or?)

If so, then the question is: is this supposed to be able to merge attributes into existing ones? My guess is no (i.e. the code is fine and the comment should be amended).

But I don't use this code, so what do I know...

davidhernandez’s picture

@roderik, setAttribute() should override an existing value. If the attribute already has 'id' defined, it will be changed, which is the intention. The only case where this would really be an issue is array attributes, like 'class'. But class is one of the only attributes that uses multiple values, and we have addClass() to account for that.

Alienpruts’s picture

Assigned: Unassigned » Alienpruts
Alienpruts’s picture

Assigned: Alienpruts » Unassigned
Status: Needs work » Needs review
FileSize
4.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,986 pass(es). View
1006 bytes

OK, bear with me...

Redid patch #36, included all changes since then.

Interdiff is from #36 on.

Problem was an issue with git (or more precisely, my mis-use of the git apply command :) )

Sorry, hope this works...

roderik’s picture

Status: Needs review » Needs work

#49 Cool, thanks. Alienpruts will pick this up.

Tom Verhaeghe’s picture

FileSize
662 bytes
4.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,932 pass(es). View
Tom Verhaeghe’s picture

Status: Needs work » Needs review
Alienpruts’s picture

Hello Tom,

tnx for the patch, appreciate it.

In the future, may I suggest you put a small comment with it describing what it is you did? Would be super helpful :)

Tnx again :)

Tom Verhaeghe’s picture

Alienpruts: I will do so. Baby steps ;-)

lauriii’s picture

Status: Needs review » Needs work

@Tom Verhaeghe: Thank you for working on this issue! I found few more things on the issue that could be fixed before moving this forward. Would you like to take look on these?

  1. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -161,6 +161,47 @@ public function addClass() {
    +      $this->offsetSet($attribute, $value);
    

    This row's indentation is wrong

  2. +++ b/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
    @@ -58,6 +58,74 @@ public function testRemove() {
    +    // Boolean value.
    +    $attribute = new Attribute();
    +    $attribute->setAttribute('checked', TRUE);
    

    Should we create follow-up issue to fix the problem why we cannot add boolean type of attributes on the initialization of the Attributes object. This is introduced on #27

Tom Verhaeghe’s picture

Status: Needs work » Needs review
FileSize
477 bytes
4.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,080 pass(es). View

Here's a fix where I corrected the indentation.

Alienpruts’s picture

Tnx Tom, looks good :)

@laurrii : I'm for creating a follow-up issue.

joelpittet’s picture

+++ b/core/lib/Drupal/Core/Template/AttributeBoolean.php
@@ -39,6 +39,13 @@ public function render() {
   /**
+   * Returns the boolean value.
+   */
+  public function value() {
+    return $this->value;
+  }
+
+  /**

This looks like it should move to AttributeValueBase so we don't have to add the same method on all 3 child classes.

@lauriii and @rteijeiro I don't see where it's failing on booleans in the constructor in either #27 or #25 can you add it to a test, maybe we can solve it here?

joelpittet’s picture

FileSize
3.84 KB
4.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,036 pass(es). View

I moved the value method to the AttributeValueBase, and tested boolean in constructor which passes locally, and used short array syntax.

joelpittet’s picture

FileSize
5.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,077 pass(es). View
1.42 KB

Moved the boolean assertions to the correct test, the constructor test.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
5.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,080 pass(es). View
1.05 KB

I moved testing boolean also to the constructor on testRemoveAttribute().

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Parent issue: » #2348381: [META-0 theme functions left] Convert/refactor core theme functions

This issue is part of enabling the theme improvements of moving logic out of preprocess and into templates and so it allowed as per https://www.drupal.org/core/beta-changes and it's benefits outweigh any disruption. Committed 80c7322 and pushed to 8.0.x. Thanks!

  • alexpott committed 80c7322 on 8.0.x
    Issue #2325517 by lauriii, Alienpruts, Tom Verhaeghe, joelpittet,...

Status: Fixed » Closed (fixed)

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