Follow-up to #1777332: Replace theme_menu_link() and menu-tree.html.twig with a single Twig template

Problem/Motivation

From the template we need to be able to add attributes to things that produce markup.

link() should be one of these things as it produces an A tag.

Proposed resolution

Allow a parameter to accept attributes on link() .

User interface changes

None.

API changes

Just an addition.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task, completing the API around adding links via Twig templates.
Issue priority Major, without these "proper" links can't be used in Twig templates
Prioritized changes Not a prioritized change, but unblocks issues such as #2345881: Refactor pager.html.twig to use the link generator
Disruption Not disruptive, just an API addition.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

chr.fritsch’s picture

Not sure how to handle link_from_url. Couldn't find a twig function with that name.

chr.fritsch’s picture

Status: Active » Needs review
star-szr’s picture

Status: Needs review » Needs work

It's just called 'link' I think so should be fine. Patch looks reasonable, this needs some test coverage. Thanks @chr.fritsch!

The last submitted patch, 2: drupal-add_attributes_to_twig_link-2342745-2.patch, failed testing.

chr.fritsch’s picture

Ok, i added one test and fixed the method doc

chr.fritsch’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 6: drupal-add_attributes_to_twig_link-2342745-6.patch, failed testing.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
2.58 KB

Build patch against HEAD

Status: Needs review » Needs work

The last submitted patch, 9: drupal-add_attributes_to_twig_link-2342745-9.patch, failed testing.

chr.fritsch’s picture

