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

Files: 
CommentFileSizeAuthor
#22 drupal-l-theme-link-1778610-22.patch12.33 KBsteveoliver
FAILED: [[SimpleTest]]: [MySQL] 47,699 pass(es), 131 fail(s), and 9 exception(s).
[ View ]
#19 drupal-l-theme-link-1778610-15.patch5 KBsteveoliver
FAILED: [[SimpleTest]]: [MySQL] 47,547 pass(es), 49 fail(s), and 0 exception(s).
[ View ]
#16 drupal-l-theme-link-1778610-14.patch4.71 KBsteveoliver
PASSED: [[SimpleTest]]: [MySQL] 47,580 pass(es).
[ View ]
#12 drupal8.l-theme.12.patch3.33 KBsun
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
#10 drupal8.l-theme.10.patch2.51 KBsun
PASSED: [[SimpleTest]]: [MySQL] 47,594 pass(es).
[ View ]
#9 drupal8.l-theme.9.patch2.61 KBsun
PASSED: [[SimpleTest]]: [MySQL] 47,606 pass(es).
[ View ]
#9 interdiff.txt2.32 KBsun
#7 drupal-l-theme-link-options-attributes-1778610-7.patch2.16 KBsteveoliver
PASSED: [[SimpleTest]]: [MySQL] 47,592 pass(es).
[ View ]
#1 drupal-template-preprocess-link-1778610-1.patch687 bytessteveoliver
PASSED: [[SimpleTest]]: [MySQL] 47,573 pass(es).
[ View ]

Comments

StatusFileSize
new687 bytes
PASSED: [[SimpleTest]]: [MySQL] 47,573 pass(es).
[ View ]

On a more general note, a preprocess for theme('link') is exactly what we need in order to use it in place of theme('toolbar_toggle') for the toolbar toggle arrow in (#1779104: Convert theme_toolbar_toggle to twig). Attached adds the preprocess that works so far.

Issue tags:+Performance

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

Issue tags:-Performance

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

<?php
function l($text, $path, array $options = array()) {
  static
$use_theme = NULL;
 
// Merge in defaults.
 
$options += array(
   
'attributes' => array(),
   
'html' => FALSE,
  );
 
// Append active class.
 
if (($path == current_path() || ($path == '<front>' && drupal_is_front_page())) &&
      (empty(
$options['language']) || $options['language']->langcode == language(LANGUAGE_TYPE_URL)->langcode)) {
   
$options['attributes']['class'][] = 'active';
  }
 
// Remove all HTML and PHP tags from a tooltip. For best performance, we act only
  // if a quick strpos() pre-check gave a suspicion (because strip_tags() is expensive).
 
if (isset($options['attributes']['title']) && strpos($options['attributes']['title'], '<') !== FALSE) {
   
$options['attributes']['title'] = strip_tags($options['attributes']['title']);
  }
 
// 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.
 
if (!isset($use_theme) && function_exists('theme')) {
   
// Allow edge cases to prevent theme initialization and force inline link
    // rendering.
   
if (variable_get('theme_link', TRUE)) {
     
drupal_theme_initialize();
     
$registry = theme_get_registry(FALSE);
     
// We don't want to duplicate functionality that's in theme(), so any
      // hint of a module or theme doing anything at all special with the 'link'
      // theme hook should simply result in theme() being called. This includes
      // the overriding of theme_link() with an alternate function or template,
      // the presence of preprocess or process functions, or the presence of
      // include files.
     
$use_theme = !isset($registry['link']['function']) || ($registry['link']['function'] != 'theme_link');
     
$use_theme = $use_theme || !empty($registry['link']['preprocess functions']) || !empty($registry['link']['process functions']) || !empty($registry['link']['includes']);
    }
    else {
     
$use_theme = FALSE;
    }
  }
 
$attributes = drupal_attributes($options['attributes']);
  unset(
$options['attributes']);
  if (
$use_theme) {
    return
theme('link', array('text' => $text, 'url' => url($path, $options)), 'attributes' => $attributes);
  }
 
// The result of url() is a plain-text URL. Because we are using it here
  // in an HTML argument context, we need to encode it properly.
 
return '<a href="' . check_plain(url($path, $options)) . '"' . $attributes . '>' . ($options['html'] ? $text : check_plain($text)) . '</a>';
}
?>

This removes the need for preprocess, hopefully avoiding performance implications, and still keeping our template clean and simple without removing any functionality.

Twig code:

<a href="{{ url }}"{{ attributes }}>{{ text }}</a>

Issue tags:+Performance

Looks good, re-adding performance tag.

For comparison:

http://api.drupal.org/api/drupal/core%21includes%21common.inc/function/l/8

That looks more acceptable, thanks. Now, who rolls a patch for this issue?

Assigned:Unassigned» steveoliver

I'll roll a patch

Status:Active» Needs review
StatusFileSize
new2.16 KB
PASSED: [[SimpleTest]]: [MySQL] 47,592 pass(es).
[ View ]

Fabian,

What do you see as the performance implications of this approach? We are still not using preprocess.

-Steve

Title:Remove the check for a preprocess function from the decision to use a template file instead of l().Rework theme_link() and l() function to use only $url, $attributes and $text.

Looks fine to me :-).

