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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

steveoliver’s picture

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.

Fabianx’s picture

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.

jenlampton’s picture

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:

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>
Fabianx’s picture

Issue tags: +Performance

Looks good, re-adding performance tag.

For comparison:

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

sun’s picture

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

steveoliver’s picture

Assigned: Unassigned » steveoliver

I'll roll a patch

steveoliver’s picture

Status: Active » Needs review
FileSize
2.16 KB

Fabian,

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

-Steve

Fabianx’s picture

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

sun’s picture

FileSize
2.32 KB
2.61 KB

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.

sun’s picture

FileSize
2.51 KB
+++ 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.

sun’s picture

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.

sun’s picture

Status: Needs work » Needs review
FileSize
3.33 KB

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.

Fabianx’s picture

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.

steveoliver’s picture

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
FileSize
4.71 KB

This is what I'm thinkin:

function theme_link($variables) {
  return '<a href="' . $variables['href'] . '"' . $variables['attributes'] . '>' . $variables['content'] . '</a>';
}
// 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';
  }
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.

sun’s picture

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

steveoliver’s picture

Status: Needs work » Needs review
FileSize
5 KB

Missed a couple things.

steveoliver’s picture

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

Fabianx’s picture

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.

steveoliver’s picture

Status: Needs work » Needs review
FileSize
12.33 KB

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.

steveoliver’s picture

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.

steveoliver’s picture

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.

Fabianx’s picture

steveoliver’s picture

Title: Rework theme_link() and l() function to use only $content, $href, and $attributes » Deprecate 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. :)

sun’s picture

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.

sun’s picture

Issue summary: View changes

Updated issue summary. Add related

Fabianx’s picture

Title: Deprecate theme_link in favor of l markup utility function » Remove 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 :-).

star-szr’s picture

Fabianx’s picture

Fabianx’s picture

Issue summary: View changes

related issue