Problem/Motivation

I tried doing this in a Twig template:

{{ link(item.title, item.url, item.attributes) }}

item.attributes was a Drupal\Core\Template\Attribute object and therefore I got this error:

Twig_Error_Runtime: An exception has been thrown during the rendering of a template ("A stray drupal_render() invocation with $is_root_call = TRUE is causing bubbling of attached assets to break.") in "themes/example/templates/menu.html.twig" at line 36. in Twig_Template->displayWithErrorHandling() (line 328 of core/vendor/twig/twig/lib/Twig/Template.php).

Proposed resolution

Let the link() function in Twig accept either an array of attributes or an Attribute object.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it completes a piece of functionality in the Twig layer.
Issue priority Normal, Attribute objects are common in D8 so this is making them usable in the link context in Twig.
Unfrozen changes Unfrozen because it only changes markup by way of link attributes.
Prioritized changes Not a prioritized change.
Disruption Not disruptive, fully backwards compatible.

Remaining tasks

None at this time.

User interface changes

n/a

API changes

Small backwards compatible API addition.

Data model changes

n/a

Files: 
CommentFileSizeAuthor
#31 make_the_twig_extension-2506151-31.patch6.16 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,261 pass(es). View
#31 interdiff.txt823 bytesjoelpittet
#28 interdiff.txt1017 byteslauriii
#28 make_the_twig_extension-2506151-28.patch6.1 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,278 pass(es). View
#24 make_the_twig_extension-2506151-24.patch5.93 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,228 pass(es). View
#24 interdiff.txt2.14 KBjoelpittet
#22 interdiff.txt4.65 KBjoelpittet
#22 make_the_twig_extension-2506151-22.patch7.71 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,303 pass(es). View
#15 interdiff.txt468 bytesCottser
#15 make_the_twig_extension-2506151-15.patch5.48 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,111 pass(es). View
#8 interdiff.txt2.85 KBjoelpittet
#8 make_the_twig_extension-2506151-8.patch5.41 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,077 pass(es). View
#4 make_the_twig_extension-2506151-4.patch4.45 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,065 pass(es), 8 fail(s), and 0 exception(s). View

Comments

Cottser’s picture

Very interesting point, that came up briefly in #2342745-32: Allow Twig link function to pass in HTML attributes but didn't make it into any patch.

Do you think it should be able to accept either?

Lukas von Blarer’s picture

I don't have enough knowledge to judge since I don't know the implications this would have. I think yes.

jibran’s picture

Category: Support request » Task

Let's allow both array and object.

joelpittet’s picture

Status: Active » Needs review
FileSize
4.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,065 pass(es), 8 fail(s), and 0 exception(s). View

@Lukas von Blarer try this out and let us know.

jibran’s picture

Looks perfect to me.

Status: Needs review » Needs work

The last submitted patch, 4: make_the_twig_extension-2506151-4.patch, failed testing.

Cottser’s picture

Looks like a missing use statement for Attribute?

joelpittet’s picture

Status: Needs work » Needs review
FileSize
5.41 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,077 pass(es). View
2.85 KB

My tests were shit too:)

Cottser’s picture

Looks RTBC, needs a bit of an issue summary update and beta eval, don't have time right now to do so.

The last submitted patch, 4: make_the_twig_extension-2506151-4.patch, failed testing.

joelpittet’s picture

Issue summary: View changes
Issue tags: -Needs beta evaluation

@Lukas von Blarer Mind updating the issue summary?

joelpittet’s picture

Issue summary: View changes
Cottser’s picture

Issue summary: View changes
jibran’s picture

Let's update some old change notice as well.

Cottser’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update
FileSize
5.48 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,111 pass(es). View
468 bytes

@jibran I agree but I don't think one exists. We could expand the docs on https://www.drupal.org/node/2486991 after commit.

Updated the issue summary. RTBC with a tiny docs addition to add the @return, didn't think that was worth holding this up.

jibran’s picture

Awesome. RTBC +1. Thanks for reporting the issue @Lukas von Blarer. Nice fix @joelpittet.

joelpittet’s picture

