Add methods for adding/removing attributes (not classes) on Attribute objects

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¢

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>

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

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.

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.

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

Fixed one documentation problem I found

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.

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

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

Working on this one from DrupalCamp Novi Sad.

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

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.

Go testbot, go!!

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.

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
   */

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

Should get a change record with this API addition.

Fixed the documentation on setAttribute

Looks ready to me.

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

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

re #38 and on halloween too!

I added a draft change record.

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

Be gentle : first sprinter here :)

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

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

Learning as we go :)

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

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

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

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

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

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

@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

Here's a fix where I corrected the indentation.

Tnx Tom, looks good :)

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

+++ 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?

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

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

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

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!

Edit: ugh. wrong issue, sorry -- too many tabs open! :(

Anyway, I was trying to use setAttribute() to solve an unrelated issue and it didn't work (i moved the comment to the right issue #293295-11: Increment <ol> numbers on views paged results, i know this is the wrong place to put this, so again I apologize.