Problem/Motivation

In MarkupInterface we say

* If the object is going to be used directly in Twig templates it should
* implement \Countable so it can be used in if statements.

However, FormattableMarkup, TranslatableMarkup etc... are all not implementing this interface and they should.

Proposed resolution

Make MarkupInterface extend \Countable so that if in Twig works as expected for all of these objects.

Remaining tasks

Commit

User interface changes

None

API changes

API additions to all MarkupInterface implementing objects.

Data model changes

None

Why should this be done during RC

Because working with these objects in Twig should be simple and not doing this will make it difficult to do simple things in twig like...

    {% if my_translated_variable is not empty %}
      <div class='morten_loves_divs'>
        <div class='for_good_measure'>
          {{ my_translated_variable }}
        </div>
      </div>
    {% endif %}
CommentFileSizeAuthor
#35 2600672-35.patch6.76 KBalexpott
#35 21-35-interdiff.txt456 bytesalexpott
#21 2600672-21.patch6.79 KBalexpott
#21 19-21-interdiff.txt632 bytesalexpott
#19 2600672-19.patch7.41 KBalexpott
#19 12-19-interdiff.txt6.86 KBalexpott
#12 2600672-12.patch8.17 KBalexpott
#12 9-12-interdiff.txt3.92 KBalexpott
#9 2600672-9.patch6.81 KBalexpott
#9 2600672-9.test-only.patch2.21 KBalexpott
#9 6-9-interdiff.txt2.7 KBalexpott
#6 interdiff.txt632 bytesdawehner
#6 2600672-6.patch4.41 KBdawehner
#4 2600672-4.patch3.79 KBalexpott
#3 2600672-3.patch12.98 KBdawehner
#2 2600672-2.patch12.98 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Active » Needs review
FileSize
2600672-2.patch12.98 KB

There we go.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
FileSize
2600672-3.patch12.98 KB

Wrong code

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2600672-4.patch3.79 KB

The patch still had extra stuff.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

Yeah I was really wondering why the patchfile didn't changed

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
FileSize
2600672-6.patch4.41 KB
interdiff.txt632 bytes

Let's readd the test.

The last submitted patch, 4: 2600672-4.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: 2600672-6.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
6-9-interdiff.txt2.7 KB
2600672-9.test-only.patch2.21 KB
2600672-9.patch6.81 KB

Added some more tests and fixed the fails.

The last submitted patch, 9: 2600672-9.test-only.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented 
+++ b/core/tests/Drupal/KernelTests/Core/Theme/TwigMarkupInterfaceTest.php
@@ -0,0 +1,58 @@
+ * Tests.

This text itself is not too verbose

In case you want to you could also expand the test coverage to include a GeneratedLink ...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
9-12-interdiff.txt3.92 KB
2600672-12.patch8.17 KB

Improve the tests and fixed GeneratedLink (nice spot).

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs review » Reviewed & tested by the community 
  1. index be1d354..21d8b35 100644
--- a/core/lib/Drupal/Core/GeneratedLink.php

--- a/core/lib/Drupal/Core/GeneratedLink.php
+++ b/core/lib/Drupal/Core/GeneratedLink.php

+++ b/core/lib/Drupal/Core/GeneratedLink.php
@@ -8,6 +8,7 @@

@@ -8,6 +8,7 @@
 namespace Drupal\Core;
 
 use Drupal\Component\Render\MarkupInterface;
+use Drupal\Component\Utility\Unicode;
 use Drupal\Core\Render\BubbleableMetadata;

    Oh shit, sometimes we actually have to test stuff ... 

  2. +++ b/core/tests/Drupal/KernelTests/Core/Theme/TwigMarkupInterfaceTest.php
@@ -8,36 +8,35 @@
   /**
+   * Modules to enable.
+   *
+   * @var array
+   */

    I'd just use inheritdoc, but nevermind

The last submitted patch, 4: 2600672-4.patch, failed testing.

The last submitted patch, 6: 2600672-6.patch, failed testing.

The last submitted patch, 9: 2600672-9.test-only.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue summary: View changes
effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented
Title: MarkupInterface should extend \Countable » Core MarkupInterface implementations used in Twig templates should implement \Countable
Status: Reviewed & tested by the community » Needs work
Issue tags: -rc target triage +rc target

Discussed with the other committers, and we agreed it makes sense to make core MarkupInterface implementations implement \Countable per the existing docs of MarkupInterface that say to do that. But we shouldn't change MarkupInterface itself, since that could result in a fatal error on existing sites using contrib/custom code with their own MarkupInterface implementations.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
12-19-interdiff.txt6.86 KB
2600672-19.patch7.41 KB

Changed the implementation to match #18 and adjusted test to ensure that objects that don't implement countable work as expected. Given the originally I didn't implement \Countable on Attribute I removed it.

Status: Needs review » Needs work

The last submitted patch, 19: 2600672-19.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
19-21-interdiff.txt632 bytes
2600672-21.patch6.79 KB

Some left over test code.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

Can we at least document on MarkupInterface that implementations of this interface should ideally implement \Countable as well, in order to be better usable in twig?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

#22 that documentation exists already :)

 *
 * If the object is going to be used directly in Twig templates it should
 * implement \Countable so it can be used in if statements.
 *
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Ah okay, so core just ignored its own documentation, nothing new.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul for Drupal Association commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/tests/Drupal/KernelTests/Core/Theme/TwigMarkupInterfaceTest.php
@@ -0,0 +1,112 @@
\ No newline at end of file

:(

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs work » Reviewed & tested by the community

@neclimdul can be fixed on commit, thanks for noticing that though.

The last submitted patch, 6: 2600672-6.patch, failed testing.

The last submitted patch, 4: 2600672-4.patch, failed testing.

The last submitted patch, 9: 2600672-9.test-only.patch, failed testing.

The last submitted patch, 19: 2600672-19.patch, failed testing.

The last submitted patch, 4: 2600672-4.patch, failed testing.

The last submitted patch, 6: 2600672-6.patch, failed testing.

The last submitted patch, 9: 2600672-9.test-only.patch, failed testing.

The last submitted patch, 19: 2600672-19.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
21-35-interdiff.txt456 bytes
2600672-35.patch6.76 KB

Let's fix it so a committer doesn't have to remember.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 5047475 on 8.0.x 
    Issue #2600672 by alexpott, dawehner: Core MarkupInterface...

Status: Fixed » Closed (fixed)

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