Problem/Motivation

Follow-up to #2330731: Attribute::addClass() adds empty class to cover more cases and add the corresponding test coverage.

#2330731: Attribute::addClass() adds empty class fixed the case of Attribute->addClass() (with no arguments) resulting in class="", but there are still several other cases not accounted for that will result in class="".

The main usage case in core is when adding classes from Twig template and using ternary operators. If not specified in the ternary, a false value returns an empty string (http://twig.sensiolabs.org/doc/templates.html#other-operators). Attribute is still returning an empty class attribute when it's passed in an empty string.

Proposed resolution

Fix it.

Remaining tasks

Patch + tests.

User interface changes

n/a

API changes

n/a

Files: 
CommentFileSizeAuthor
#20 2336355.20.patch6.05 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,618 pass(es). View
#20 16-20-interdiff.txt1.43 KBalexpott
#16 2336355.16.patch6.04 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,283 pass(es). View
#16 14-16-interdiff.txt1.33 KBalexpott
#14 2336355.14.patch5.8 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,330 pass(es). View
#14 12-14-interdiff.txt518 bytesalexpott
#13 10-12-interdiff.txt1.25 KBalexpott
#13 2336355.12.patch5.8 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,311 pass(es). View
#11 9-10-interdiff.txt1.85 KBalexpott
#11 2336355.10.patch5.6 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,311 pass(es). View
#10 2336355.9.patch4.69 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,311 pass(es). View
#6 interdiff.txt882 bytesCottser
#6 2336355-6.patch1.93 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,184 pass(es). View
#2 2336355-2.patch2.01 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,183 pass(es). View
#2 2336355-2-testonly.patch1.17 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,097 pass(es), 1 fail(s), and 0 exception(s). View

Comments

Cottser’s picture

Issue summary: View changes

Adding a bit more information to the summary.

Cottser’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
1.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,097 pass(es), 1 fail(s), and 0 exception(s). View
2.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,183 pass(es). View

Here's one proposed fix anyway. I know in the original addClass/removeClass issue we didn't want to add array_filter() on print. This is just modifying the addClass(), not on print.

We may need another issue to address what happens when you removeClass() all the classes ;)

The last submitted patch, 2: 2336355-2-testonly.patch, failed testing.

mdrummond’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me. Tests seem to be set up correctly. Pretty straightforward. Let's get this in, so we can move forward on other preprocess conversions. We can file another issue for remove classes if necessary.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -143,7 +143,12 @@ public function addClass() {
+        $classes = array_filter(array_merge($classes, (array) $arg));
+      }
+      // If there are no classes to add, return early.

Let's just do one array_filter()... we can move $classes = array_filter($classes); outside the loop.

Cottser’s picture

Status: Needs work » Needs review
FileSize
1.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,184 pass(es). View
882 bytes

Good point!

Cottser’s picture

Status: Needs review » Reviewed & tested by the community

This is blocking and #5 has been addressed so I'm going to move this back to RTBC per #4.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Why is AttributeArray not protecting us from this?

  /**
   * Implements the magic __toString() method.
   */
  public function __toString() {
    // Filter out any empty values before printing.
    $this->value = array_filter($this->value);
    return String::checkPlain(implode(' ', $this->value));
  }
Cottser’s picture

Assigned: Unassigned » Cottser

I'll see if I can do another take on this by overriding render in AttributeArray.

alexpott’s picture

Assigned: Cottser » Unassigned
FileSize
4.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,311 pass(es). View

