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:
- 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.
- Use l() in the preprocess function wherever possible to generate a rendered string and pass it to the template, allow l() to be used in Twig templates as a function. l() is the fastest way we have of producing a consistently formatted string with all required sanitisation and attribute/class handling to pass tests, it's also the way we've always handled themed links in core so would require the least work to implement.
- If the contents of the link is more complex than plain text, especially if the contents of the link is desired to be a render array all the way to the template, using l() is severely limiting inside the preprocess function.
- To use l() in a Twig template when render arrays are passed into the template, l() will need to be extended (made as slow as theme('link')) to understand them, making it equivalent to theme('link') but with uglier, less "twigy" syntax.
- l() currently can "decide" whether or not to make itself theme-able via theme('link') depending on a system setting and/or whether theme override files currently exist on the system. This means that if a site builder wants to make even *one* link on their site themable, they need to accept the lower performance version of l() for all links. It really isn't obvious to anyone who hasn't read the source code of l() that this is the case.
- From what I've read in #drupal-twig in IRC, using a theme function, or a function that calls a theme function inside a Twig template is pretty frowned upon.
-
Use theme('link') alongside render arrays to produce link markup in Twig templates. This guarantees extensibility and consistency of all links across the board, it also allows a developer to wrap a render array in a link and preserve the data structure through to the template.
- Using a theme function in Drupal 7 vs. calling l() was clocked at ~20% slower in Drupal 7 during preliminary benchmarks, with link-heavy sites seeing an overall performance hit of ~2% on a page load. How much slower is a Drupal 8 Twig template + preprocess than a Drupal 7 theme function? well nobody knows yet, or at least nobodys published anything useful anywhere I've seen... Let's just say "theme('link') is slow and l() is fast".
- Currently theme('link') doesn't actually work, in that it *assumes that it will be called by l() every time*. Therefore it currently does no sanitisation of urls or processing/translation of the "active" class. It simply isn't useful in it's present state #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.
- Add drupal_alter('l') to l() and theme_link() - Couldn't see an existing issue for this, was suggested originally by Fabinx in IRC.
- Don't let l() try to "get smart" with theme('link'), make it fast and dumb like it used to be #1778610: Remove the check for a link template from l(), have l() always output just a string., #1833920: [META] Markup Utility Functions.
- Once l() is "dumb", expose it to Twig templates as a function. If it can't call theme functions there's no reason not to allow themers access to the .active class and extra sanitisation provided by l().
- Make theme_link() able to render a link at least as well as l() currently can #1778616: Add an .active class for links rendered by theme('link') that have an href to the URL being viewed and sanitize output of url()
- Let people know that this is how we want to move forward #1812562: Create best practices about use of the l() function and the url() function in templates
- Clean up all the weirdly similar theme_X_link() functions in core to be either a flat l() or a template #1595614: [meta] Remove all the theme functions and templates in core that simply output a link. Replace with #type 'link' render arrays
- Close this as "won't fix" #1187032: theme_link needs to be clearer about saying not to call it directly
User interface changes
none
API changes
- Developers will need to make a conscious decision as to whether the content or context of a link justifies a fully themable element or whether it would be better as the traditional and faster l() call.
- There will be a new alter hook for modifying the structure (not the markup) of all links whether generated by l() or through templates.
- theme('link') will be useful.
- l() will no longer invoke theme functions.
- l() will be available from within Twig templates.
Comment | File | Size | Author |
---|---|---|---|
#118 | 1922454-118.patch | 34.26 KB | thedavidmeister |
#118 | interdiff-117-118.txt | 805 bytes | thedavidmeister |
#117 | 1922454-117.patch | 34.25 KB | thedavidmeister |
#117 | interdiff-116-117.txt | 894 bytes | thedavidmeister |
#116 | 1922454-116.patch | 34.13 KB | thedavidmeister |
Comments
Comment #1
thedavidmeister CreditAttribution: thedavidmeister commentedsorry, this is a meta issue.
Comment #2
jenlamptonI disagree with 3. Can't we just remove theme_link entirely? I don't see any good reason to include a template here.
Comment #3
thedavidmeister CreditAttribution: thedavidmeister commentedI 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 :)
Comment #4
Fabianx CreditAttribution: Fabianx commented#2: Usage within render arrays, also paving the way to more complex structures later.
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:
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:
or
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.
Comment #5
thedavidmeister CreditAttribution: thedavidmeister commentedrough patch attached. At the very least it needs extra documentation.
Comment #7
Fabianx CreditAttribution: Fabianx commentedUhm, this change is probable not wanted ...
Otherwise looks on first glance good to me.
Comment #8
thedavidmeister CreditAttribution: thedavidmeister commented@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 :/
Comment #9
jenlamptonI'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?
Comment #10
thedavidmeister CreditAttribution: thedavidmeister commented@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?
Comment #11
Fabianx CreditAttribution: Fabianx commented#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:
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!
Comment #12
thedavidmeister CreditAttribution: thedavidmeister commentedOk, 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 :/
Comment #13
jenlamptonBaby steps++ I'll go make noise in the other issues then :) Carry on!
P.S. you both rock. :)
Comment #14
Fabianx CreditAttribution: Fabianx commentedYes, 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.
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.
Comment #15
thedavidmeister CreditAttribution: thedavidmeister commentedAdded documentation to link.html.twig, Fixed #7, hopefully fixed the language switching test fails.
Comment #17
Fabianx CreditAttribution: Fabianx commented#15: 1922454-gut-l-fix-theme-link-15.patch queued for re-testing.
Comment #18
sunI 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?Comment #19
Fabianx CreditAttribution: Fabianx commented#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.
Comment #20
thedavidmeister CreditAttribution: thedavidmeister commented#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:
I didn't see any reason to make more work for everyone following that thread.
Comment #21
thedavidmeister CreditAttribution: thedavidmeister commented@jenlampton, In #1812562: Create best practices about use of the l() function and the url() function in templates you said:
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.
Comment #22
dcrocks CreditAttribution: dcrocks commentedAlso 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. ) are necessary for the global data-* attribute. Currently it is impossible to implement the data-* attribute on menu links.
Comment #23
thedavidmeister CreditAttribution: thedavidmeister commented@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():
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?
Comment #24
dcrocks CreditAttribution: dcrocks commentedMy 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.
Comment #25
thedavidmeister CreditAttribution: thedavidmeister commented#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().
Comment #26
dcrocks CreditAttribution: dcrocks commentedI have created an issue, #1930322: Character references should not be double encoded while rendering HTML element attributes., to start discussion on this.
Comment #27
steveoliver CreditAttribution: steveoliver commentedOK,
...but I don't think that's related to this.
Comment #28
steveoliver CreditAttribution: steveoliver commented...and a test for for hook_link_alter.
Comment #29
thedavidmeister CreditAttribution: thedavidmeister commentedI 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 :(
Comment #30
Fabianx CreditAttribution: Fabianx commentedThis 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.
Comment #31
tstoeckleras
url_is_active()
is a public function this should account for$options['query']
not being set.Comment #32
steveoliver CreditAttribution: steveoliver commentedAs per #31...
Comment #33
steveoliver CreditAttribution: steveoliver commentedI'll write the tests, probably today...
Comment #34
thedavidmeister CreditAttribution: thedavidmeister commentedrunning tests on #32.
Comment #36
steveoliver CreditAttribution: steveoliver commentedSo.. apparently we allow empty query parameters to url().
Changed !empty() check to isset().
Comment #37
tstoecklerDoesn'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.
Comment #38
steveoliver CreditAttribution: steveoliver commentedFirst, answering @tstoekler's comments in #31 and #37:
@tstoeckler: re #37:
$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.
Comment #40
steveoliver CreditAttribution: steveoliver commentedHaha -- woops, this is diff is actually backwards. Swap - with + ;)
Comment #41
steveoliver CreditAttribution: steveoliver commentedOK, both were backwards.
Here we go. Sorry for all the noise.
Comment #42
thedavidmeister CreditAttribution: thedavidmeister commented#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().
Comment #43
tstoecklerWe should also have some unit-y tests that test url_is_active() directly.
I.e.
Comment #44
tstoecklerOh, and also:
Sorry, that I'm obsessing so much about this line, but this will be true if
$options['query']
isFALSE
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.Comment #45
steveoliver CreditAttribution: steveoliver commented#42, oh that's right.
#43, yeah, right, since it can be called outside of l/theme('link')
#44, yeah, strict check it better.
Comment #46
steveoliver CreditAttribution: steveoliver commentedSo check this out:
Here's the page callback for UrlTest, which tests for active links on classes:
api docs
Being tested by this:
api docs
Yet a but it's theme('link') ('#type' => 'link') that's producing those links for the page callback:
(theme.inc)
Auuuh?
Comment #47
steveoliver CreditAttribution: steveoliver commentedI 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...
Comment #48
nevets CreditAttribution: nevets commentedI 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.
Comment #49
thedavidmeister CreditAttribution: thedavidmeister commented#48 - nobody is suggesting we use a tags without using url() to generate the href first :)
Comment #50
steveoliver CreditAttribution: steveoliver commentedRegarding 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
Comment #51
thedavidmeister CreditAttribution: thedavidmeister commented#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?
Comment #52
steveoliver CreditAttribution: steveoliver commented#51, Yes, let's move this forward. Unassigning myself as I've got to get some actual work-work done today.
Comment #53
thedavidmeister CreditAttribution: thedavidmeister commentedhaving a look at this today.
Comment #54
thedavidmeister CreditAttribution: thedavidmeister commentedBuilding 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.phpThis 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.
Comment #55
thedavidmeister CreditAttribution: thedavidmeister commented#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
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.
Comment #56
thedavidmeister CreditAttribution: thedavidmeister commentedThis 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.
Comment #57
tstoeckler@thedavidmeister: I guess that's a fair point. A little array sorting wouldn't hurt either IMO, but I don't really care either.
Comment #58
dcrocks CreditAttribution: dcrocks commentedA 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.
Comment #59
thedavidmeister CreditAttribution: thedavidmeister commented#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 :)
Comment #60
dcrocks CreditAttribution: dcrocks commentedThanx 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.
Comment #61
thedavidmeister CreditAttribution: thedavidmeister commented#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().
Comment #62
thedavidmeister CreditAttribution: thedavidmeister commentedwriting some tests atm.
Comment #63
thedavidmeister CreditAttribution: thedavidmeister commentedThis 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...
Comment #64
Fabianx CreditAttribution: Fabianx commented* 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!
Comment #65
thedavidmeister CreditAttribution: thedavidmeister commented#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?
Comment #66
steveoliver CreditAttribution: steveoliver commentedWe'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.
Comment #67
thedavidmeister CreditAttribution: thedavidmeister commented#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 :)
Comment #68
thedavidmeister CreditAttribution: thedavidmeister commentedI'm looking at this now.
Comment #69
thedavidmeister CreditAttribution: thedavidmeister commentedWithout 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:
Comment #70
Fabianx CreditAttribution: Fabianx commentedInteresting.
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
Comment #71
tim.plunkettWow, this is intense. Just a couple nitpicks before commit.
No blank line between @params
URL, not url
These extra parentheses around the ternary aren't a common pattern in core, can they be removed?
Drupal::service('request')->query->all()
Drupal::service('module_handler')->alter('link', $variables);
I'm surprised this works, ID is supposed to be singular.
Should just use drupal_render()
The trailing commas shouldn't be here, or it should be multiline
Comment #72
thedavidmeister CreditAttribution: thedavidmeister commentedpatch for #71
Comment #73
tim.plunkettThanks! 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.
Comment #74
Crell CreditAttribution: Crell commentedNow 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
Comment #75
thedavidmeister CreditAttribution: thedavidmeister commented#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.
Comment #76
thedavidmeister CreditAttribution: thedavidmeister commentedThis 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:
has the same effect as
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?
Comment #78
thedavidmeister CreditAttribution: thedavidmeister commentedOops.
I got some advice in #drupal-wscci IRC and re-rolled #76 with tests now coming back green locally.
Comment #79
thedavidmeister CreditAttribution: thedavidmeister commentedComment #80
thedavidmeister CreditAttribution: thedavidmeister commentedI just realised I did the same thing (used hook_menu) in common_test.module.
Comment #81
thedavidmeister CreditAttribution: thedavidmeister commentedHopefully this is better. No more hook_menu().
Comment #82
thedavidmeister CreditAttribution: thedavidmeister commentedIt 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...
Comment #83
thedavidmeister CreditAttribution: thedavidmeister commentedComment #84
tstoecklerThe interdiff looks good, IMO. Haven't dug deep enough in this issue to RTBC myself, but it has my endorsement :-)
Comment #85
steveoliver CreditAttribution: steveoliver commentedA 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.
Comment #86
thedavidmeister CreditAttribution: thedavidmeister commented#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.
Comment #87
steveoliver CreditAttribution: steveoliver commentedAs discussed in Hangout today, let's profile both l() and theme_link() before and after this patch:
*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()
Comment #88
Fabianx CreditAttribution: Fabianx commentedOkay, 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.
Comment #89
webchickAlso 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
Comment #90
thedavidmeister CreditAttribution: thedavidmeister commented#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.
Comment #91
Fabianx CreditAttribution: Fabianx commented#90: Yes, will try to do that.
Comment #92
thedavidmeister CreditAttribution: thedavidmeister commented#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
Comment #93
catchTo 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.
Comment #94
thedavidmeister CreditAttribution: thedavidmeister commentedWhat is that, specifically?
That sounds cool. Do it now or followup?
Comment #95
catchWait 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).
Comment #96
thedavidmeister CreditAttribution: thedavidmeister commented#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:
You mean l() in general is, not this patch specifically right?
Comment #97
catchYep just l() in general.
Comment #98
star-szr@Fabianx is helping me work on the profiling for this.
Comment #99
thedavidmeister CreditAttribution: thedavidmeister commented#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).
Comment #100
star-szrSummary
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()
)http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=517c1770a3264&...
HEAD vs. #85:
(links generated by
theme_link()
)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()
)http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=517c75d3de7c7&...
Alter hook - #85 vs. #85 with node_link_alter()
(links generated by
l()
)http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=517c7d73caad2&...
[Slightly tweaked the title as well.]
Comment #101
thedavidmeister CreditAttribution: thedavidmeister commented#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:
?
What I find really interesting though is that the inline docs for l() in HEAD make this claim:
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?
Comment #102
thedavidmeister CreditAttribution: thedavidmeister commentedoh, btw @Cottser, thank you very very much for doing this profiling.
Comment #103
thedavidmeister CreditAttribution: thedavidmeister commentedI'm re-rolling a patch with url_is_active() inlined.
Comment #104
thedavidmeister CreditAttribution: thedavidmeister commentedHere 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.
Comment #105
thedavidmeister CreditAttribution: thedavidmeister commentedComment #106
thedavidmeister CreditAttribution: thedavidmeister commentedyay, 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.
Comment #107
thedavidmeister CreditAttribution: thedavidmeister commentedAlso relevant #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links.
Comment #108
thedavidmeister CreditAttribution: thedavidmeister commentedAh, 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.
Comment #109
star-szrI 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()
Comment #110
thedavidmeister CreditAttribution: thedavidmeister commentedHopefully this is a little faster (and still works).
Comment #111
Fabianx CreditAttribution: Fabianx commented+1 for #110. Benchmarks (http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?symbol=l&run1=517d5..., http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?symbol=l&run1=517d5..., http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=517d51ae434de&...) show that the inlined and re-optimized url_is_active is better.
Benchmarks are using #110 going forward.
Comment #112
star-szrHere 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.hook_link_alter()
introduced in this patch is almost 4% faster than the previoustemplate_preprocess()
implementation.Results
HEAD vs. #110, links generated by
l()
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=517d51ae434de&...
HEAD vs. #110, links generated by
theme_link()
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.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=517d7319aefb8&...
Comment #112.0
star-szrAdd TL;DR section as the description is really long
Comment #113
Fabianx CreditAttribution: Fabianx commentedWith 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
Comment #114
webchickGREAT work, folks!
Due to the performance-sensitive nature of this, I think it's best to assign to catch for sign-off here.
Comment #115
thedavidmeister CreditAttribution: thedavidmeister commentedBeen 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"?
Comment #116
thedavidmeister CreditAttribution: thedavidmeister commentedI'm just curious to see how this goes...
Comment #117
thedavidmeister CreditAttribution: thedavidmeister commented@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
Comment #118
thedavidmeister CreditAttribution: thedavidmeister commentedSorry for all the patches. Whitespace fix.
Comment #119
star-szrI 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.
Comment #120
thedavidmeister CreditAttribution: thedavidmeister commented#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().
Comment #121
star-szrOkay, 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.hook_link_alter()
introduced in this patch is almost 5% faster than the previoustemplate_preprocess()
implementation.Results
HEAD vs. #118, links generated by
l()
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=518080a5ca60d&...
HEAD vs. #118, links generated by
theme_link()
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.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5180892944f6b&...
Comment #122
Fabianx CreditAttribution: Fabianx commentedWow, 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
Comment #123
catchI'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!
Comment #124
c4rl CreditAttribution: c4rl commentedI'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.
Comment #125
webchickYes, 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.)
Comment #126
c4rl CreditAttribution: c4rl commentedPer #124, Follow-up: #1985470: Remove theme_link()
Comment #127.0
(not verified) CreditAttribution: commentedUpdated issue summary.