We're coming across a lot of templates that call the l() function, and in some cases I think it can be replaced with an actual anchor tag.
The more-help-link example:
http://api.drupal.org/api/drupal/core!includes!theme.inc/function/theme_...
<div class="more-help-link">' . <a href="{{ url(path) }}" target="_blank">{{ More help|t }}</a></div>
The filter-tips-more-info example:
http://api.drupal.org/api/drupal/core!includes!theme.inc/function/theme_...
<p>{{ <a href="{{ url('filter/tips') }}" target="_blank">{{ 'More information about text formats'|t }}</p>
Does anyone have any strong feelings about this? Will it adversely affect performance?
And more importantly, should we create some best practices about when to use an anchor tag with url() and when to use l()? I was thinking:
- If it will not adversely affect performance
- If it greatly improves readability of the file
Assigning to effulgentsia since I think he knows the most about what has been discussed before about the use of l() and url() in templates, and how that may affect performance :)
Related Issues
This issue is complicated because opinions on the "best" way(s) to expose the functionality currently provided exclusively by l() are still being voiced. It's therefore hard to know what the "best" way to implement l()-like behaviour in twig templates should be as the intended role of the l() function itself within Drupal may change.
- #1778610: Remove the check for a link template from l(), have l() always output just a string.
- #318636: Make l() themable
- #1187032: theme_link needs to be clearer about saying not to call it directly
- #1833920: [META] Markup Utility Functions
- #1778616: Add an .active class for links rendered by theme('link') that have an href to the URL being viewed and sanitize output of url()
- #1836730: Add a renderable object that is equivalent to l()
- #1595614: [meta] Remove all the theme functions and templates in core that simply output a link. Replace with #type 'link' render arrays
- #1898420: image.module - Convert theme_ functions to Twig
- #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig
- #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links
Comments
Comment #1
ishmael-sanchez CreditAttribution: ishmael-sanchez commentedAnother example:
Comment #2
anthonyR CreditAttribution: anthonyR commentedl() also does some stuff like adding an
.active
class when the current path is the href of the link tag.In lots of links (and in the examples) this probably won't be a issue though.
Comment #3
Fabianx CreditAttribution: Fabianx commentedvariables.url needs to be preprocessed to a "resolved" / absolute url for this to work.
Besides that, changing to
<a href
markup should be fine.Similar to if url is needed in templates.
(No, it isn't)Thinking about url() in templates again:
Hm, it might be needed. Consider I want to link to the front-page and my site is in /d8-twig/
is obviously wrong while
will only work in a production environment.
However
will work everywhere.
Comment #4
markabur CreditAttribution: markabur commentedThinking about #2, would
.active
and other classes be present invariables.attributes.class
?Comment #5
jenlamptonI agree that url() might still be necessary. If you wanted to add a query string for a lightbox, for example, someone might want to do that from a template rather than preprocess. But I think the l() function might be going too far, and most front-end devs would prefer to work with an anchor tag anyway.
I propose that we allow use of the l function in templates, just incase people want to use it, but as a general rule we try not to use it, except where it would adversely affect performance to do so. In the two examples below, I don't think it will adversely affect performance, but in the case of menu items it may.
@markabur & @anthonyR yes, the active class would still be available in attributes where the link actually has attributes. In the two examples below we don't happen to have any :)
Let's go back to the more-help-link example:
Now let's look at the filter-tips-more-info example:
How do people feel about these examples?
Comment #5.0
jenlamptonadded code samples for two templates
Comment #6
thedavidmeister CreditAttribution: thedavidmeister commented@jenlampton - how would the active class be generated if l() isn't used? $is_active is determined by l() - http://api.drupal.org/api/drupal/core%21includes%21common.inc/function/l/8
Edit: The "active" class definitely doesn't seem to be coming from the {{ attributes }}, causing tests to fail for the work I'm doing at #1898420: image.module - Convert theme_ functions to Twig
Comment #7
steveoliver CreditAttribution: steveoliver commentedI see no reason why l() should not be allowed within templates. url() is already available. A full link is a fine thing to be able to generate from a template (especially when we #1778610: Remove the check for a link template from l(), have l() always output just a string.), and the .active class added by l() is helpful for themeing and required for tests like in #1898420: image.module - Convert theme_ functions to Twig.
When you should use l():
When you want a complete link generated
and/or
You want an active class included in the link attributes.
When you should use url():
When you want to construct your own
<a>
tag.and/or
You want to modify the classes or other attributes of the link itself.
* This means you will not get /any/ attributes--you will only get the url.
These are kinda crude options, but are really all we have at the moment. Later, like D9, we'll have more intelligent and utilitous renderable objects to work with. For now both these callbacks should be available in templates. I'll open a core issue.
Comment #8
Fabianx CreditAttribution: Fabianx commentedWell, there is two things about this:
a) if l() is doing something that theme_link is not, then that is fundamentally wrong and the active class processing should be in a separate function.
Second this brings an interesting point that attributes are not aware to what they belong.
b) but keeping l() for performance reasons might make sense, though it makes templates much less twiggy ...
Comment #9
thedavidmeister CreditAttribution: thedavidmeister commentedJust some more contextual information about where things currently stand.
From the documentation for theme_link() - http://api.drupal.org/api/drupal/core%21includes%21theme.inc/function/th...
That "initial preprocessing" = add active class inline (ie. not through some is_active() function call) + strip tags from "title". Not only that but l(), by design it seems, can be configured to force theme_link() to never be called by setting
config('system.performance')->get('theme_link')
to false, so always using theme_link() instead of l() would be considered a regression, see #318636: Make l() themable and #1187032: theme_link needs to be clearer about saying not to call it directly.l() is doing quite a bit that theme_link() isn't, as an indication l() is about 60 lines of code whereas theme_link() is just 3 lines long.
From inside l():
Comment #10
thedavidmeister CreditAttribution: thedavidmeister commentedOne solution might be to extend template_preprocess() and create a new attributes variable tailored for links.
We currently have:
We could add 'link_attributes' in there which is $default_attributes + the active class if required?
Edit: Disregard this suggestion. It's actually a bad idea.
Comment #11
thedavidmeister CreditAttribution: thedavidmeister commentedAlso quite relevant #1833920: [META] Markup Utility Functions.
Comment #12
thedavidmeister CreditAttribution: thedavidmeister commentedAnd #1778616: Add an .active class for links rendered by theme('link') that have an href to the URL being viewed and sanitize output of url().
Comment #13
thedavidmeister CreditAttribution: thedavidmeister commentedLol, how many Drupal issues does it take to render a link? #1836730: Add a renderable object that is equivalent to l() There's more, higher level discussion around renderable objects as a concept in general but this is the bit that relates to l().
Comment #13.0
thedavidmeister CreditAttribution: thedavidmeister commentedcleanup
Comment #14
jenlamptonI'd also like to add - for the record - that we have zero use cases for actually needing to theme override links. Having a theme_link() at all goes against one of our principles: build for use cases. theme_link() was added as a "what if at some point someone wanted it" and I have not yet heard one good reason for keeping it around.
I still vote to kill theme_link eventually. We should either use l() instead, or write the anchor tag ourselves by printing the innards as follows:
<a href="{{ link.href }}" class="{{ link.attributes.class }}"{{ link.attributes }}>{{ link.content }}</a>
.This will work especially nicely if we can get a link to be a renderable thingy :)
However, since I don't want the move to Twig to be blocked by or inability to decide what to do with l()...
Can we expose l() as a function within templates? In preprocess, we can prepare $variables['link'] as an array with three or four parts: href, content, attributes, and options. Then we can call l() in our templates using these parameters.
Later on, we can decide how to make a $variables['link'] do the calling of l() for us within Twig (and/or add in the missing attributes where we need them, so we can continue to write links like the code sample above).
Comment #15
thedavidmeister CreditAttribution: thedavidmeister commented#14 - At some point I was thinking along exactly the same lines, I didn't think it would be accepted as a final solution because "content" may be a string or it may be a renderable array/object. I don't know enough about the way Twig works to be sure about this but on the face of it, doesn't that mean that l() would now need to be extended to understand what to do with $content other than a string?
At the least this would mean where we do this on the final line
($options['html'] ? $text : check_plain($text))
we'd have to do this instead($options['html'] || is_array($text) ? render($text) : check_plain($text))
. At this point we can no longer guarantee that l() is fast because render() is not guaranteed to be fast :/I got the impression from the various threads around l() that any proposed solution where l() is not guaranteed-fast would be a hard sell.
Also, in the "markup utility function" discussion, is it ok for a markup utility function to call render()? I had thought that MUFs would only deal in string manipulation and option-style-arrays.
This of course is all moot if Twig calls render() on the variables sent through to l() from templates as l() can just keep dealing with strings, but that seems like weird behaviour to me.
These questions need further explanation/discussion before I'd be happy relying exclusively on l() in templates.
I personally liked your idea in comment #9 of #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig to teach drupal_render() about markup utility functions, or at least "render callbacks" so we could say something similar to '#render' => 'l' instead of '#theme' => 'link' because this just sidesteps every problem raised so far by allowing MUFs to always be as fast as possible while still being compatible with renderable arrays.
Comment #15.0
thedavidmeister CreditAttribution: thedavidmeister commentedAdding related issues to issue summary
Comment #16
Fabianx CreditAttribution: Fabianx commentedI think we are better off with custom Twig syntax here for our markup utility functions. For example think of a "link" filter:
or
I personally think that this would be a viable solution and a good compromise for the MUAs. It is also nicer and more twiggy than l() and would be compatible if we chose to use the {% trans %} way. The filter would call template_preprocess_link, such "active" class etc. would be there.
Best,
Fabian
Comment #17
thedavidmeister CreditAttribution: thedavidmeister commented#16 - I'm a little confused by the first example. I thought filters needed a pipe symbol, eg:
{{ my_link | link }}
Regardless of the syntax, or even whether we use functions or filters we'd still need to put render() inside l() to actually implement this wouldn't we? or is this proposed "link" filter something renderable in parallel to l(), because that could work - how does it differ from theme_link()?
Comment #18
Fabianx CreditAttribution: Fabianx commented#17: This assumes that {{ my_link }} is a #theme=>'link' render array already.
The link filter would just do something like:
This was just an idea to make working with render arrays easier.
Comment #19
thedavidmeister CreditAttribution: thedavidmeister commented#18 - seems fair. I think at some point we need to make a summary of all the proposed solutions that are popping up across the different l() threads.
Comment #19.0
thedavidmeister CreditAttribution: thedavidmeister commentedAdded a related issue.
Comment #20
thedavidmeister CreditAttribution: thedavidmeister commentedThere may be light at the end of the tunnel for the .active class problem #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links.
Also, theme_link() is now useable and supports renderable arrays being passed in as content #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig.
Comment #20.0
thedavidmeister CreditAttribution: thedavidmeister commentedAdded a relevant issue
Comment #21
klonosThis issue here is marked as NW, but it hasn't been updated in 10 years, and there is no project associated with it. I'm assigning it to Coding Standards.
the component may need to be switched to "Documentation", but not 100% sure about that, so leaving as "Drupal Core Standards" for the time.
Comment #22
quietone CreditAttribution: quietone at PreviousNext commented@jenlampton, thanks for suggesting an improvement to our coding standards. And to everyone here who worked to resolve this.
I didn't know there could be issues without a project.
I gather from the issue summary that this is about Drupal 7 and has not been worked on for 11 years. Since Drupal 7 will be EOL in less than a year, we can be confident that there will not be any coding standard changes specific to that version of Drupal. Therefore I am closing this as a won't fix.