Twig using the \Countable interface when it does an if on an object - all of our SafeStringsInterface implementing objects should do this.

CommentFileSizeAuthor
#33 2575867-10.0.x-32.patch6.84 KBalexpott
#33 29-32-interdiff.txt1.33 KBalexpott
#30 2575867-10.0.x-29.patch6.87 KBalexpott
#30 28-29-interdiff.txt669 bytesalexpott
#29 2575867-10.0.x-29.patch6.87 KBalexpott
#29 28-29-interdiff.txt669 bytesalexpott
#28 2575867-10.0.x-28.patch6.79 KBalexpott
#28 25-28-interdiff.txt1.71 KBalexpott
#27 interdiff_25-27.txt376 bytespooja saraah
#27 2575867-27.patch5.45 KBpooja saraah
#25 2575867-10.0.x-25.patch5.52 KBalexpott
#25 21-25-interdiff.txt1.05 KBalexpott
#24 interdiff_23-24.txt804 bytesbruno.bicudo
#24 2575867-24.patch6.55 KBbruno.bicudo
#23 interdiff_21-23.txt448 bytesravi.shankar
#23 2575867-23.patch5.86 KBravi.shankar
#21 interdiff-19-21.txt3.82 KBsmustgrave
#21 2575867-21.patch5.85 KBsmustgrave
#19 2575867-19.patch1.32 KBsmustgrave
#5 2575867-3.patch5.1 KBalexpott
#5 2-3-interdiff.txt2.22 KBalexpott
#2 2575867-2.patch2.88 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 2575867-2.patch2.88 KB

Status: Needs review » Needs work

The last submitted patch, 2: 2575867-2.patch, failed testing.

The last submitted patch, 2: 2575867-2.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 2-3-interdiff.txt2.22 KB
new 2575867-3.patch5.1 KB

Ah Attribute was why I didn't do this originally but this was a mistake.

dawehner’s picture

dawehner
Primary language German
commented

Does that mean we need a test for that behaviour?

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

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

dawehner
Primary language German
commented
Title: SafeStringInterface should implement \Countable » MarkupInterface should implement \Countable
Status: Needs review » Needs work

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review
StatusFileSize
new 2575867-19.patch1.32 KB
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

Countable::count() must be declared as int return type in PHP 8.

smustgrave’s picture

smustgrave commented
StatusFileSize
new 2575867-21.patch5.85 KB
new interdiff-19-21.txt3.82 KB

@longwave not sure I know what you mean? Like you want it as

public function count(): int;

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

The error that DrupalCI is reporting is

Deprecated: Return type of Drupal\Component\Render\MarkupInterface::count() should either be compatible with Countable::count(): int, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/core/lib/Drupal/Component/Render/MarkupInterface.php on line 50
ravi.shankar’s picture

ravi.shankar commented
StatusFileSize
new 2575867-23.patch5.86 KB
new interdiff_21-23.txt448 bytes

Trying to fix comment #22.

bruno.bicudo’s picture

bruno.bicudo
Primary language Portuguese, Brazil
Location Bragança Paulista - São Paulo
commented
StatusFileSize
new 2575867-24.patch6.55 KB
new interdiff_23-24.txt804 bytes

Trying to fix #23. Kindly review it.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 21-25-interdiff.txt1.05 KB
new 2575867-10.0.x-25.patch5.52 KB

There's no need for MarkupInterface to define count - that's coming from \Countable.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review 
+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -329,6 +329,14 @@ public function __toString() {
+  /**
+   * {@inheritdoc}
+   */
+  #[\ReturnTypeWillChange]
+  public function count() {
+    return count($this->__toString());
+  }

This one is interesting. I think we should implement this has return count($this->storage); because this is an array of strings and implements ArrayAccess so it's not really the same as other things here.

pooja saraah’s picture

pooja saraah commented
StatusFileSize
new 2575867-27.patch5.45 KB
new interdiff_25-27.txt376 bytes

Addressed comment #26
Attached interdiff patch

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 25-28-interdiff.txt1.71 KB
new 2575867-10.0.x-28.patch6.79 KB

Looking at twig_length_filter(), twig_test_empty() yeah I think we should do ##2 it means we won't render attributes twice.

Here's what twig_test_empty() is doing:

function twig_test_empty($value)
{
    if ($value instanceof \Countable) {
        return 0 === \count($value);
    }

    if ($value instanceof \Traversable) {
        return !iterator_count($value);
    }

    if (\is_object($value) && method_exists($value, '__toString')) {
        return '' === (string) $value;
    }

    return '' === $value || false === $value || null === $value || [] === $value;
}

Hmmm... on the other hand this new implementation is more consistent with expectations! Because we're only providing a positive count when something is rendered... i.e we are accounting now for the logic in

    $return = '';
    /** @var \Drupal\Core\Template\AttributeValueBase $value */
    foreach ($this->storage as $value) {
      $rendered = $value->render();
      if ($rendered) {
        $return .= ' ' . $rendered;
      }
    }
    return $return;

So actually the current implementation of \Drupal\Core\Template\Attribute::count() is not right - you can't call count() on a string in PHP 8.1 without generating a deprecation. The patch attached fixes this. Hmmm... this is all very tricky. At the moment Twig is using the 

    if ($value instanceof \Traversable) {
        return !iterator_count($value);
    }

For attributes however this is giving the wrong answer for an attribute like this:

    $attributes = new Attribute();
    $attributes['class'] = [];

Because it will give the answer of 1 rather than 0 but such an attribute will render to an empty string.

What do we want an attribute object such as the one above to count to? Looking at twig_test_empty() I think we want it to count to 0 as nothing will be rendered.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 28-29-interdiff.txt669 bytes
new 2575867-10.0.x-29.patch6.87 KB

Making the test more explicit about how ArrayAttributes are counted... maybe we should change that. Not sure.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 28-29-interdiff.txt669 bytes
new 2575867-10.0.x-29.patch6.87 KB

Re #29 - yeah I think we should be more elegant here. And count the number of class in the count. So something like this:

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

#30 is the same as #29?

smustgrave’s picture

smustgrave commented

Compared #29 and #30 and it does appear to be the same patch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 29-32-interdiff.txt1.33 KB
new 2575867-10.0.x-32.patch6.84 KB

Here's the patch I meant to upload.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Now looking at #33 I'm not so sure that it is better than #30 ... like count on the attributes object should return the number of attributes that would be printed... so additional classes should not affect the count...

smustgrave’s picture

smustgrave commented

So should this move back to NW ?

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs issue summary update

Seems there is still discussion.

Think whatever solution there is should be added to the issue summary.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

acbramley’s picture

acbramley commented
Issue tags: +Bug Smash Initiative, +Needs reroll

Came up in BSI. Is this still relevant? Looks like we still need an IS update and now need a reroll on to an MR if so.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.