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.

TL;DR

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

Problem/Motivation

Currently there is some discussion about the best way to use l() and url() in twig templates #1812562: Create best practices about use of the l() function and the url() function in templates and there is work being done towards converting all the existing core modules to use templates #1757550: [Meta] Convert core theme functions to Twig 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 is #1812562: Create best practices about use of the l() function and the url() function in templates but 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:

  1. 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 #1898420: image.module - Convert theme_ functions to Twig for what can only be the tip of the iceberg.
  2. 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.
  3. 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 #1187032: theme_link needs to be clearer about saying not to call it directly.
    • 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() #1836730: Add a renderable object that is equivalent to l(). This is inline with a general movement towards renderable objects #1843798: [meta] Refactor Render API to be OO 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 #1833920: [META] Markup Utility Functions and Steveoliver in #1836730: Add a renderable object that is equivalent to l() 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.

Proposed resolution

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.

Remaining tasks

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.

User interface changes

none

API changes

  1. 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.
  2. There will be a new alter hook for modifying the structure (not the markup) of all links whether generated by l() or through templates.
  3. theme('link') will be useful.
  4. l() will no longer invoke theme functions.
  5. l() will be available from within Twig templates.
CommentFileSizeAuthor
#118 1922454-118.patch34.26 KBthedavidmeister
#118 interdiff-117-118.txt805 bytesthedavidmeister
#117 1922454-117.patch34.25 KBthedavidmeister
#117 interdiff-116-117.txt894 bytesthedavidmeister
#116 1922454-116.patch34.13 KBthedavidmeister
#116 interdiff-109-116.txt1.88 KBthedavidmeister
#110 1922454-109.patch33.74 KBthedavidmeister
#110 interdiff-104-109.txt2.05 KBthedavidmeister
#105 interdiff-85-104.txt2.76 KBthedavidmeister
#104 1922454-104.patch33.06 KBthedavidmeister
#100 1922454-100-head-l-do-not-test.patch817 bytesstar-szr
#100 1922454-100-head-theme-link-do-not-test.patch967 bytesstar-szr
#100 1922454-100-gut-l-85-alter-do-not-test.patch34.97 KBstar-szr
#85 drupal-l-theme-link--1922454-83-85-interdiff.txt11.45 KBsteveoliver
#85 drupal-l-theme-link--1922454-85.patch33.72 KBsteveoliver
#83 1922454-gut-l-fix-theme-link-83.patch34.63 KBthedavidmeister
#83 interdiff-81-83.txt2.34 KBthedavidmeister
#81 1922454-gut-l-fix-theme-link-81.patch33.52 KBthedavidmeister
#81 interdiff-78-81.txt7.77 KBthedavidmeister
#78 1922454-gut-l-fix-theme-link-78.patch30.77 KBthedavidmeister
#78 interdiff-76-78.txt872 bytesthedavidmeister
#76 1922454-gut-l-fix-theme-link-76.patch30.78 KBthedavidmeister
#76 interdiff-72-76.txt7.73 KBthedavidmeister
#72 1922454-gut-l-fix-theme-link-72.patch29.83 KBthedavidmeister
#72 interdiff-69-72.txt6.12 KBthedavidmeister
#69 1922454-gut-l-fix-theme-link-69.patch29.8 KBthedavidmeister
#69 interdiff-63-69.txt4.17 KBthedavidmeister
#63 1922454-gut-l-fix-theme-link-63.patch31.66 KBthedavidmeister
#63 interdiff-56-63.txt16.52 KBthedavidmeister
#56 1922454-gut-l-fix-theme-link-56.patch18.56 KBthedavidmeister
#56 interdiff-54-56.txt7.9 KBthedavidmeister
#54 1922454-gut-l-fix-theme-link-54.patch10.53 KBthedavidmeister
#54 interdiff-41-54.txt1.74 KBthedavidmeister
#41 1922454-gut-l-fix-theme-link-41.patch10.31 KBsteveoliver
#41 1922454-gut-l-fix-theme-link-36-41--interdiff--do-not-test.patch952 bytessteveoliver
#38 1922454-gut-l-fix-theme-link-38.patch10.32 KBsteveoliver
#38 1922454-gut-l-fix-theme-link-36-38--interdiff--do-not-test.patch952 bytessteveoliver
#36 drupal-l-theme-link--1922454-36.patch10.26 KBsteveoliver
#32 drupal-l-theme-link--1922454-32.patch10.26 KBsteveoliver
#32 drupal-l-theme-link--1922454-27-32--interdiff-do-not-test.patch740 bytessteveoliver
#27 drupal-l-theme-link--1922454-27.patch10.23 KBsteveoliver
#27 drupal-l-theme-link--1922454-15-27--interdiff--do-not-test.patch6.36 KBsteveoliver
#15 1922454-gut-l-fix-theme-link-15.patch8.93 KBthedavidmeister
#5 1922454-gut-l-fix-theme-link-5.patch8.51 KBthedavidmeister
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

thedavidmeister’s picture

Title: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of twig. » [meta] Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of twig.

sorry, this is a meta issue.

jenlampton’s picture

I disagree with 3. Can't we just remove theme_link entirely? I don't see any good reason to include a template here.

thedavidmeister’s picture

I don't believe that we can get rid of it entirely. See http://drupal.org/node/1833920#comment-7075384 for my reasoning on that and the rest of the thread for more discussion on themable links, there's definitely interest from the community in having themable links

The summary of the problems I have with no themeable links is that if you have $my_complex_render_array and you want to pass it to the template as a render array, and you want to wrap part or whole of that render array in a link, you can't preserve the structure through the preprocess to the template without adding something like '#theme' => 'link' to your render array.

Unless I'm completely missing something about how drupal_render() works, as soon as we fix theme('link') we can use it in render arrays, right?

I'm happy to chat in #drupal-twig about this sometime if you'd like. I personally don't see a clear way forward on #1812562: Create best practices about use of the l() function and the url() function in templates without all three options "working" but i'd love to hear about alternatives :)

Fabianx’s picture

#2: Usage within render arrays, also paving the way to more complex structures later.

$x['abc']['cde'] = array(
  '#theme' => 'link',
  '#path' => 'admin/people'
  '#text' => t('My text'),
  '#options' => array ( 'html' => 'TRUE' ),
  // ...,
);

Because:

a) We don't want to use l() or theme('link') now, because we want to lazy-render.
b) We can't necessarily use l() within a template, because then we need to do something like:

$x['abc']['cde'] = array(
  'path' => 'admin/people',
  'text' => t('My text'),
  // ...,
);

but then we can also use #theme => 'link' in the first place ...

Also:

Even if we have theme_link, a template author could still access the parts of the original array like:

{{ l('mytext', abc.cde['#path'], abc.cde['#options']) }}

or

<a href="{{ url(abc.cde['#path'], abc.cde['#options']) }}" >mytext</a>

And then again the render array solution is again more helpful than the pre-rendered things ...

I hope this clears up the confusion.

l() is useful, but really only in cases when

a) An HTML string is needed - i.e. in a template
b) We need the speed of it, i.e. for menu_links or such

This solution gives the most flexibility.

thedavidmeister’s picture

Status: Active » Needs review
FileSize
8.51 KB

rough patch attached. At the very least it needs extra documentation.

Status: Needs review » Needs work

The last submitted patch, 1922454-gut-l-fix-theme-link-5.patch, failed testing.

Fabianx’s picture

