Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
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. |
Comment | File | Size | Author |
---|---|---|---|
#60 | interdiff.txt | 946 bytes | star-szr |
#60 | allow_twig_link-2342745-60.patch | 4.71 KB | star-szr |
#56 | interdiff.txt | 2.36 KB | star-szr |
#56 | allow_twig_link-2342745-56.patch | 4.71 KB | star-szr |
#48 | interdiff-42-47.txt | 3.53 KB | star-szr |
Comments
Comment #1
star-szrComment #2
chr.fritschNot sure how to handle link_from_url. Couldn't find a twig function with that name.
Comment #3
chr.fritschComment #4
star-szrIt's just called 'link' I think so should be fine. Patch looks reasonable, this needs some test coverage. Thanks @chr.fritsch!
Comment #6
chr.fritschOk, i added one test and fixed the method doc
Comment #7
chr.fritschComment #9
chr.fritschBuild patch against HEAD
Comment #11
chr.fritschDont overwrite existing attributes
Comment #12
dawehnerDid you considered to use
(if (!is_null())
which is exactly what Url::getOption returns?We do use two spaces instead of 4 spaces :(
Meh, so people might always use relative paths from Drupal. Do we let themers on the longrun not use paths anymore? Just curious here.
Comment #13
chr.fritschComment #14
dawehnerThis is already a good progress ...
Comment #15
alexpottWe only have test coverage where the url does not have existing attributes.
We have no test coverage of this.
Comment #16
lauriiiI love writing tests!
Comment #17
star-szrComment #18
siva_epari CreditAttribution: siva_epari commentedPatch rerolled.
Comment #20
siva_epari CreditAttribution: siva_epari commentedSmall fixes.
Comment #21
star-szrThank you @epari.siva!
We could use short array syntax
[]
here. And might be good to type hint $attributes in the function signature:Wasn't introduced with the reroll but there is an extra space before the last comma here.
Comment #22
siva_epari CreditAttribution: siva_epari commentedFixed as per suggestions.
Comment #23
joelpittetMore kitten tests!
Comment #25
joelpittetDrunk testbot.
Comment #26
star-szrAdded beta evaluation.
Comment #27
alexpottMissing 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?
This is an unnecessary new line. The file already has one.
Comment #28
siva_epari CreditAttribution: siva_epari commentedChanges applied.
Comment #29
joelpittetBack to RTBC.
@alexpott I'm not sure where we are either. (/me guilty of poor documenting)
Comment #30
star-szrThere's this page for filters: https://www.drupal.org/node/2357633
We could add a similar one for functions.
Comment #31
xjmI'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".
Comment #32
joelpittet@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:
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
andArrayIterator
interface toAttribute
, to make it easier to work with as an array.http://php.net/manual/en/arrayiterator.getarraycopy.php
Comment #33
joelpittetOr slightly better?
Comment #34
joelpittetMoving to major because it blocks a major #2345881: Refactor pager.html.twig to use the link generator and needs a re-roll.
Comment #35
wesruv CreditAttribution: wesruv commentedGoing to reroll this like a boss.
Comment #36
wesruv CreditAttribution: wesruv commentedConsider this re-rolled (probably)
Comment #38
lauriiiThanks 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:
Short array syntax could be used: $attributes = [].
Same here :)
This has to end for a dot.
Comment #39
star-szrMore nits! And the mode changes definitely need to be fixed.
Minor: extra space before comma in array_merge().
These mode change shouldn't be here.
Minor: Shouldn't be a space before the : per Twig coding standards: http://twig.sensiolabs.org/doc/coding_standards.html
Minor: Extra blank line added to the end of this Twig file is not needed.
Comment #40
star-szrI 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 :)
Comment #41
star-szrNew reroll, thanks for the efforts @wesruv :)
Then I will work on the nits.
Comment #42
star-szrBasically 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 :)
Comment #44
star-szrRan the previous failing test locally and it still fails so I cancelled my last patch and am working on it still.
Comment #46
star-szrAs 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:
I also changed another coding standards thing in the test Twig file, besides the |render.
Comment #47
joelpittetBoo-urns to
|render
for this to workComment #48
star-szrOne more tweak to avoid unnecessary array_merge()ing. More better.
Interdiff is from #42.
Comment #49
star-szr@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! :(
Comment #50
lauriiiSetting back to NW per #47
Comment #51
lauriiiBack to NR after short discussion with Cottser and Joel. |render problem needs to be fixed in a follow-up.
Comment #52
star-szr…and we can also just remove the |render and just pass a string of
/user/register
:)Comment #53
lauriii@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.
Comment #54
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedYes, 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 ...
Comment #55
Wim LeersJust pass in a URI? i.e.:
change that to:
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.
Comment #56
star-szrI like it, definitely didn't know you could do that. Thanks @Wim Leers!
(and agreed about the quote consistency, fixed.)
Comment #57
star-szrComment #58
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedBack to RTBC
Comment #59
anavarreSorry for the nit: the closing link() parenthesis has an extra whitespace.
Could be fixed on commit.
Comment #60
star-szr@anavarre no need to apologize, thanks for catching that!
Comment #61
star-szrTitle tweak.
Comment #62
alexpottCommitted e285124 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.
Comment #65
star-szrInteresting bit of follow up: #2506151: Make the Twig extension link() accept Attribute objects