Re-titling, have to try this out, but from the look it is RTBC to me ...

StatusFileSize
new2.32 KB
new2.61 KB
PASSED: [[SimpleTest]]: [MySQL] 47,606 pass(es).
[ View ]

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

StatusFileSize
new2.51 KB
PASSED: [[SimpleTest]]: [MySQL] 47,594 pass(es).
[ View ]

+++ b/core/includes/common.inc
@@ -2369,12 +2369,19 @@ function l($text, $path, array $options = array()) {
+  // Remove HTML attributes from options array; url() does not need them.
+  unset($options['attributes']);

This unset() appears to be unnecessary. Removed it.

Status:Needs review» Needs work

Hrm, unfortunately this proposed inlining of variable escaping into l() does not fly:

@@ -2369,12 +2369,17 @@ function l($text, $path, array $options = array()) {
+  $href = check_plain(url($path, $options));
+  $title = ($options['html'] ? $text : check_plain($text));
+  $attributes = new Attribute($options['attributes']);
...
function theme_link($variables) {
-  return '<a href="' . check_plain(url($variables['path'], $variables['options'])) . '"' . new Attribute($variables['options']['attributes']) . '>' . ($variables['options']['html'] ? $variables['text'] : check_plain($variables['text'])) . '</a>';
+  return '<a href="' . $variables['href'] . '"' . $variables['attributes'] . '>' . $variables['title'] . '</a>';
}

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:

$build = array(
  '#theme' => 'link__better_login',
  '#title' => t('Log in'),
  '#href' => 'better/login',
  '#attributes' => array('class' => array('better-login user-login')),
);

Thus, this does not go through l(), but instead ends up in theme_link() directly, without any variables being sanitized. Furthermore, the 'href' would likely not have gone through url() yet, and the 'attributes' variable is not a printable Attribute.

We could combat this by adding a template_preprocess_link() preprocess function, but that would make theme_link() even slower than already documented.

Status:Needs work» Needs review
StatusFileSize
new3.33 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

Like this...

Status:Needs review» Needs work
Issue tags:-Performance, -Twig, -theme system cleanup

The last submitted patch, drupal8.l-theme.12.patch, failed testing.

Status:Needs work» Needs review

#12: drupal8.l-theme.12.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Performance, +Twig, +theme system cleanup

The last submitted patch, drupal8.l-theme.12.patch, failed testing.

Title:Rework theme_link() and l() function to use only $url, $attributes and $text.Rework theme_link() and l() function to use only $content, $href, and $attributes
Status:Needs work» Needs review
StatusFileSize
new4.71 KB
PASSED: [[SimpleTest]]: [MySQL] 47,580 pass(es).
[ View ]

This is what I'm thinkin:

<?php
function theme_link($variables) {
  return
'<a href="' . $variables['href'] . '"' . $variables['attributes'] . '>' . $variables['content'] . '</a>';
}
?>

<?php
// Implementation (toolbar.module):
// Add an anchor to be able to toggle the visibility of the drawer.
 
$build['toolbar_toggle'] = array(
   
'#theme' => 'link__toolbar_toggle',
   
'#content' => _toolbar_is_collapsed() ? t('Show toolbar') : t('Hide toolbar'),
   
'#href' => 'toolbar/toggle',
   
'#options' => array('query' => drupal_get_destination()),
   
'#attributes' => new Attribute(array('class' => array('toggle'))),
  );
  if (
_toolbar_is_collapsed()) {
   
$build['toolbar_toggle']['#attributes']['class'][] = 'toggle-active';
  }
?>

<?php
function l($content, $href, $options, $attributes) {
 
// ...
 
$attributes = new Attribute($options['attributes']);
 
// There is no reason for url() to receive HTML attributes.
 
unset($options['attributes']);
 
// The result of url() is a plain-text URL. Because we are using it here
  // in an HTML argument context, we need to encode it properly.
 
$href = check_plain(url($path, $options));
 
$title = $options['html'] ? $text : check_plain($text);
  if (
$use_theme) {
    return
theme('link', array(
     
'title' => $title,
     
'href' => $href,
     
'attributes' => $attributes
   
));
  }
  return
'<a href="' . $href . '"' . $attributes . '>' . $title . '</a>';
}
?>

Status:Needs review» Needs work

The last submitted patch, drupal-l-theme-link-1778610-14.patch, failed testing.

well, 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).

Status:Needs work» Needs review
StatusFileSize
new5 KB
FAILED: [[SimpleTest]]: [MySQL] 47,547 pass(es), 49 fail(s), and 0 exception(s).
[ View ]

Missed a couple things.

Status:Needs review» Needs work

Yeah, I see what you're saying about $options being available or check_plain(url()) and new Attribute in theme_link or _preprocess.

Hmm...

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

Status:Needs work» Needs review
StatusFileSize
new12.33 KB
FAILED: [[SimpleTest]]: [MySQL] 47,699 pass(es), 131 fail(s), and 9 exception(s).
[ View ]

Re: Fabianx (#21):

I think we should separate l() from theme_link().

This patch does:
1. Simplies l function, removing check for template_preprocess_link or theme_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.

Re:

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.

I don't see how we'd need to overwrite two template files or why we'd need preprocess for the l() function.

So I wonder what people think about this (sun brought up in #9):

+++ b/core/includes/theme.inc
@@ -1620,25 +1620,33 @@ function theme_status_messages($variables) {
@@ -2967,7 +2975,7 @@ function drupal_common_theme() {
@@ -2967,7 +2975,7 @@ function drupal_common_theme() {
       'variables' => array('display' => NULL),
     ),
     'link' => array(
-      'variables' => array('text' => NULL, 'path' => NULL, 'options' => array()),
+      'variables' => array('content' => NULL, 'path' => NULL, 'options' => array(), 'attributes' => array()),
     ),
     'links' => array(
       'variables' => array('links' => array(), 'attributes' => array('class' => array('links')), 'heading' => array()),

Variable names 'text' or 'title' seem inaccurate while something like 'content' seems the most appropriate name for a variable that could basically be anything.

+++ b/core/includes/common.inc
@@ -5357,31 +5331,32 @@ function drupal_pre_render_conditional_comments($elements) {
+  if (!isset($element['#content']) || !isset($element['#path'])) {
+    $oh_shit = TRUE;
   }

haha -- woops.

Status:Needs review» Needs work

The last submitted patch, drupal-l-theme-link-1778610-22.patch, failed testing.

Title:Rework theme_link() and l() function to use only $content, $href, and $attributesDeprecate theme_link in favor of l markup utility function

Changing title in light of #1833920: [META] Markup Utility Functions, which strikes to the root of this issue. :)

Status:Needs work» Postponed

Jesus. I'm annoyed by all the duplicate issues. Would be great if we could stop filing duplicates.

Postponing on #1833920: [META] Markup Utility Functions.

Issue summary:View changes

Updated issue summary. Add related

Title:Deprecate theme_link in favor of l markup utility functionRemove the check for a link template from l(), have l() always output just a string.
Status:Postponed» Active

And 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 :-).

Issue summary:View changes

related issue