Status: Needs work » Active
+++ b/core/includes/theme.incundefined
@@ -3144,7 +3167,8 @@ function drupal_common_theme() {
-      'variables' => array('text' => NULL, 'path' => NULL, 'options' => array()),
+      'variables' => array('link' => array()),

Uhm, this change is probable not wanted ...

Otherwise looks on first glance good to me.

thedavidmeister’s picture

@Fabianx, yeah. That's a mistake. I don't know what I was trying to do there. I'll fix that and add some better documentation. Not really sure why the tests are failing to find the active class for different languages when I copied and pasted exactly what was there previously :/

jenlampton’s picture

I'm still not convinced there's any good reason to have themeable links. Let's stop and think about what a theme function (or in this case, a Twig template file) provides for a second. It would allow the markup for all the anchor tags on a site to be overridden from a theme (or even a module). Do we really want that?

We may need to add something for render arrays - so that they know which parts of the array need to be turned into an actual link - but let's not do that by allowing all links on a site to be overridden with a template file. It's the wrong tool for the job. Yes, maybe we have it already, but that doesn't mean we should be using it for this :)

Yes, we also need to be able to alter links, but that is not the job of a templating system. If we can get some kind of renderable thingy for a link - a data structure that can be changed plus a function to spit it out - then the need for a themeable link goes away.

Can we re-examine markup utility functions for this, please?

thedavidmeister’s picture

@jenlampton, So you'd like to extend drupal_render() to take something like '#render_callback' => 'l' and just gut l() and kill theme('link'). That works for me, like you've said across multiple threads, I have zero use-cases to theme links, I only want renderables to work.

Would you like me to roll a patch as a proof of concept for this?

Fabianx’s picture

#9: Can we please start with the approach here and do the cleanup as follow-up based on use cases we encounter during the conversion. There are several bugs and limitations related to l() and theme_link() and we _need_ to fix them anyway.

Second:

#theme => link is currently according to our docs our best practice to create links, and you can even with them being a render array use:

<a href={{ url(link['#path'], link['#options']) }}>{{ link['#text'] }}</a>

within Twig right now. (though you would loose the alterability)

Please do not block a quite simple patch that is fixing bugs and limitations based on a future change that needs more thought and might have deep internal restructuring needs as well.

This is simple and it will allow easy customizations now.

theme_link does exist right now. Lets kill it when we have a viable alternative. There is a reason dozens of other issues are still open, this one focuses just on these goals:

* Make l() and theme_link() equally usable standalone, but separate them
* Make l() always fast
* Make theme_link() always flexible
* Make both changeable via alter.

I agree that this is not the end solution we want, but it is IMHO a first step in the right direction and it will make it easier to implement a nicer solution. I can promise so much already :-). Micro Steps, pretty please?

Thanks!

thedavidmeister’s picture

Ok, well the patch I put up before should be easy to fix - $options['language'] is just completely wrong inside url_is_active() based on how I implemented that. Changing $query to $options in url_is_active() and fixing #7 should make that patch come back green.

Fabianx, are you sure you don't even want to look at a drupal_render() extension patch right now? it would be nice to know how many tests pass/fail for this idea of jen's so we know where it stands from a maintaining-existing-functionality perspective.

Also where is it documented that '#theme' => 'link' is the right thing to do? it really shouldn't be considering its complete lack of url sanitisation :/

jenlampton’s picture

Baby steps++ I'll go make noise in the other issues then :) Carry on!

P.S. you both rock. :)

Fabianx’s picture

Fabianx, are you sure you don't even want to look at a drupal_render() extension patch right now? it would be nice to know how many tests pass/fail for this idea of jen's so we know where it stands from a maintaining-existing-functionality perspective.

Yes, I want to see 100s of patches for that, but as a follow-up / different issue. It is imperative that we make progress first, then work on the harder patch.

This patch has the potential to unblock more best practices for template conversions.

Also where is it documented that '#theme' => 'link' is the right thing to do? it really shouldn't be considering its complete lack of url sanitisation :/

a) It is not documented explicitly, but if we ask contributors to "always use render arrays within preprocess, do not flatten strings" the implicit consequence is a usage of '#theme' => 'link' for now. Does that make sense?

b) URL Sanitation - That is an interesting point. We definitely should check_plain or '| e' the url output for theme_link like in l(). Too bad we don't have auto-autoescape yet.

If that is wrong, that is a bug, but could be fixed as part of this issue.

thedavidmeister’s picture

Status: Active » Needs review
FileSize
8.93 KB

Added documentation to link.html.twig, Fixed #7, hopefully fixed the language switching test fails.

Status: Needs review » Needs work
Issue tags: -Twig

The last submitted patch, 1922454-gut-l-fix-theme-link-15.patch, failed testing.

Fabianx’s picture

Status: Needs work » Needs review
Issue tags: +Twig
sun’s picture

I don't really understand the patch in #15 — instead of removing theme_link(), it seems to advanced on it and turning it into a theme template even?

Fabianx’s picture

#18 Yes, that is an intermediate step that allows to use #theme => link within preprocess functions.

However we could keep it as a theme_function for now as really we want something else as well.

It would still be great to have this in to make #theme => link work.

thedavidmeister’s picture

#18 Yep, part of this issue is to make theme('link') an acceptable standalone option otherwise it can't be pulled out of l() without losing desired functionality as Fabianx said in #19.

It could be left as a theme function, but this would be contrary to the Twig conversion efforts outlined at #1757550: [Meta] Convert core theme functions to Twig templates:

Now that the Twig engine is in core, we must convert all existing core *.tpl.php template files and theme_ functions to use Twig. As a result, all theme callbacks will utilize .html.twig templates.

I didn't see any reason to make more work for everyone following that thread.

thedavidmeister’s picture

@jenlampton, In #1812562: Create best practices about use of the l() function and the url() function in templates you said:

In preprocess, we can prepare $variables['link'] as an array with three or four parts: href, content, attributes, and options.

Is this something that you would like to see implemented here? currently l() and theme_link() both take only three parameters - $path, $text and $options but if you have a case for using four parameters (like making Twig templates easier to write/more readable) then that is possibly something I should change in this patch.

dcrocks’s picture

Also please consider that not all attribute values are for printing to the screen. The present design of l() uses Attribute which runs 'htmlspecialchars' on the attribute values, assuming any attribute is like 'title', printable to the the screen. This has been ok because attributes like 'class' or aria 'roles' don't typically have special characters in them. But 'aria=*' global attributes might. And "numeric character references"(eg. &#xe000) are necessary for the global data-* attribute. Currently it is impossible to implement the data-* attribute on menu links.

thedavidmeister’s picture

@dcrocks, Could you link to an issue where this is being addressed/discussed with reference to the Attribute class?

I believe using Attribute is correct here, and has never been any other way - drupal_attributes() in l() which uses check_plain() goes back to Drupal 4.7 at least.

I only understand check_plain() as a very standard Drupal security feature, from the docs on drupal_attributes():

 // By running the value in the following statement through check_plain,
  // the malicious script is neutralized.
  drupal_attributes(array('title' => t('<script>steal_cookie();</script>')));

  // The statement below demonstrates dangerous use of drupal_attributes, and
  // will return an onmouseout attribute with JavaScript code that, when used
  // as attribute in a tag, will cause users to be redirected to another site.
  //
  // In this case, the 'onmouseout' attribute should not be whitelisted --
  // you don't want users to have the ability to add this attribute or others
  // that take JavaScript commands.
  drupal_attributes(array('onmouseout' => 'window.location="http://malicious.com/";')));

