Proposed commit message:
Issue #1922454 by thedavidmeister, steveoliver, Cottser, Fabianx: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig.
For future reviewers/contributors, the scope of the patch in this issue covers:
* Converting theme_link() into a theme template as per the core efforts to convert all theme functions in this way.
* Removing the ability for l() to conditionally call theme_link() based on some internal logic that cannot be controlled by code calling l().
* Mirroring testbot coverage for links themed with default templates to match the coverage we have for l().
* Exposing url_is_active() logic (not changing it).
* Adding an alter to re-implement some of the flexibility/alterability we're removing from l().
Currently there is some discussion about the best way to use l() and url() in twig templatesand there is work being done towards converting all the existing core modules to use templates . The latter will sooner or later be bottlenecked/blocked by the lack of decisive action across the related but slightly different issues referenced in the former.
The question causing problems isbut it appears to be jumping the gun a bit in that, with the current tools we have there is no "best" way to render links, only three different but equally broken methods.
There are three main propositions for what has been suggested as potential "best practise" and their discussed limitations:
- Put the
<a>tag directly in the template and generate a url for the href attribute in the preprocess function. This keeps things simple and arguably more readable in the template than the alternatives.
- There is no way currently for the "active" class to be generated for an HTML tag in a Twig template, in theory we could try to do something in the preprocess to generate attributes but in practise this would probably get messy trying to keep things consistent and performant while avoiding code duplication. Without the "active" class, many existing tests will fail, see comment #4 on for what can only be the tip of the iceberg.
- Use l() in the preprocess function wherever possible to generate a rendered string and pass it to the template, allow l() to be used in Twig templates as a function. l() is the fastest way we have of producing a consistently formatted string with all required sanitisation and attribute/class handling to pass tests, it's also the way we've always handled themed links in core so would require the least work to implement.
- If the contents of the link is more complex than plain text, especially if the contents of the link is desired to be a render array all the way to the template, using l() is severely limiting inside the preprocess function.
- To use l() in a Twig template when render arrays are passed into the template, l() will need to be extended (made as slow as theme('link')) to understand them, making it equivalent to theme('link') but with uglier, less "twigy" syntax.
- l() currently can "decide" whether or not to make itself theme-able via theme('link') depending on a system setting and/or whether theme override files currently exist on the system. This means that if a site builder wants to make even *one* link on their site themable, they need to accept the lower performance version of l() for all links. It really isn't obvious to anyone who hasn't read the source code of l() that this is the case.
- From what I've read in #drupal-twig in IRC, using a theme function, or a function that calls a theme function inside a Twig template is pretty frowned upon.
Use theme('link') alongside render arrays to produce link markup in Twig templates. This guarantees extensibility and consistency of all links across the board, it also allows a developer to wrap a render array in a link and preserve the data structure through to the template.
- Using a theme function in Drupal 7 vs. calling l() was clocked at ~20% slower in Drupal 7 during preliminary benchmarks, with link-heavy sites seeing an overall performance hit of ~2% on a page load. How much slower is a Drupal 8 Twig template + preprocess than a Drupal 7 theme function? well nobody knows yet, or at least nobodys published anything useful anywhere I've seen... Let's just say "theme('link') is slow and l() is fast".
- Currently theme('link') doesn't actually work, in that it *assumes that it will be called by l() every time*. Therefore it currently does no sanitisation of urls or processing/translation of the "active" class. It simply isn't useful in it's present state .
- Since this is not the way Drupal currently handles links anywhere, it would probably involve the most work to implement across the board.
There's also the idea of creating something new entirely, a renderable *object* (not array) as an equivalent to l(). This is inline with a general movement towards renderable objects but has only had very preliminary work done. Additionally Fabianx told me in IRC that this would be the slowest option by far, although Sun in and Steveoliver in suggested that it might not be so bad.
Setting aside renderable objects until they mature a bit, while making literally everything in Drupal renderable and themable with arrays might be "cool" we do have to be mindful of performance since Drupal doesn't really have room to move on that front right now and links in particular are very common page elements so a small change here can make a big difference if applied across the board.
Allow for all three options by making l() and theme_link() both more viable solutions but provide as clear instruction to themers and developers as possible as to when/how each should be used. By looking at all the duplicate and postponed issues around this topic I think it's pretty clear that a one-size-fits-all approach won't work here, or at least there are no clear proposals outlining what it would look like.
<a>tags should be used by themers when the sanitised url is available in the Twig template and readability is important
<a>tags should not be used when the "active" class could feasibly aid navigation or improve accessibility, or when we cannot guarantee the url has been sanitised beforehand
<a>tags will probably have limited viable use cases in core where a call to l() would not be a better option, unless we know in advance that the template appears in only well defined and predictable places and those places are not "navigation" links. This technique will likely appeal to site builders and themers.
- the l() function should be used wherever the theme system provides excessive flexibility and is likely to degrade performance through many repeat calls per page load, Sun suggested menu links as an example of this.
- the l() function should not be used anywhere that either that theming the contents of the link or the link markup itself may be desirable, in this case build a render array which will handled by theme('link'). Avoid using l() inside a preprocess function for wrapping anything more complex than a plain text string.
- In the case that l() is being used inside a preprocess function, developers should provide the unlinked content and the sanitised url to themers as variables in the template so as not to subtly but critically undermine the usefulness of the template. If this seems like overkill for the current template, consider bypassing the theme system altogether and using l() directly or restructuring what you're trying to achieve.
- theme('link') should not be called directly, use l() or a render array (which will in turn call theme('link') when required).
- If in doubt between locking markup with l() or providing extensibility, opt for a render array. We want to avoid premature optimisation, especially if it comes at the cost of consistency. If you're suspicious of a heavy theme('link') call, try to prototype a site and profile the impact.
- In the case that flexibility for existing l() usage is required, use hook_l_alter() to modify $text, $path, $options.
Some of the things in the proposed resolution don't currently work/exist, as well as deciding whether this is how we want to move forward we'd have to make these things work.
- Add drupal_alter('l') to l() and theme_link() - Couldn't see an existing issue for this, was suggested originally by Fabinx in IRC.
- Don't let l() try to "get smart" with theme('link'), make it fast and dumb like it used to be , .
- Once l() is "dumb", expose it to Twig templates as a function. If it can't call theme functions there's no reason not to allow themers access to the .active class and extra sanitisation provided by l().
- Make theme_link() able to render a link at least as well as l() currently can
- Let people know that this is how we want to move forward
- Clean up all the weirdly similar theme_X_link() functions in core to be either a flat l() or a template
- Close this as "won't fix"
User interface changes
- Developers will need to make a conscious decision as to whether the content or context of a link justifies a fully themable element or whether it would be better as the traditional and faster l() call.
- There will be a new alter hook for modifying the structure (not the markup) of all links whether generated by l() or through templates.
- theme('link') will be useful.
- l() will no longer invoke theme functions.
- l() will be available from within Twig templates.
|PASSED: [[SimpleTest]]: [MySQL] 55,635 pass(es).|
|PASSED: [[SimpleTest]]: [MySQL] 55,460 pass(es).|
|PASSED: [[SimpleTest]]: [MySQL] 55,417 pass(es).|