Oops - I already did that :(

How about something like the patch attached - this means we only do the filtering and uniqueness once per ArrayAttribute and on render - plus all array attributes get this benefit.

Note that I didn't push these change to AttributeValueBase::render() is because it breaks expectations around code like this:

$a1 = new Attribute(array('alt' => ''));
print $a1;
$a2 = new Attribute(array('alt' => NULL));
print $a2

The first print will output alt="" the second print will output nothing. This is tested in AttributesTest (btw AttributesTest vs AttributeTest :) that's nuts).

Personally I feel the the ability to output alt="" and prevent class="" are kind of contrary expectations.

No interdiff because it is a new approach.

alexpott’s picture

FileSize
5.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,311 pass(es). View
1.85 KB

Refined the approach so that ArrayAttribute does not have a custom render.

Cottser’s picture

Title: Attribute::addClass() (still) adds empty class » Refactor Attribute rendering so that class="" is not printed
Issue tags: -Quick fix

Okay that looks much better than anything I would have come up with. Thank you so much @alexpott. removeClass() is also fixed with this patch!

alexpott’s picture

FileSize
5.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,311 pass(es). View
1.25 KB
+++ b/core/lib/Drupal/Core/Template/AttributeValueBase.php
@@ -17,6 +17,11 @@
   /**
+   * Renders '$name=""' if $value is to an empty string.
+   */
+  const PRINT_IF_EMPTY_STRING = TRUE;

@Cottser noticed an extra word

alexpott’s picture

FileSize
518 bytes
5.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,330 pass(es). View

lol - thanks @Cottser

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Awesome, this was exactly the way I was going on Friday, but I got stuck in trying to debug the UnitTest failures... then gave up :(

  1. +++ b/core/lib/Drupal/Core/Template/AttributeArray.php
    @@ -30,6 +30,14 @@
    +  const PRINT_IF_EMPTY_STRING = FALSE;
    

    Didn't think to abstract out that exception for alt tags, nice!

  2. +++ b/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
    @@ -69,6 +69,25 @@ public function testAddClasses() {
    +      $this->assertEmpty((string) $attribute);
    

    This is great, actually testing the output and not the structure.

  3. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -185,7 +181,7 @@ public function removeClass() {
    -      $classes = array_diff($this->storage['class']->value(), $classes);
    +      $classes = array_values(array_diff($this->storage['class']->value(), $classes));
    

    Not sure why this array_values() is needed? Secret sauce?

alexpott’s picture

FileSize
1.33 KB
6.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,283 pass(es). View

Re #15.3 because it reindexes the array. So if the values array is 'bb', 'aa' if you remove 'bb' then the values array is 'aa' and if you var_dump() it it will be array(0 => 'aa') not array(1 => 'aa').

Patch adds a comments to explain and show where test coverage is. Leaving at RTBC since this just adds clarification.

Cottser’s picture

Thanks @alexpott! I was wondering about that too, makes sense.

joelpittet’s picture

Ah thanks for the clarification, yes still RTBC:) And now we have clean indexes too!

davidhernandez’s picture

Cottser and I were discussing this change on IRC and were curious about what affect on performance it might have. I did some profiling. I tested many times with a custom page that displays teasers for 100 nodes. I also tested a lot on just one node page. We have a few of the preprocess changes already in, including node (Thanks, Alex!) and that template alone has three calls to addClass in it. The numbers I'm seeing fluctuate between 0.5% slower and 0.5% faster, though the majority of runs were in the positive direction. I assume most of it is the margin of error. I'd, unscientifically, call the mode to be around 0.1% faster.

alexpott’s picture

FileSize
1.43 KB
6.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,618 pass(es). View

Improved constant name after discussions with @catch. This class is fully tested by AttributeTest and AttributesTest we should merge them into a single AttributeTest since Attribute is the class under test. Leaving at rtbc since this is a very minor change.

re #19 nice work - this is a patch that will keep on giving though - as we ramp up the use of addClass its affect will be bigger.

Cottser’s picture

+1, that name makes more sense to me because I think we were saying it could print for NULL, FALSE, etc. as well, not just empty strings.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed ae1a1d8 on 8.0.x
    Issue #2336355 by alexpott, Cottser: Fixed Refactor Attribute rendering...

Status: Fixed » Closed (fixed)

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