I'm not convinced this thread is the place to discuss making check_plain() or the Attribute class more permissive of allowable characters in HTML attributes and I don't understand what alternative implementation you're proposing?

dcrocks’s picture

My issue is #1836160: Support icon fonts in menu links. It is true that this has worked fine for a very long time. The only attributes you might have found in a link were 'title' or 'class'. But now you might find data-* or aria-* as an attribute. The patch I was working on did take into account the possibility of embedding html script into an attribute(basically the same way as the 'title' attribute is checked). Times have changed. This should probably be another issue, if for no other reason than I expect increased use of aria-* attributes and there is currently no security checking done on them. But I also get the feeling there might be a lack of interest in pursuing this at this time because of other pressing issues. And this issue emphasizes making l() simpler and quicker, not adding additional code. I guess I need some input as to whether it is worthwhile to pursue this. I am not seeing much discussion in my issue and I don't want to open a issue that will be ignored.

thedavidmeister’s picture

#24 - I can't see any patch on that thread, only some jpgs. Regardless, I'm not saying that your problem isn't worth following up, just that it should probably be handled inside the Attribue class instead of inside l(). If HTML attributes are not correctly being handled by Attribute, then you should file an issue against that, or provide a patch in your existing issue that addresses Attribute. If you are looking for feedback on work you've done, please upload a patch and set your issue to "needs review".

aria- and data- attributes can be used on more than just links, so l() shouldn't know anything about handling them. l() should only know things about creating markup specific to links and handball any other processing off to other specialised functions.

The only attribute specific to links is the "active" class, and we're handling that in l().

dcrocks’s picture

steveoliver’s picture

OK,

  1. the link.html.twig template didn't get picked up without 'template' added to drupal_common_theme's entry for link.
  2. Also, if anything were actually calling theme('link'), the template wouldn't have worked as attributes weren't being called properly.
  3. I edited some docs.
  4. I ran the URL generation test from the Common group and all Theme group tests locally. After the URL generation test it actually failed with:

    An AJAX HTTP error occurred. HTTP Result Code: 200 Debugging information follows. Path: /8.xx/index.php/batch?id=2&op=do_nojs&op=do StatusText: OK ResponseText: Uncaught exception thrown in shutdown function.PDOException: SQLSTATE[HY000]: General error in cache_invalidate_tags() (line 80 of /usr/local/var/www/8.xx/core/includes/cache.inc).Uncaught exception thrown in shutdown function.Drupal\Core\Database\IntegrityConstraintViolationException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'state-menu_expanded' for key 'PRIMARY': DELETE FROM {semaphore} WHERE (value = :db_condition_placeholder_0) ; Array ( [:db_condition_placeholder_0] => 21021903485131ac9d068503.21857834 ) in Drupal\Core\Database\Connection->query() (line 551 of /usr/local/var/www/8.xx/core/lib/Drupal/Core/Database/Connection.php).

    ...but I don't think that's related to this.

  5. Last, I think we'll want to add at least one test for the use of theme('link').
steveoliver’s picture

Issue tags: +Needs tests

...and a test for for hook_link_alter.

thedavidmeister’s picture

Status: Needs review » Needs work

I like the refactor of l() in #27, makes it easier to read. The changes in the interdiff all make sense to me. nice work :)

We definitely need tests around this though, I had a typo in one of my variables and #15 came back green. not good :(

Fabianx’s picture

Title: [meta] Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of twig. » Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of twig.

This is not a meta issue.

Given that l() and template_preprocess_link both already call url_is_active(), I would be fine with a helper function that is shared by l() and template_preprocess_link() to reduce the code duplication.

However calling template_preprocess_link from l() - like proposed by thedavimeister in IRC before does not feel right to me.

I already like the patch like it is now very much. Just saying that this could be nice to have.

tstoeckler’s picture

+  $query_match = (drupal_container()->get('request')->query->all() == $options['query']);

as url_is_active() is a public function this should account for $options['query'] not being set.

steveoliver’s picture

steveoliver’s picture

Assigned: Unassigned » steveoliver

I'll write the tests, probably today...

thedavidmeister’s picture

Status: Needs work » Needs review

running tests on #32.

Status: Needs review » Needs work

The last submitted patch, drupal-l-theme-link--1922454-32.patch, failed testing.

steveoliver’s picture

Status: Needs work » Needs review
FileSize
10.26 KB

So.. apparently we allow empty query parameters to url().

Changed !empty() check to isset().

tstoeckler’s picture

Doesn't this make url_is_active('some/site') always return FALSE, because I did not set $options['query']? We should also have some DrupalUnitTest's for url_is_active(). We should be able to create mock request objects and put those in the container.

steveoliver’s picture

First, answering @tstoekler's comments in #31 and #37:

@tstoeckler: re #37:

+++ b/core/includes/common.inc
@@ -2227,68 +2261,50 @@ function drupal_http_header_attributes(array $attributes = array()) {
  *   An HTML string containing a link to the given path.
  *
  * @see url()
+ * @see theme_link()
  */
 function l($text, $path, array $options = array()) {
-  static $use_theme = NULL;
+  // Build a variables array to keep the structure of the alter consistent with
+  // template_preprocess_link().
+  $variables = array(
+    'text' => $text,
+    'path' => $path,
+    'options' => $options,
+  );
 
   // Merge in defaults.
-  $options += array(
+  $variables['options'] += array(
     'attributes' => array(),
     'query' => array(),
     'html' => FALSE,

$options['query'] will be an empty array if not set by the caller.
Similarly, drupal_container()->get('request')->query->all() will return an empty array if no query is in $_GET.

But to your point in #31, as url_is_active is a public function, we do want to support query not being set at all.

So this should pass.

Last, re: Tests: Actually, I'm not seeing what needs tests.

We already have tests for suggestions.
We already have tests for alters.
We already have a test for active link classes.

Let me know if I'm missing something.

Status: Needs review » Needs work

The last submitted patch, 1922454-gut-l-fix-theme-link-38.patch, failed testing.

steveoliver’s picture

Status: Needs work » Needs review
+++ b/core/includes/common.inc
@@ -2185,10 +2185,9 @@ function url_is_active($path, $options = array()) {
   // query parameters of the url equal those of the current request, since the
   // same request with different query parameters may yield a different page
   // (e.g., pagers).
-  $path_query = drupal_container()->get('request')->query->all();
   $path_match = ($path == current_path() || ($path == '<front>' && drupal_is_front_page()));
   $lang_match = (empty($options['language']) || $options['language']->langcode == language(LANGUAGE_TYPE_URL)->langcode);
-  $query_match = (empty($options['query']) && empty($path_query)) || ($options['query'] == $path_query);
+  $query_match = (drupal_container()->get('request')->query->all() == $options['query']);
 
   return $path_match && $lang_match && $query_match;

Haha -- woops, this is diff is actually backwards. Swap - with + ;)

steveoliver’s picture

OK, both were backwards.

Here we go. Sorry for all the noise.

thedavidmeister’s picture

#38 - I believe those tests are run against markup generated by l() aren't they? We'd need the same suite of tests run against links generated with '#theme' => 'link' to ensure consistent behaviour of both the MUF and the theme function (pre-overrides on the theme function of course). One of the main reasons we're rolling this patch is that theme('link') currently is *not* consistent with the output of l() in that it doesn't check the active class or sanitize the return of url().

tstoeckler’s picture

We should also have some unit-y tests that test url_is_active() directly.
I.e.

$this->container->set(new Request('foo/bar'));
// Note the code above will not actually work, this is just an example. I don't
// know off the top of my head how to set a mock request in a DrupalUnitTestBase.
// Only in the case that is not easily possible, we can leave the tests to a follow-up.
$this->assertTrue(url_is_active('foo/bar'));
tstoeckler’s picture

Oh, and also:

+  $query_match = (empty($options['query']) && empty($path_query)) || ($options['query'] == $path_query);

Sorry, that I'm obsessing so much about this line, but this will be true if $options['query'] is FALSE and $path_query is '0', for example. We should use a strict comparison (===) instead. It may (or may not, your call) make sense to leave a comment for that.

steveoliver’s picture

Issue tags: +Needs tests

#42, oh that's right.
#43, yeah, right, since it can be called outside of l/theme('link')
#44, yeah, strict check it better.

steveoliver’s picture

So check this out:

Here's the page callback for UrlTest, which tests for active links on classes:

/**
 * Page callback: Displays links to the current page, one with a query string.
 */
function common_test_l_active_class() {
  return array(
    'no_query' => array(
      '#type' => 'link',
      '#title' => t('Link with no query string'),
      '#href' => current_path(),
    ),
    'with_query' => array(
      '#type' => 'link',
      '#title' => t('Link with a query string'),
      '#href' => current_path(),
      '#options' => array(
        'query' => array(
          'foo' => 'bar',
        ),
      ),
    ),
  );
}

api docs

Being tested by this:

  /**
   * Tests for active class in l() function.
   */
  function testLActiveClass() {
    $path = 'common-test/l-active-class';
    $options = array();

    $this->drupalGet($path, $options);
    $links = $this->xpath('//a[@href = :href and contains(@class, :class)]', array(':href' => url($path, $options), ':class' => 'active'));
    $this->assertTrue(isset($links[0]), 'A link to the current page is marked active.');

    $options = array('query' => array('foo' => 'bar'));
    $links = $this->xpath('//a[@href = :href and not(contains(@class, :class))]', array(':href' => url($path, $options), ':class' => 'active'));
    $this->assertTrue(isset($links[0]), 'A link to the current page with a query string when the current page has no query string is not marked active.');

    $this->drupalGet($path, $options);
    $links = $this->xpath('//a[@href = :href and contains(@class, :class)]', array(':href' => url($path, $options), ':class' => 'active'));
    $this->assertTrue(isset($links[0]), 'A link to the current page with a query string that matches the current query string is marked active.');

    $options = array();
    $links = $this->xpath('//a[@href = :href and not(contains(@class, :class))]', array(':href' => url($path, $options), ':class' => 'active'));
    $this->assertTrue(isset($links[0]), 'A link to the current page without a query string when the current page has a query string is not marked active.');
  }

api docs

Yet a but it's theme('link') ('#type' => 'link') that's producing those links for the page callback:

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>';
}

(theme.inc)

Auuuh?

steveoliver’s picture

Status: Needs review » Postponed

I just threw a question out on #drupal-wscci and learned that #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence means we need to talk to katbailey about why this issue is probably stupid :)

so I'll see what she's got to say...

nevets’s picture

I realize I am let to this thread and may have missed a point but the use of l()/url() have one great advantage over straight 'a' tags. They use relative urls AND are aware of the sites base url making the resulting links portable.

thedavidmeister’s picture

#48 - nobody is suggesting we use a tags without using url() to generate the href first :)

steveoliver’s picture

Status: Postponed » Active

Regarding the active classes, see what we may end up doing with #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence in my comment #84

thedavidmeister’s picture

#50 - that looks promising, but that patch isn't even coming back green yet and you've already requested your own, new things to refactor in there. Do we really want this patch (which is very close and blocking a bunch of other issues) to wait on that fairly sizeable restructure of the way that url processing works?

Wouldn't it be better to get this patch sorted, get it in core, unblock issues waiting for renderable links to get Twig templates ready for core and then either post a follow up issue, or refactor url_is_active() in the other url() issue you linked?

steveoliver’s picture

Assigned: steveoliver » Unassigned

#51, Yes, let's move this forward. Unassigning myself as I've got to get some actual work-work done today.

thedavidmeister’s picture

Assigned: Unassigned » thedavidmeister

having a look at this today.

thedavidmeister’s picture

Status: Active » Needs review
FileSize
1.74 KB
10.53 KB

Building on the work from #41 I have to say that I disagree with the idea in #44 and #45 that strict equality checking for query parameters is a good idea.

drupal_container()->get('request')->query->all() will always return an array - see the all() function in vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/ParameterBag.php

This means that we never encounter anything like 0 == FALSE proposed in #44, at worst FALSE or 0 would match an empty array which seems harmless to me.

However, what *does* happen is that if we try to use strict equality checking for arrays the *order* of the array is checked as well which is a problem.

For example if we have the url "http://example.com/foo?b=2&a=1" and we check to see if it is active with this code url_is_active('foo', array(a=>1, b=>2)) then we will get FALSE - which is very unlikely to be the desired behaviour.

I did see that #41 introduced some PHP warning errors when there was a query in the url but $options['query'] was not set.

Essentially though this issue is getting well off track if we're trying to change the *existing* logic generating active classes. Here's a patch to address #37 without completely re-writing the active class logic.

thedavidmeister’s picture

#46 - I just inspected the return from element_info('link'); which shows how render() would generate markup for '#type' => 'link' and it isn't theme_link() producing that markup for the test, it's drupal_pre_render_link().

If theme_link() could produce an active class like that, we wouldn't be working on this patch :P

/**
 * Pre-render callback: Renders a link into #markup.
 *
 * Doing so during pre_render gives modules a chance to alter the link parts.
 *
 * @param $elements
 *   A structured array whose keys form the arguments to l():
 *   - #title: The link text to pass as argument to l().
 *   - #href: The URL path component to pass as argument to l().
 *   - #options: (optional) An array of options to pass to l().
 *
 * @return
 *   The passed-in elements containing a rendered link in '#markup'.
 */
function drupal_pre_render_link($element) {
  // By default, link options to pass to l() are normally set in #options.
  $element += array('#options' => array());
  // However, within the scope of renderable elements, #attributes is a valid
  // way to specify attributes, too. Take them into account, but do not override
  // attributes from #options.
  if (isset($element['#attributes'])) {
    $element['#options'] += array('attributes' => array());
    $element['#options']['attributes'] += $element['#attributes'];
  }

  // This #pre_render callback can be invoked from inside or outside of a Form
  // API context, and depending on that, a HTML ID may be already set in
  // different locations. #options should have precedence over Form API's #id.
  // #attributes have been taken over into #options above already.
  if (isset($element['#options']['attributes']['id'])) {
    $element['#id'] = $element['#options']['attributes']['id'];
  }
  elseif (isset($element['#id'])) {
    $element['#options']['attributes']['id'] = $element['#id'];
  }

  // Conditionally invoke ajax_pre_render_element(), if #ajax is set.
  if (isset($element['#ajax']) && !isset($element['#ajax_processed'])) {
    // If no HTML ID was found above, automatically create one.
    if (!isset($element['#id'])) {
      $element['#id'] = $element['#options']['attributes']['id'] = drupal_html_id('ajax-link');
    }
    // If #ajax['path] was not specified, use the href as Ajax request URL.
    if (!isset($element['#ajax']['path'])) {
      $element['#ajax']['path'] = $element['#href'];
      $element['#ajax']['options'] = $element['#options'];
    }
    $element = ajax_pre_render_element($element);
  }

  $element['#markup'] = l($element['#title'], $element['#href'], $element['#options']);
  return $element;
}

It's basically just a wrapper that presets some variables and then calls l() directly. I actually wasn't aware of this function before, not sure why it's never come up in these discussions around theming l() before... I guess you learn something new every day >.<

Anyway, for the tests I think we want to duplicate/extend what common_test_l_active_class() does with '#type' swapped out for '#theme' to make sure we get the same thing for both.

thedavidmeister’s picture

Assigned: thedavidmeister » Unassigned
FileSize
7.9 KB
18.56 KB

This patch extends the tests provided by testLActiveClass() to both cover theme('link') and to explicitly test that re-arranging the order of query parameters in the url doesn't confound the active class logic as I mentioned in #54.

TODO:

- Extend testLCustomClass() to provide coverage for theme('link')
- Provide test coverage for url_is_active() similar to testUrl() and testExternalUrls(), don't forget to test language switching like the test I failed in #5 from LanguageSwitchingTest.php!

I'm un-assigning this ticket from myself so that anyone else can feel free to jump in and mop up these last couple of tasks. Otherwise I'll just re-assign myself when I find free time again (probably next weekend) and hopefully get this knocked off.

tstoeckler’s picture

@thedavidmeister: I guess that's a fair point. A little array sorting wouldn't hurt either IMO, but I don't really care either.

dcrocks’s picture

A question. You have 2 invocations of a hook (drupal_container()->get('module_handler')->alter('link', $variables);) in the patch that weren't in the previous code. Are there existing places in core where you expected the hook to be implemented? I'm asking because it might be relevant to another issue I am looking at.

thedavidmeister’s picture

#58 - the alters have been there at least since #15. AFAIK they haven't been changed across reviews since they were added. Wasn't planning on implementing them directly.

As per the issue summary, the alters were originally suggested by Fabianx in IRC. The reasoning is that while we are remove the themability of l(), it is for performance reasons, not because we believe other modules shouldn't be able to modify the output from l(). Providing an alter facilitates replacing a little of the flexibility that we're losing in this patch.

If you know of another issue that's relevant to what's happening here, by all means link it for us :)

dcrocks’s picture

Thanx for the info. The summary is quite a long read. It doesn't look like these hooks are relevant to my issue but I am scanning a lot of stuff right now.

thedavidmeister’s picture

#57 - ATM we're really not planning on changing any of the existing logic that was in l() before this thread started. If you'd like to change how the active class is generated then please open a separate issue and link to it here so interested parties can track progress.

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

thedavidmeister’s picture

Assigned: Unassigned » thedavidmeister

writing some tests atm.

thedavidmeister’s picture

Assigned: thedavidmeister » Unassigned
FileSize
16.52 KB
31.66 KB

This patch adds tests for the following:

- XSS filtering for theme('link') - already existed for l()
- Adding custom classes with theme('link') - already existed for l()
- Test the active class across different languages for both l() and theme('link') - sort of existed for l()
- Improves documentation slightly

I wanted to do "unit-y test" stuff for url_is_active() as proposed in #43 but I wasn't sure how. Using current_path() inside the testbot always returns "batch" - floated the problem in IRC and didn't get much of a response. Open to suggestions/patches as to how this could be achieved...

Fabianx’s picture

Status: Needs review » Needs work

* Updated issue summary
* Reviewed the patch again in-depth
* Looks really good, has extensive test coverage, addressed feedback (as far as I can see) AND ...
* ... was ready to mark it RTBC, but:

According to the current twig-template-commit-freeze, we need to use a theme function for now.

So I need to put this to CNW for now. I think afterwards we are ready to try ourselves at RTBC.

Thanks!

thedavidmeister’s picture

#64 - there's a freeze on twig templates? why isn't that written all over #1757550: [Meta] Convert core theme functions to Twig templates? could you link in the discussion where they were frozen?

steveoliver’s picture

--- /dev/null
+++ b/core/modules/system/templates/link.html.twig
@@ -0,0 +1,15 @@
+{#
+/**
+ * Returns HTML for a link tag.
+ *
+ * Available variables
+ * - text: The content to display within the <a> element.
+ * - url: The sanitized url for this link.
+ * - url_is_active: TRUE if the url for this link leads to the current page.
+ * - attributes: (optional) HTML attributes to apply to the <a> element.
+ *
+ * @see template_preprocess_link()
+ * @ingroup themeable
+ */
+#}
+<a href="{{ url }}" class="{{ attributes.class }}" {{ attributes }}>{{ text }}</a>

We've recently discussed getting everything Twig RTBC, then holding all RTBC patches as [READY] or some other issue prefix until we can commit everything at once. So I think Fabianx is just saying to leave the Twig conversion out of this, returning markup from theme_link for now, so this patch can get in.

thedavidmeister’s picture

#66 - commit everything at once?? woah...

well chances are I won't get to update the patch til the weekend. would anyone else have a chance to swap the preprocess out for a theme function before then? should be straightforward enough :)

thedavidmeister’s picture

Assigned: Unassigned » thedavidmeister

I'm looking at this now.

thedavidmeister’s picture

Status: Needs work » Needs review
FileSize
4.17 KB
29.8 KB

Without the twig template, there seemed to be no more reason for all the duplicate code in the theme/preprocess function for links. theme_link() looks like this now:

function theme_link(&$variables) {
  return l($variables['text'], $variables['path'], $variables['options']);
}
Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Interesting.

That is good enough for me, works nicely and unifies this as well and is also compatible with #type => link and #type => links, which also calls l().

Lets get this in as a first step:

* Passes tests.
* Has extensive test coverage.
* Unifies l() and #theme => link
* Unblocks other twig theme conversions

=> RTBC

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs tests

Wow, this is intense. Just a couple nitpicks before commit.

+++ b/core/includes/common.incundefined
@@ -2161,6 +2161,46 @@ function url_is_external($path) {
+ * @param string $path
+ *   An internal path or external url as could be used by url().
+ *
+ * @param array $options

No blank line between @params

+++ b/core/includes/common.incundefined
@@ -2161,6 +2161,46 @@ function url_is_external($path) {
+  // query parameters of the url equal those of the current request, since the

URL, not url

+++ b/core/includes/common.incundefined
@@ -2161,6 +2161,46 @@ function url_is_external($path) {
+  $path_match = ($path == current_path() || ($path == '<front>' && drupal_is_front_page()));
+  $lang_match = (empty($options['language']) || $options['language']->langcode == language(LANGUAGE_TYPE_URL)->langcode);

@@ -2229,68 +2269,50 @@ function drupal_http_header_attributes(array $attributes = array()) {
+  $text = ($variables['options']['html']) ? $variables['text'] : check_plain($variables['text']);

These extra parentheses around the ternary aren't a common pattern in core, can they be removed?

+++ b/core/includes/common.incundefined
@@ -2161,6 +2161,46 @@ function url_is_external($path) {
+  $query_match = drupal_container()->get('request')->query->all() == $options['query'];

Drupal::service('request')->query->all()

+++ b/core/includes/common.incundefined
@@ -2229,68 +2269,50 @@ function drupal_http_header_attributes(array $attributes = array()) {
+  drupal_container()->get('module_handler')->alter('link', $variables);

Drupal::service('module_handler')->alter('link', $variables);

+++ b/core/modules/language/tests/language_test/language_test.moduleundefined
@@ -119,3 +129,89 @@ function language_test_menu() {
+          'id' => array('no_lang_link'),
...
+          'id' => array('fr_link'),
...
+          'id' => array('en_link'),
...
+          'id' => array('no_lang_link'),
...
+          'id' => array('fr_link'),
...
+          'id' => array('en_link'),

I'm surprised this works, ID is supposed to be singular.

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/UrlTest.phpundefined
@@ -28,47 +28,99 @@ public static function getInfo() {
+    $theme_link = render($link_array);
...
+    $link_theme = render($theme_link);

Should just use drupal_render()

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/UrlTest.phpundefined
@@ -28,47 +28,99 @@ public static function getInfo() {
+    $options_query = array('query' => array('foo' => 'bar', 'one' => 'two',));
+    $options_query_reverse = array('query' => array('one' => 'two', 'foo' => 'bar',));

The trailing commas shouldn't be here, or it should be multiline

thedavidmeister’s picture

Status: Needs work » Needs review
FileSize
6.12 KB
29.83 KB

patch for #71

tim.plunkett’s picture

Assigned: thedavidmeister » sun

Thanks! That was fast. I'm assigning to sun, because I'm pretty sure he will have an opinion about this. I hope its a good one.

Crell’s picture

Status: Needs review » Needs work
+++ b/core/modules/language/tests/language_test/language_test.module
@@ -109,6 +109,16 @@ function language_test_menu() {
+  $items['language_test/l-active-class'] = array(
+    'page callback' => 'language_test_l_active_class',
+    'access callback' => TRUE,
+    'type' => MENU_CALLBACK,
+  );
+  $items['language_test/theme-link-active-class'] = array(
+    'page callback' => 'language_test_theme_link_active_class',
+    'access callback' => TRUE,
+    'type' => MENU_CALLBACK,
+  );

Now that the new controllers are in place and the conversion is in progress, we should not be adding any more old-style routes. These and the page callback functions below should be implemented as routes and controllers. In fact, since these are MENU_CALLBACK there's no need for hook_menu for them at all.

See the newly minted docs: http://drupal.org/node/1953342

thedavidmeister’s picture

#74 - mmk, I'll have a look at that conversion process nowish.. 26th of March, that really is "newly minted" :P

I'm leaving this assigned to Sun though until we get his opinion on the matter.

thedavidmeister’s picture

Status: Needs work » Needs review
FileSize
7.73 KB
30.78 KB

This is my attempt at the WSCII conversion that Crell was talking about in #74. I wouldn't be surprised if I got all or some of it wrong though, it's my first time working with the new system. I basically just copied and pasted the tutorial examples...

One thing that I am suss about is:

  requirements:
    _permission: 'true'

has the same effect as

  requirements:
    _permission: 'foofoofoo'

But omitting the requirements altogether resulted in a 403 on my local. What's the current standard for setting an "access callback" to TRUE in d8?

Status: Needs review » Needs work

The last submitted patch, 1922454-gut-l-fix-theme-link-76.patch, failed testing.

thedavidmeister’s picture

Oops.

I got some advice in #drupal-wscci IRC and re-rolled #76 with tests now coming back green locally.

thedavidmeister’s picture

Status: Needs work » Needs review
thedavidmeister’s picture

Status: Needs review » Needs work

I just realised I did the same thing (used hook_menu) in common_test.module.

thedavidmeister’s picture

Status: Needs work » Needs review
FileSize
7.77 KB
33.52 KB

Hopefully this is better. No more hook_menu().

thedavidmeister’s picture

Status: Needs review » Needs work

It just dawned on me that the main reason I wanted to do this, which was allowing renderable arrays as content for links to preserve drillable data in Twig templates isn't satisfied by #81 - l() hates array('#markup' => 'foo') passed in for '#text'. We lost that functionality when we pulled out the Twig template and I didn't replace it in theme_link() in #70 (or write any tests for it!).

Need to have a bit of a think about the best way to get theme('link') to play nice without overcomplicating it...

thedavidmeister’s picture

Status: Needs work » Needs review
FileSize
2.34 KB
34.63 KB
tstoeckler’s picture

The interdiff looks good, IMO. Haven't dug deep enough in this issue to RTBC myself, but it has my endorsement :-)

steveoliver’s picture

A few nitpick docs changes and renamed one akward class name.

Also, is it true that these new active class tests are the first to test i.e.:

1. on page /en/language_test/l-active-class
2. link to /language_test/l-active-class is marked active

I believe this is the first time we're testing this. If so, this is an addition to the changes already pointed out in the issue summary.

If I wasn't posting changes, I'd mark this RTBC again. It's totally ready to go.

thedavidmeister’s picture

#85 - The language switcher tests already implicitly checks l() for active classes on links - see the failed test in #5. All I've done is added some more tests with the same logic to cover "rendering links while switching languages" rather than "the language switcher which has links". Thanks for the documentation updates.

steveoliver’s picture

Issue tags: +needs profiling

As discussed in Hangout today, let's profile both l() and theme_link() before and after this patch:

  • Home page (/node) with 50 nodes & Recent Comments block
  • An administrative page with ~50 users:
    1. before: l()
    2. before: l() + template_preprocess/template implementation
    3. after: l()
    4. after: l() + module_handler alter implementation
    5. after: theme_link() implementation
  • One page with ~50 links generated by l() / #type 'link'
  • One page with ~50 links generated by theme_link() / #theme 'link'

*If there are major performance regressions:
- url_is_active helper function being called multiple times? Inline contents of the function back into l().
- it may be impacted by Drupal::service('module_handler')->alter()

Fabianx’s picture

Okay, we live reviewed that in Twig Hangout today and it is ready for **RTBC** from my perspective.

Besides the profiling, which I will do ASAP.

webchick’s picture

Also I just want to chime in here and saw W-O-W regarding all the awesome test coverage here! You all are doing amazing work! :D

thedavidmeister’s picture

#88 - @Fabianx, if you're doing some profiling would you mind also comparing the speed of the Twig version last seen in #63?

It would be nice to know how theme functions compare against templates in this case, for the sake of future conversions.

Fabianx’s picture

#90: Yes, will try to do that.

thedavidmeister’s picture

#91 - @Fabianx would you know a rough ETA on the profiling?

I've been looking around the patches coming up for the conversion tasks and there's a bunch that are already implementing '#theme' => 'link' style arrays, which is awkward :S

catch’s picture

To make markup cacheable when it has links in it we'll need to handle the active class differently (I was starting to wonder if we could remove that completely and just rely on what the browser provides, or add it via js). So if url_is_active() contributes to the slow down here then it might be worth considering revisiting that overall.

thedavidmeister’s picture

rely on what the browser provides

What is that, specifically?

add it via js

That sounds cool. Do it now or followup?

catch’s picture

Wait the browser doesn't provide anything does it :( I've been using Drupal too long.

Doing it in js probably a follow-up unless the active checking is causing a serious performance issue here.

But since nearly all rendered content includes links run through l(), this is a direct blocker to #1830598: [meta] support render caching properly (entity reference links, menu blocks etc. all run into this).

thedavidmeister’s picture

#95 - The plot thickens. again. :/

I'd like to do the js thing as a followup if possible (and it sounds very cool, assuming we don't need l() in contexts other than HTML - like JSON). We're not creating a new problem that wasn't there before in this patch. When you say:

this is a direct blocker to

You mean l() in general is, not this patch specifically right?

catch’s picture

Yep just l() in general.

star-szr’s picture

Assigned: sun » star-szr

@Fabianx is helping me work on the profiling for this.

thedavidmeister’s picture

#98 - @Cottser, when you're profiling Twig templates for theme('link') vs. theme functions vs. l(), #1961876: Convert theme_link() to Twig is probably a better source for an example of a Twig template for links than #63 (which I previously suggested in #90).

star-szr’s picture

Title: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of twig. » Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig
Assigned: star-szr » Unassigned
Issue tags: -needs profiling
FileSize
34.97 KB
967 bytes
817 bytes

Summary

  • l() is roughly 1% slower.
  • theme_link() is about 60% faster.
  • Alter hook implementations are fast.

Testing process

I used @Fabianx’s xhprof-kit for profiling.

I set the default theme to Stark, added 50 nodes with devel generate, and then added 50 links to these nodes by adding a for loop in page.tpl.php - the links are either printed by l() or theme_link(), depending on the test. I then set the site homepage to node/1.

For the alter hook I added node_alter_link() to change the link path for all links.

The profiling results refer to local git branches (e.g. head-l, gut-l-85), and I’ve attached patches for a few of these branches to show the testing code used.

Results

HEAD vs. #85:

(links generated by l())

=== head-l..gut-l-85 compared (517c1770a3264..517c1836bbd84):

ct : 35,235|35,747|512|1.5%
wt : 180,226|181,813|1,587|0.9%
cpu : 161,192|162,713|1,521|0.9%
mu : 10,863,848|10,857,800|-6,048|-0.1%
pmu : 10,977,752|10,971,696|-6,056|-0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=517c1770a3264&...

HEAD vs. #85:

(links generated by theme_link())

=== head-theme-link..gut-l-85-theme-link compared (517c1bb406b02..517c1c27a12c9):

ct : 58,995|36,547|-22,448|-38.1%
wt : 466,540|182,026|-284,514|-61.0%
cpu : 279,177|162,564|-116,613|-41.8%
mu : 11,154,056|10,709,000|-445,056|-4.0%
pmu : 11,392,288|10,822,968|-569,320|-5.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=517c1bb406b02&...

HEAD vs. #85 + Twig conversion (#1961876: Convert theme_link() to Twig):

(links generated by theme_link())

=== head-theme-link..gut-l-85-twig-theme-link compared (517c75d3de7c7..517c76da9b0d3):

ct : 58,995|39,784|-19,211|-32.6%
wt : 464,718|199,334|-265,384|-57.1%
cpu : 279,173|179,954|-99,219|-35.5%
mu : 12,450,728|12,207,616|-243,112|-2.0%
pmu : 12,691,568|12,338,448|-353,120|-2.8%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=517c75d3de7c7&...

Alter hook - #85 vs. #85 with node_link_alter()

(links generated by l())

=== gut-l-85..gut-l-85-alter compared (517c7d73caad2..517c7dc1dc4c3):

ct : 35,747|35,802|55|0.2%
wt : 202,452|202,805|353|0.2%
cpu : 181,401|182,337|936|0.5%
mu : 13,202,008|13,204,488|2,480|0.0%
pmu : 13,331,784|13,334,240|2,456|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=517c7d73caad2&...

[Slightly tweaked the title as well.]

thedavidmeister’s picture

#100 - I'm not sure that I understand how theme_link() could possibly have gotten faster :/

When you were comparing theme_link() against HEAD are these the two scenarios:

  • HEAD is generating links with l() but there's a theme function currently implemented so l() detects that and calls theme_link() "intelligently".
  • In the patch, links are generated with a '#theme' => 'link' style renderable array.

?

What I find really interesting though is that the inline docs for l() in HEAD make this claim:

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

But unless I'm reading the figures wrong, this profiling indicates that without this patch, generating links through the theme system actually takes about 2.6x (466/180) longer than l() without any overrides, which is a slowdown of 160% - about 8x what is currently documented, leading to a 16% increase in the total page request time for a site spending 10% on l()!

We can also see that this patch, while adding 0.89% to l(), means that theme_link() implementations would only add a little under 1% (182/180.2) to l() in HEAD and 0.1% (182/181.8) to l() in #85 (instead of 160%) - making theme_link() much more palatable. This is a bit misleading though as the Twig implementation would almost immediately follow, leading to theme_link() with a Twig template being 10.6% (199.3/180.2) slower than l() in HEAD and 9.6% (199.3/181.8) slower than l() in #85 - less than half the overhead currently quoted in the docs.

I think then, since there *is* a measurable slowdown on l() when #85 is applied, it would be nice to see how much speed we can get back by moving url_is_active() back to inline logic within l() - even if that means duplicating it in the preprocess during Twig conversion. If we can get l() in this issue on par with l() in HEAD even with the alter while simultaneously getting a dramatic performance boost in theme_link() post-Twig vs. HEAD... well, that would just be RTBC for this wouldn't it?

thedavidmeister’s picture

oh, btw @Cottser, thank you very very much for doing this profiling.

thedavidmeister’s picture

Assigned: Unassigned » thedavidmeister

I'm re-rolling a patch with url_is_active() inlined.

thedavidmeister’s picture

Assigned: thedavidmeister » Unassigned
FileSize
33.06 KB

Here we go. Tests are failing locally, but it seems to be because after I pulled url() stopped working for port 8888 with MAMP... I'll see what d.o. testbots think.

thedavidmeister’s picture

FileSize
2.76 KB
thedavidmeister’s picture

Issue tags: +needs profiling

yay, the testbots are happy.

If we could get profiling for just l() in #104 vs HEAD so we can see what impact url_is_active() itself has on performance, that would be amazing.

thedavidmeister’s picture

Status: Needs review » Needs work

Ah, I just realised that the original implementation of $is_active is faster and at some point we slowed it down.

In #104 we check path, language and query all independently but HEAD only checks language and query if path and then language match, which would be faster for every link that *isn't* active based on the path - likely most of them.

star-szr’s picture

Status: Needs review » Needs work

I missed a thing (or three) for the profiling, taking another go at it today. My head-theme-link branch was throwing errors for one and I didn't realize.

Edit: It's also worth mentioning that the Twig implementation already has a couple optimizations in progress which would improve the numbers, one RTBC:
#1979290: Add static caching to Twig_Environment::getTemplateClass()
#1938430: Don't add a default theme hook class in template_preprocess()

thedavidmeister’s picture

Status: Needs work » Needs review
FileSize
2.05 KB
33.74 KB

Hopefully this is a little faster (and still works).

Fabianx’s picture

Status: Needs work » Needs review
star-szr’s picture

Issue tags: -needs profiling

Here is round 2 of the profiling.

Summary

Tested 200 links instead of 50:

  • l() is about 1% slower for 200 links which is a big improvement from being 1% slower for 50 links.
  • theme_link() is about 4% slower, but this is due to theme_link() now being correct - e.g. adding active class.
  • For altering link properties, hook_link_alter() introduced in this patch is almost 4% faster than the previous template_preprocess() implementation.

Results

HEAD vs. #110, links generated by l()

=== head-l..gut-l-110 compared (517d51ae434de..517d52b5a5611):

ct  : 41,535|42,535|1,000|2.4%
wt  : 208,918|211,344|2,426|1.2%
cpu : 189,930|191,326|1,396|0.7%
mu  : 13,212,688|13,202,432|-10,256|-0.1%
pmu : 13,347,992|13,337,776|-10,216|-0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=517d51ae434de&...

HEAD vs. #110, links generated by theme_link()

=== head-theme-link..gut-l-110-theme-link compared (517d750163e11..517d7587ada9c):

ct  : 42,330|45,735|3,405|8.0%
wt  : 217,172|225,343|8,171|3.8%
cpu : 196,787|206,447|9,660|4.9%
mu  : 13,217,264|13,206,192|-11,072|-0.1%
pmu : 13,353,376|13,342,328|-11,048|-0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=517d750163e11&...

HEAD - template_preprocess_link() implementation vs. #110 - hook_link_alter() implementation

(links generated by l())

Both implementations change the path of all links to node/2.

=== head-l-preprocess..gut-l-110-alter compared (517d7319aefb8..517d73f55c548):

ct  : 45,010|42,749|-2,261|-5.0%
wt  : 223,123|214,807|-8,316|-3.7%
cpu : 202,474|193,886|-8,588|-4.2%
mu  : 13,223,504|13,204,792|-18,712|-0.1%
pmu : 13,357,832|13,338,968|-18,864|-0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=517d7319aefb8&...

star-szr’s picture

Issue summary: View changes

Add TL;DR section as the description is really long

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

With this profiling done, I can finally RTBC this!

Many thanks to Cottser, thedavidmeister and steveoliver!

This is RTBC, because:

* it has very very extensive test coverage to fix new bugs encountered in core
* it has been group reviewed during a live hangout.
* the performance is:
** better for the altering case
** about the same (within error margin) for the l() case
** worse for theme_link, but that is because theme_link is now consistent with all other link type functions.

The stated goals of:

* l() being always fast (while keeping flexible)
* theme_link being correct

have been reached!

Great work!

RTBC

webchick’s picture

Assigned: Unassigned » catch

GREAT work, folks!

Due to the performance-sensitive nature of this, I think it's best to assign to catch for sign-off here.

thedavidmeister’s picture

Been thinking a little more about keeping l() fast. What do you guys think about statically caching current_path(), drupal_is_front_page(), language(LANGUAGE_TYPE_URL)->langcode and Drupal::service('request')->query->all() when checking "active"?

thedavidmeister’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.88 KB
34.13 KB

I'm just curious to see how this goes...

thedavidmeister’s picture

Issue tags: +needs profiling
FileSize
894 bytes
34.25 KB

@Fabianx would like to use drupal_static() here. Really preliminary benchmarking (just running a loop with l() and timer_start() and timer_stop()) shows that the static variables here improve the speed of l() by about 5-10%-ish

thedavidmeister’s picture

FileSize
805 bytes
34.26 KB

Sorry for all the patches. Whitespace fix.

star-szr’s picture

I like the idea of adding more static caching where it makes sense. drupal_is_front_page() already has drupal_static_fast caching. Probably worth profiling that to see what the effect is of avoiding those function calls altogether.

thedavidmeister’s picture

#119 - I have a feeling that all of those functions are statically cached. In #118 I'm just trying to avoid function calls altogether to see how much of a difference it makes - for the same reason that we "inlined" url_is_active().

star-szr’s picture

Issue tags: -needs profiling

Okay, I think we're talking now, great work @thedavidmeister!

Round 3 results:

Summary

Tested 200 links:

  • l() is 0.5% faster for 200 links!
  • theme_link() is about 2% slower, but this is due to theme_link() now being correct - e.g. adding active class.
  • For altering link properties, hook_link_alter() introduced in this patch is almost 5% faster than the previous template_preprocess() implementation.

Results

HEAD vs. #118, links generated by l()

=== head-l..gut-l-118 compared (518080a5ca60d..518081a41c05e):

ct  : 41,752|40,728|-1,024|-2.5%
wt  : 231,723|230,504|-1,219|-0.5%
cpu : 211,573|210,277|-1,296|-0.6%
mu  : 16,129,240|16,124,920|-4,320|-0.0%
pmu : 16,206,160|16,201,832|-4,328|-0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=518080a5ca60d&...

HEAD vs. #118, links generated by theme_link()

=== head-theme-link..gut-l-118-theme-link compared (518084f28082c..51808553c131f):

ct  : 42,547|43,928|1,381|3.2%
wt  : 237,987|242,958|4,971|2.1%
cpu : 217,944|222,520|4,576|2.1%
mu  : 16,133,424|16,128,392|-5,032|-0.0%
pmu : 16,210,360|16,205,496|-4,864|-0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=518084f28082c&...

HEAD - template_preprocess_link() implementation vs. #118 - hook_link_alter() implementation

(links generated by l())

Both implementations change the path of all links to node/2.

=== head-l-preprocess..gut-l-118-alter compared (5180892944f6b..51808a75c5e76):

ct  : 45,227|40,942|-4,285|-9.5%
wt  : 244,539|232,684|-11,855|-4.8%
cpu : 224,396|212,486|-11,910|-5.3%
mu  : 16,139,624|16,127,160|-12,464|-0.1%
pmu : 16,215,232|16,202,840|-12,392|-0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5180892944f6b&...

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Wow, great work! I am blown away from this great work. The profiling results speak for themselves:

Repeating my RTBC from #113 for sake of simplicity:

Many thanks to Cottser, thedavidmeister and steveoliver!

This is RTBC, because:

* it has very very extensive test coverage to fix new bugs encountered in core
* it has been group reviewed during a live hangout.
* the performance is:
** 5% better for the altering case than the old preprocess way
** 0.5% better for the normal l() case - and saves 1000 function calls
** only 2.1% worse for theme_link, but that is because theme_link is now consistent with all other link type functions and is actually _correct_ now.

The stated goals of:

* l() being always fast (while keeping flexible)
* theme_link being correct

have been reached!

And due to the recent optimization the new l() is even faster than the old l().

Absolutely fantastic work!

I can only say:

RTBC

catch’s picture

Status: Reviewed & tested by the community » Fixed

I've opened #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links to look at taking the active class adding out of l() altogether.

For now keeping the performance comparable while fixing the behaviour is good. I was mildly concerned about subrequests with the static caching but the active class is explicitly global and if we do #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links it'll go anyway.

Committed/pushed to 8.x, thanks!

c4rl’s picture

I'd like to fix some API inconsistencies that maybe aren't relevant to this issue in particular, but I noticed while perusing this issue.

#theme => link uses #path and #text while #type => link uses #title and #href. Unless there's a specific reason for these differences, I'd like to open up a separate issue to consolidate these.

+++ b/core/modules/language/tests/language_test/lib/Drupal/language_test/Controller/LanguageTestController.phpundefined
@@ -0,0 +1,113 @@
+      'en' => array(
+        '#theme' => 'link',
+        '#text' => t('Link to an English version of the current path.'),
+        '#path' => current_path(),
+        '#options' => array(
+          'language' => $languages['en'],
+          'attributes' => array(
+            'id' => 'en_link',
+          ),
+        ),
+      ),
+++ b/core/modules/language/tests/language_test/lib/Drupal/language_test/Controller/LanguageTestController.phpundefined
@@ -0,0 +1,113 @@
+      'en' => array(
+        '#type' => 'link',
+        '#title' => t('Link to an English version of the current path.'),
+        '#href' => current_path(),
+        '#options' => array(
+          'language' => $languages['en'],
+          'attributes' => array(
+            'id' => 'en_link',
+          ),
+        ),
+      ),
+    );
webchick’s picture

Issue tags: +Needs followup

Yes, please open up a follow-up to change those to #that-thingy-up-at-the-top and #them-little-blue-words-i-click-on.

(j/k. ;) +1, good idea.)

c4rl’s picture

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.