@jibran Do you happen to know where or if there was an old change record?

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -262,6 +262,22 @@ public function __toString() {
    +    /** @var \Drupal\Core\Template\AttributeValueBase $value */
    

    Instead of doing this, it should be on $this->storage:

    @@ -43,10 +43,11 @@
      * htmlspecialchars() and the entire attribute string is marked safe for output.
      */
     class Attribute implements \ArrayAccess, \IteratorAggregate, SafeStringInterface {
    +
       /**
        * Stores the attribute data.
        *
    -   * @var array
    +   * @var \Drupal\Core\Template\AttributeValueBase[]
        */
       protected $storage = array();
     
    
  2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -260,17 +260,20 @@ public function getUrlFromPath($path, $options = array()) {
    +   * @param array|Attribute $attributes
    

    This should have FQCN: \Drupal\Core\Template\Attribute

  3. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -260,17 +260,20 @@ public function getUrlFromPath($path, $options = array()) {
    +      if ($attributes instanceOf Attribute) {
    

    instanceof

  4. +++ b/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php
    @@ -57,6 +58,7 @@ public function linkGeneratorRender() {
    +      '#attributes' => new Attribute(['class' => ['llama', 'kitten', 'panda']]),
    

    Also, it seems weird that we have #attributes (and $attributes above) getting a single Attribute

joelpittet’s picture

Status: Reviewed & tested by the community » Needs work

@tim.plunkett

re #18.1 It won't merge properly if we don't get the value(). Actually I think this needs another test because it should be a deep merge regardless and that is a bug before this patch.

#18.2 cool thanks, will fix:)

#18.3 interesting didn't know that was the way we were doing it but ok cool.

#18.4 I actually should have put a comment for why I did that because I wanted to explicitly test the attribute object and not assume that special cased conversion. It would do the same thing as we have it but I didn't want the test to make extra assumptions.

tim.plunkett’s picture

18.1, I just meant skipping the inline @var, and moving it to the property itself.

18.3,

$ ag instanceOf --ignore-dir vendor | wc -l
      34
$ ag instanceof --ignore-dir vendor | wc -l
     669
joelpittet’s picture

@tim.plunkett well you helped me find a bug, so cool anyways:)

joelpittet’s picture

Status: Needs work » Needs review
FileSize
7.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,303 pass(es). View
4.65 KB

Here's those fixes from #18

Cottser’s picture

I'm not sure we should be changing those other instanceOf, just doing the new one lowercase. Otherwise, looks good.

joelpittet’s picture

FileSize
2.14 KB
5.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,228 pass(es). View

@Cottser of course you are right ;) Moved those to a novice follow-up #2515018: Lowercase the instances of camelcase'd instanceOf in core for consistency

Cottser’s picture

Status: Needs review » Reviewed & tested by the community

Nice, back to RTBC. Thanks @joelpittet @tim.plunkett.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module
@@ -43,7 +43,11 @@ function twig_theme_test_theme($existing, $type, $theme, $path) {
   $items['twig_theme_test_link_generator'] = array(
...
+    'variables' => [

This is not a good thing because at least my texteditor will give an warning for having new syntax inside old syntax :/

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community

We aren't blocking on array syntax diff.

lauriii’s picture

FileSize
6.1 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,278 pass(es). View
1017 bytes

Lets fix it anyway :)

joelpittet’s picture

OK, still RTBC;)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. This looks like a sensible improvement. It feels almost like a bug fix looking at the issue summary.
  2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -260,17 +260,20 @@ public function getUrlFromPath($path, $options = array()) {
    +   * @param array|\Drupal\Core\Template\Attribute $attributes
        *   An optional array of link attributes.
    

    These docs need updating.

  3. +++ b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module
    @@ -43,7 +43,11 @@ function twig_theme_test_theme($existing, $type, $theme, $path) {
    -    'variables' => array('test_url' => NULL, 'test_url_attribute' => NULL),
    +    'variables' => [
    +      'test_url' => NULL,
    +      'test_url_attribute' => NULL,
    +      'attributes' => [],
    +    ],
    

    Why is this change necessary?

joelpittet’s picture

Status: Needs work » Needs review
FileSize
823 bytes
6.16 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,261 pass(es). View

@alexpott re #30.2 Because we need to define any variable your template expects. Added 'attributes' to explicitly pass an Attribute object to the template. I tried to change the logic to make it looser on defining the variables here but got shot down:P If you want clarity, @fabianx is the one to ping.

Thanks for the note on the docs #30.1, missed that.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

#31 addresses #30

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for addressing my feedback. Committed df44c00 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed df44c00 on 8.0.x
    Issue #2506151 by joelpittet, Cottser, lauriii, Lukas von Blarer, tim....

Status: Fixed » Closed (fixed)

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