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.
Files: 
CommentFileSizeAuthor
#60 interdiff.txt946 bytesCottser
#60 allow_twig_link-2342745-60.patch4.71 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,827 pass(es). View
#56 interdiff.txt2.36 KBCottser
#56 allow_twig_link-2342745-56.patch4.71 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,507 pass(es). View
#48 interdiff-42-47.txt3.53 KBCottser
#48 allow_twig_link-2342745-47.patch4.67 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,505 pass(es). View
#46 interdiff.txt3.53 KBCottser
#46 allow_twig_link-2342745-46.patch4.66 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,430 pass(es). View
#42 interdiff.txt1.73 KBCottser
#42 allow_twig_link-2342745-42.patch4.57 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#41 allow_twig_link-2342745-41.patch4.57 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,414 pass(es), 6 fail(s), and 5 exception(s). View
#36 allow_twig_link-2342745-36.patch4.81 KBwesruv
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,416 pass(es), 6 fail(s), and 5 exception(s). View
#28 allow_twig_link-2342745-28.patch4.5 KBepari.siva
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,846 pass(es). View
#28 interdiff-22-28.txt1.36 KBepari.siva
#22 allow_twig_link-2342745-22.patch4.5 KBepari.siva
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,833 pass(es). View
#22 interdiff-20-22.txt1.03 KBepari.siva
#20 allow_twig_link-2342745-20.patch4.51 KBepari.siva
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,629 pass(es). View
#20 interdiff-18-20.txt1.74 KBepari.siva
#18 allow_twig_link-2342745-18.patch4.5 KBepari.siva
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,636 pass(es), 1 fail(s), and 0 exception(s). View
#16 interdiff-2342745-13-15.txt2.53 KBlauriii
#16 drupal-add_attributes_to_twig_link-2342745-15.patch4.63 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,493 pass(es). View

Comments

Cottser’s picture

chr.fritsch’s picture

FileSize
874 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,727 pass(es), 2 fail(s), and 0 exception(s). View

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
Cottser’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

FileSize
2.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-add_attributes_to_twig_link-2342745-6.patch. Unable to apply patch. See the log in the details link for more information. View

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
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,747 pass(es), 2 fail(s), and 0 exception(s). View

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

Status: Needs work » Needs review
FileSize
2.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,747 pass(es). View

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

FileSize
2.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,530 pass(es). View
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
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,493 pass(es). View
2.53 KB

I love writing tests!

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
epari.siva’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,636 pass(es), 1 fail(s), and 0 exception(s). View

Patch rerolled.

Status: Needs review » Needs work

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

epari.siva’s picture

Status: Needs work » Needs review
FileSize
1.74 KB
4.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,629 pass(es). View

Small fixes.

Cottser’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.

epari.siva’s picture

Status: Needs work » Needs review
FileSize
1.03 KB
4.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,833 pass(es). View

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.

Cottser’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.

epari.siva’s picture

Status: Needs work » Needs review
FileSize
1.36 KB
4.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,846 pass(es). View

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)

Cottser’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

FileSize
4.81 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,416 pass(es), 6 fail(s), and 5 exception(s). View

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.

Cottser’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.

Cottser’s picture

Assigned: Unassigned » Cottser

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 :)

Cottser’s picture

Status: Needs work » Needs review
FileSize
4.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,414 pass(es), 6 fail(s), and 5 exception(s). View

New reroll, thanks for the efforts @wesruv :)

Then I will work on the nits.

Cottser’s picture

Assigned: Cottser » Unassigned
FileSize
4.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
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.

Cottser’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.

Cottser’s picture

Status: Needs work » Needs review
FileSize
4.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,430 pass(es). View
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

Cottser’s picture

Status: Needs work » Needs review
FileSize
4.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,505 pass(es). View
3.53 KB

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

Interdiff is from #42.

Cottser’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.

Cottser’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.

Cottser’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,507 pass(es). View
2.36 KB

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

(and agreed about the quote consistency, fixed.)

Cottser’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.

Cottser’s picture

FileSize
4.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,827 pass(es). View
946 bytes

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

Cottser’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.

Cottser’s picture