I'd like to add a preprocess function for link, so that we can remove the call to url() from the template file (I asked the theme devs we had at DrupalCon Munich, and they all said they didn't ever use any of the parameters passed to url as part of their theming at the template level) . Unfortunately the way things are now, if a preprocess function exists for link then the link template gets called every time someone calls the l() function.
I'd like to keep the use of l - the way we have it now - where this link template doesn't ever get called until needed. Does that mean we need to keep url in the template file, or can we remove the check for a preprocess function from the decision to use a template over l()?
Related issues
#1696786: Integrate Twig into core: Implementation issue
#1804614: [meta] Consolidate theme functions and properly use theme suggestions in core
Comment | File | Size | Author |
---|---|---|---|
#22 | drupal-l-theme-link-1778610-22.patch | 12.33 KB | steveoliver |
#19 | drupal-l-theme-link-1778610-15.patch | 5 KB | steveoliver |
#16 | drupal-l-theme-link-1778610-14.patch | 4.71 KB | steveoliver |
#12 | drupal8.l-theme.12.patch | 3.33 KB | sun |
#10 | drupal8.l-theme.10.patch | 2.51 KB | sun |
Comments
Comment #1
steveoliver CreditAttribution: steveoliver commentedOn a more general note, a preprocess for
theme('link')
is exactly what we need in order to use it in place oftheme('toolbar_toggle')
for the toolbar toggle arrow in (#1779104: Convert theme_toolbar_toggle to twig). Attached adds the preprocess that works so far.Comment #2
Fabianx CreditAttribution: Fabianx commentedThis has performance implications.
Lets leave this for now, toolbar can explicitly set the Attributes as new Attribute(...). The HTML support will work with theme_link:
{{ html?text:(text|e) }} for now, such the preprocess is not needed currently and this issue can be worked on.
Comment #3
jenlamptonWe just restructured the l() function. Major changes are 1) create attributes and pass them into the template file separately from options. 2) call url() and pass the result into the theme function as a variable named 'url' instead of it's components of 'path' and 'options'. 3) not to pass options into the template file at all.
New code is as follows:
This removes the need for preprocess, hopefully avoiding performance implications, and still keeping our template clean and simple without removing any functionality.
Twig code:
Comment #4
Fabianx CreditAttribution: Fabianx commentedLooks good, re-adding performance tag.
For comparison:
http://api.drupal.org/api/drupal/core%21includes%21common.inc/function/l/8
Comment #5
sunThat looks more acceptable, thanks. Now, who rolls a patch for this issue?
Comment #6
steveoliver CreditAttribution: steveoliver commentedI'll roll a patch
Comment #7
steveoliver CreditAttribution: steveoliver commentedFabian,
What do you see as the performance implications of this approach? We are still not using preprocess.
-Steve
Comment #8
Fabianx CreditAttribution: Fabianx commentedLooks fine to me :-).
Re-titling, have to try this out, but from the look it is RTBC to me ...
Comment #9
sunLooks good. But you forgot to update the theme hook variables — done so for you :)
I only have a small tweak and change request:
That 'text' theme variable is literally the only one throughout Drupal that uses 'text' instead of 'title'. Likewise, 'path' is no longer a path, but the 'href' attribute value.
By renaming these two to 'title' and 'href', we can additionally eliminate some more confusion.
Comment #10
sunThis unset() appears to be unnecessary. Removed it.
Comment #11
sunHrm, unfortunately this proposed inlining of variable escaping into
l()
does not fly:The reason being that other code might not go through
l()
, especially if it wants to use theme hook suggestions.I.e., if I were better_login module and if I'd output a link, and I'd like themers to be able to target specifically my link, then I would do this:
Thus, this does not go through
l()
, but instead ends up intheme_link()
directly, without any variables being sanitized. Furthermore, the 'href' would likely not have gone throughurl()
yet, and the 'attributes' variable is not a printableAttribute
.We could combat this by adding a
template_preprocess_link()
preprocess function, but that would maketheme_link()
even slower than already documented.Comment #12
sunLike this...
Comment #14
Fabianx CreditAttribution: Fabianx commented#12: drupal8.l-theme.12.patch queued for re-testing.
Comment #16
steveoliver CreditAttribution: steveoliver commentedThis is what I'm thinkin:
Comment #18
sunwell, that has still the problem of #11, for which I could only see #12 as a possible solution (but obviously, that's going to be slow, as it effectively doubles the amount of function calls for every link that is rendered).
Comment #19
steveoliver CreditAttribution: steveoliver commentedMissed a couple things.
Comment #20
steveoliver CreditAttribution: steveoliver commentedYeah, I see what you're saying about
$options
being available orcheck_plain(url())
andnew Attribute
intheme_link
or_preprocess
.Hmm...
Comment #21
Fabianx CreditAttribution: Fabianx commentedWell, what about we get back to the original question:
Why does l() needs to use the template as soon as there is a preprocess function?
Or questioned otherwise:
In which cases do we want l() to be "slow" and render the link template?
I could think of decoupling l() and link: Yes, in that case you would need to overwrite two template files to overwrite it, but _you_ should only do this if you are sure about the performance implications anyway, so it may make sense to have l() having its own hook_link_l_preprocess / theme / template functions, because you _should_ know what you are doing if you are overwriting it.
Comment #22
steveoliver CreditAttribution: steveoliver commentedRe: Fabianx (#21):
I think we should separate l() from theme_link().
This patch does:
1. Simplies
l
function, removing check fortemplate_preprocess_link
ortheme_link
.2. Adds
template_preprocess_link
to set$variables['href'] = check_plain(url($path, $options));
and$variables['attributes'] = new Attribute($attributes);
3. Changes shortcut.module and toolbar.module implementations of
theme('link')
.Let's see what testbot has to say.
Comment #23
steveoliver CreditAttribution: steveoliver commentedRe:
I don't see how we'd need to overwrite two template files or why we'd need preprocess for the l() function.
Comment #24
steveoliver CreditAttribution: steveoliver commentedSo I wonder what people think about this (sun brought up in #9):
Variable names
'text'
or'title'
seem inaccurate while something like'content'
seems the most appropriate name for a variable that could basically be anything.haha -- woops.
Comment #26
Fabianx CreditAttribution: Fabianx commentedRelated:
#1833920: [META] Markup Utility Functions
Comment #27
steveoliver CreditAttribution: steveoliver commentedChanging title in light of #1833920: [META] Markup Utility Functions, which strikes to the root of this issue. :)
Comment #28
sunJesus. I'm annoyed by all the duplicate issues. Would be great if we could stop filing duplicates.
Postponing on #1833920: [META] Markup Utility Functions.
Comment #28.0
sunUpdated issue summary. Add related
Comment #29
Fabianx CreditAttribution: Fabianx commentedAnd re-activating! (because Markup Utility functions has been postponed)
After some discussions in #1833920-19: [META] Markup Utility Functions (see also #18 and #19 there), we are back here :-).
Comment #30
star-szrTaking a guess that this will be covered by #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig? If so, we should close this one :)
Comment #31
Fabianx CreditAttribution: Fabianx commentedRight, this is properly handled in #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig.
Comment #31.0
Fabianx CreditAttribution: Fabianx commentedrelated issue