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:

  1. If it will not adversely affect performance
  2. 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.

Comments

ishmael-sanchez’s picture

Another example:

function theme_filter_tips_more_info() {
  return '<p>' . l(t('More information about text formats'), 'filter/tips', array('attributes' => array('target' => '_blank'))) . '</p>';
anthonyR’s picture

l() 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.

Fabianx’s picture

variables.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/

<a href="/d8-twig/" >Home</a>

is obviously wrong while

<a href="/" >Home</a>

will only work in a production environment.

However

<a href="{{ url('<front>') }}" >Home</a>

will work everywhere.

markabur’s picture

Thinking about #2, would .active and other classes be present in variables.attributes.class?

jenlampton’s picture

Title: Create best practices about use of the l() function (more-help-link.html.twig) » Create best practices about use of the l() function and the url() function in templates
Issue tags: +Coding standards, +Twig

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

<div class="more-help-link">' . <a href="{{ url(path) }}" target="_blank">{{ More help|t }}</a></div>

Now let's look at the filter-tips-more-info example:

<p>{{ <a href="{{ url('filter/tips') }}" target="_blank">{{ 'More information about text formats'|t }}</p>

How do people feel about these examples?

jenlampton’s picture

Issue summary: View changes

added code samples for two templates

thedavidmeister’s picture

Component: Twig templates » Twig templates conversion (front-end branch)

@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

steveoliver’s picture

Status: Active » Needs work

I 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.

Fabianx’s picture

Well, 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 ...

thedavidmeister’s picture

Just 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...

All Drupal code that outputs a link should call the l() function. That function performs some initial preprocessing, and then, if necessary, calls theme('link') for rendering the anchor tag.

To optimize performance for sites that don't need custom theming of links, the l() function includes an inline copy of this function, and uses that copy if none of the enabled modules or the active theme implement any preprocess or process functions or override this theme implementation.

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

  // Determine if rendering of the link is to be done with a theme function
  // or the inline default. Inline is faster, but if the theme system has been
  // loaded and a module or theme implements a preprocess or process function
  // or overrides the theme_link() function, then invoke theme(). Preliminary
  // benchmarks indicate that invoking theme() can slow down the l() function
  // by 20% or more, and that some of the link-heavy Drupal pages spend more
  // than 10% of the total page request time in the l() function.
thedavidmeister’s picture

One solution might be to extend template_preprocess() and create a new attributes variable tailored for links.

We currently have:

  $variables += $default_variables + array(
    'attributes' => clone $default_attributes,
    'title_attributes' => clone $default_attributes,
    'content_attributes' => clone $default_attributes,
  );

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.

thedavidmeister’s picture

thedavidmeister’s picture

Lol, 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().

thedavidmeister’s picture

Issue summary: View changes

cleanup

jenlampton’s picture

Assigned: effulgentsia » Unassigned

I'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).

thedavidmeister’s picture

#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.

thedavidmeister’s picture

Issue summary: View changes

Adding related issues to issue summary

Fabianx’s picture

I think we are better off with custom Twig syntax here for our markup utility functions. For example think of a "link" filter:

{{ my_link }}

or

{% filter link(my_link) %}
<a href="{{ url }}"{{ attributes }}>{{ text }}</a>
{% end filter %}

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

thedavidmeister’s picture

#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()?

Fabianx’s picture

#17: This assumes that {{ my_link }} is a #theme=>'link' render array already.

The link filter would just do something like:

// get parts of link
// create internal variables
// process variables via template_preprocess_link and other preprocessors
// Allow usage of vars within "subtemplate"

This was just an idea to make working with render arrays easier.

thedavidmeister’s picture

#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.

thedavidmeister’s picture

Issue summary: View changes

Added a related issue.

thedavidmeister’s picture

thedavidmeister’s picture

Issue summary: View changes

Added a relevant issue

klonos’s picture

Project: » Coding Standards
Component: Twig templates conversion (front-end branch) » Drupal Core Standards
Issue summary: View changes

This 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.

quietone’s picture

Status: Needs work » Closed (won't fix)

@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.