Dont overwrite existing attributes

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -204,17 +204,24 @@ public function getUrlFromPath($path, $options = array()) {
    +      $existing_attributes = $url->getOption('attributes');
    +      if (!$existing_attributes) {
    

    Did you considered to use (if (!is_null()) which is exactly what Url::getOption returns?

  2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -204,17 +204,24 @@ public function getUrlFromPath($path, $options = array()) {
    +          $existing_attributes = array();
    

    We do use two spaces instead of 4 spaces :(

  3. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -204,17 +204,24 @@ public function getUrlFromPath($path, $options = array()) {
           // @todo Convert once https://www.drupal.org/node/2306901 is in
    -      return _l($text, $url);
    +      return _l($text, $url, array('attributes' => $attributes));
    

    Meh, so people might always use relative paths from Drupal. Do we let themers on the longrun not use paths anymore? Just curious here.

chr.fritsch’s picture

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This is already a good progress ...

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -204,17 +204,24 @@ public function getUrlFromPath($path, $options = array()) {
    +      $existing_attributes = $url->getOption('attributes');
    +      if (!is_array($existing_attributes)) {
    +        $existing_attributes = array();
    +      }
    +      $url->setOption('attributes', array_merge($existing_attributes , $attributes));
    

    We only have test coverage where the url does not have existing attributes.

  2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -204,17 +204,24 @@ public function getUrlFromPath($path, $options = array()) {
    +      return _l($text, $url, array('attributes' => $attributes));
    

    We have no test coverage of this.

lauriii’s picture

Status: Needs work » Needs review
FileSize
4.63 KB
2.53 KB

I love writing tests!

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
siva_epari’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.5 KB

Patch rerolled.

Status: Needs review » Needs work

The last submitted patch, 18: allow_twig_link-2342745-18.patch, failed testing.

siva_epari’s picture

Status: Needs work » Needs review
FileSize
1.74 KB
4.51 KB

Small fixes.

star-szr’s picture

Status: Needs review » Needs work

Thank you @epari.siva!

  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -231,13 +231,20 @@ public function getUrlFromPath($path, $options = array()) {
    +  public function getLink($text, $url, $attributes = array()) {
    ...
    +        $existing_attributes = array();
    

    We could use short array syntax [] here. And might be good to type hint $attributes in the function signature:

    array $attributes = []
    
  2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -231,13 +231,20 @@ public function getUrlFromPath($path, $options = array()) {
    +      $url->setOption('attributes', array_merge($existing_attributes , $attributes));
    

    Wasn't introduced with the reroll but there is an extra space before the last comma here.

siva_epari’s picture

Status: Needs work » Needs review
FileSize
1.03 KB
4.5 KB

Fixed as per suggestions.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

More kitten tests!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: allow_twig_link-2342745-22.patch, failed testing.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community

Drunk testbot.

star-szr’s picture

Issue summary: View changes

Added beta evaluation.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -231,13 +231,20 @@ public function getUrlFromPath($path, $options = array()) {
    +   *   An array of link attributes
    

    Missing full stop. Also should be pointing to some further document about what can actually be in here? And this makes we wonder where are we documenting all twig methods we are adding?

  2. +++ b/core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.link_generator.html.twig
    @@ -1 +1,5 @@
    +
    

    This is an unnecessary new line. The file already has one.

siva_epari’s picture

Status: Needs work » Needs review
FileSize
1.36 KB
4.5 KB

Changes applied.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

@alexpott I'm not sure where we are either. (/me guilty of poor documenting)

star-szr’s picture

There's this page for filters: https://www.drupal.org/node/2357633

We could add a similar one for functions.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

I'm not sure all the feedback has been addressed here -- @alexpott suggested that the parameter should go into more detail or reference what can be in the "array of attributes".

joelpittet’s picture

@xjm and @alexpott I agree it could be expanded on, but it doesn't take an Attribute object so it's fairly simple structure... hmm I wonder how we can document this better...

Maybe as simple as:

An array of HTML link attributes as taken by Attribute::__construct() and merged with existing link attributes.

Was/am debating whether this should actually take in an Attribute Object as well and get it's array value before the merge, but really not sure if I should mess with it? If that is the case I'd likely balloon the scope by adding a getArrayCopy and ArrayIterator interface to Attribute, to make it easier to work with as an array.
http://php.net/manual/en/arrayiterator.getarraycopy.php

joelpittet’s picture

Issue tags: +Needs reroll

Or slightly better?

An array of HTML link attributes as taken by the Attribute class and merged with existing link attributes.

joelpittet’s picture

Priority: Normal » Major
Issue tags: +Novice

Moving to major because it blocks a major #2345881: Refactor pager.html.twig to use the link generator and needs a re-roll.

wesruv’s picture

Going to reroll this like a boss.

wesruv’s picture

Consider this re-rolled (probably)

Status: Needs review » Needs work

The last submitted patch, 36: allow_twig_link-2342745-36.patch, failed testing.

lauriii’s picture

Issue tags: -Needs reroll

Thanks for working on this issue! This actually starts looking good for me. Removing the Needs reroll tag since @wesruv rerolled this like a boss! Here is still some nit picks that I can find:

  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -222,13 +222,20 @@ public function getUrlFromPath($path, $options = array()) {
    +  public function getLink($text, $url, $attributes = array()) {
    

    Short array syntax could be used: $attributes = [].

  2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -222,13 +222,20 @@ public function getUrlFromPath($path, $options = array()) {
    +        $existing_attributes = array();
    

    Same here :)

  3. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -222,13 +222,20 @@ public function getUrlFromPath($path, $options = array()) {
    +   *   An array of link attributes
    

    This has to end for a dot.

star-szr’s picture

More nits! And the mode changes definitely need to be fixed.

  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -222,13 +222,20 @@ public function getUrlFromPath($path, $options = array()) {
    +      $url->setOption('attributes', array_merge($existing_attributes , $attributes));
    

    Minor: extra space before comma in array_merge().

  2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -222,13 +222,20 @@ public function getUrlFromPath($path, $options = array()) {
    diff --git a/core/modules/system/src/Tests/Theme/EngineTwigTest.php b/core/modules/system/src/Tests/Theme/EngineTwigTest.php
    
    diff --git a/core/modules/system/src/Tests/Theme/EngineTwigTest.php b/core/modules/system/src/Tests/Theme/EngineTwigTest.php
    old mode 100644
    new mode 100755
    
    +++ b/core/modules/system/src/Tests/Theme/EngineTwigTest.php
    @@ -77,10 +77,14 @@ public function testTwigUrlGenerator() {
    diff --git a/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php b/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php
    
    diff --git a/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php b/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php
    old mode 100644
    new mode 100755
    

    These mode change shouldn't be here.

  3. +++ b/core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.link_generator.html.twig
    @@ -1 +1,5 @@
    +<div>link via the linkgenerator: {{ link("register", test_url, { 'foo' : 'bar' }) }}</div>
    +<div>link via the linkgenerator: {{ link("register", test_url_attribute, { 'id' : 'kitten' }) }}</div>
    

    Minor: Shouldn't be a space before the : per Twig coding standards: http://twig.sensiolabs.org/doc/coding_standards.html

  4. +++ b/core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.link_generator.html.twig
    @@ -1 +1,5 @@
    +
    

    Minor: Extra blank line added to the end of this Twig file is not needed.

star-szr’s picture

Assigned: Unassigned » star-szr

I didn't really register that the tests were failing before going through the patch. Since we have to fix the tests and 2 rounds of nitpicks I will work on this :)

star-szr’s picture

Status: Needs work » Needs review
FileSize
4.57 KB

New reroll, thanks for the efforts @wesruv :)

Then I will work on the nits.

star-szr’s picture

Assigned: star-szr » Unassigned
FileSize
4.57 KB
1.73 KB

Basically most of the nits were from the previous reroll. No worries @wesruv it's all part of learning! I like to diff the pre-reroll patch and my reroll to make sure I'm on the right track.

Also I still found another small tweak besides my #39.3 :)

Status: Needs review » Needs work

The last submitted patch, 42: allow_twig_link-2342745-42.patch, failed testing.

star-szr’s picture

Ran the previous failing test locally and it still fails so I cancelled my last patch and am working on it still.

The last submitted patch, 41: allow_twig_link-2342745-41.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
FileSize
4.66 KB
3.53 KB

As of #2335661: Outbound path & route processors must specify cacheability metadata url() in Twig actually returns a render array, not a string, so we need to pipe it through the render filter when using it as an argument. Not sure how I feel about this but it works for now :/

Overall there were bugs in both the tests and the implementation so this needs another good review for sure. In short:

  • Attributes would previously only be set for URLs passed in as strings
  • Some of the tests did not use the 'attributes' key of the Url options array

I also changed another coding standards thing in the test Twig file, besides the |render.

joelpittet’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.link_generator.html.twig
@@ -1 +1,4 @@
+<div>link via the linkgenerator: {{ link("register", url('user.register')|render, {'id': 'kitten'}) }}</div>

Boo-urns to |render for this to work

star-szr’s picture

Status: Needs work » Needs review
FileSize
4.67 KB
3.53 KB

One more tweak to avoid unnecessary array_merge()ing. More better.

Interdiff is from #42.

star-szr’s picture

@joelpittet please create a follow-up to #2335661: Outbound path & route processors must specify cacheability metadata to address that, it's not this issue's fault! :(

lauriii’s picture

Status: Needs review » Needs work

Setting back to NW per #47

lauriii’s picture

Status: Needs work » Needs review

Back to NR after short discussion with Cottser and Joel. |render problem needs to be fixed in a follow-up.

star-szr’s picture

…and we can also just remove the |render and just pass a string of /user/register :)

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

@Cottsers lates patch looks good to me. Lets create follow-up for that |render thing because it doesn't seem to be very logical in this use case.

Fabianx’s picture

Yes, RTBC + 1, link() function should understand a url render array ...

Link / Url generation is still a whole mess in core and latest changes even make it more complicated.

We tried to 'shield' the theme system from it though, hence returning a render array, but that shows that my original idea of either having toString() go via the renderer or have a toRenderableArray() was better ...

Wim Leers’s picture

Just pass in a URI? i.e.:

+++ b/core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.link_generator.html.twig
@@ -1,4 +1,4 @@
+<div>link via the linkgenerator: {{ link("register", url('user.register')|render, {'id': 'kitten'}) }}</div>

change that to:

link("register", "route:user.register", {'id': 'kitten'})

Also, passing the results of one Twig function to another Twig function seems a relative edge case. Since Twig is about rendering, it IMO is natural for Twig functions to return render arrays, especially if they're doing things that depend on other parts of the system, and therefore need to be able to bubble cacheability metadata. If they don't return on external things, then Twig functions returning plain strings makes sense, of course.


Finally, the usage of both single and double quotes on a single line looks weird.

star-szr’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.71 KB
2.36 KB

I like it, definitely didn't know you could do that. Thanks @Wim Leers!

(and agreed about the quote consistency, fixed.)

star-szr’s picture

Issue summary: View changes
Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

anavarre’s picture

+++ b/core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.link_generator.html.twig
@@ -1,4 +1,4 @@
+<div>link via the linkgenerator: {{ link('register', test_url_attribute, {'id': 'kitten'} ) }}</div>

Sorry for the nit: the closing link() parenthesis has an extra whitespace.

Could be fixed on commit.

star-szr’s picture

@anavarre no need to apologize, thanks for catching that!

star-szr’s picture

Title: Allow twig link generator to pass in attributes. » Allow Twig link function to pass in HTML attributes

Title tweak.

alexpott’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Committed e285124 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed e285124 on 8.0.x
    Issue #2342745 by Cottser, chr.fritsch, epari.siva, lauriii, wesruv,...

Status: Fixed » Closed (fixed)

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

star-szr’s picture