Problem/Motivation
There is currently duplication between l(), theme_link() and theme_more_link(). Attempting to consolidate through a common function (like url_is_active) that all three call introduces a performance drop due to l() being called hundreds of times and having an extra function call.
Proposed resolution
Add a (render=>FALSE) flag to the options supplied to l() that causes l() to return an array of element structure rather than a string. The structured output can then be used directly by preprocess functions for Templates without duplicating the logic of creating a link.
Remaining tasks
- Patch
- Documentation
- Tests
Related Issues
- #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig
- #1961876: Convert theme_link() to Twig
- #1939098: Convert theme_more_link() to Twig
- #1986116: Improve performance by replacing very small and simple templates and theme function with "Markup Utility Functions"
Comment | File | Size | Author |
---|---|---|---|
#12 | 1985974-12.patch | 9.42 KB | thedavidmeister |
#12 | 1985974-interdiff-7-12.txt | 6.52 KB | thedavidmeister |
#7 | interdiff.txt | 1.21 KB | shanethehat |
#7 | l-optional-array-1985974-7.patch | 2.84 KB | shanethehat |
#4 | interdiff.txt | 2.42 KB | shanethehat |
Comments
Comment #0.0
shanethehat CreditAttribution: shanethehat commentedimproving summary
Comment #1
thedavidmeister CreditAttribution: thedavidmeister commentedComment #1.0
thedavidmeister CreditAttribution: thedavidmeister commentedupdated summary
Comment #1.1
Fabianx CreditAttribution: Fabianx commentedAdded proposed flag
Comment #1.2
Fabianx CreditAttribution: Fabianx commentedclarification
Comment #2
shanethehat CreditAttribution: shanethehat commentedNot sure if I've got the array keys right, but this is a start that hopefully won't break anything.
Comment #3
thedavidmeister CreditAttribution: thedavidmeister commentednitpick but i believe that in D8 for mixed return values where we know the different data types we're saying
string|array
instead ofmixed
.Also, rather than try to make assumptions about what parts of the structure of l() the caller is looking for, can we just return $variables as-is post-sanitisation?
Comment #4
shanethehat CreditAttribution: shanethehat commentedMaking it return $variables means that the array needs to be updated with the operations l() performs.
Comment #5
thedavidmeister CreditAttribution: thedavidmeister commented+ $variables['path'] = check_plain(url($variables['path'], $variables['options']));
I feel like rather than overwriting $variables['path'] we should add $variables['url'] and use/return that.
Looking good though :)
Comment #6
shanethehat CreditAttribution: shanethehat commentedJust a thought, but assigning to an array is slightly slower than to a local variable. If we want to prioritise the string output and really get every bit of performance back, should we maybe prepare variables like it did before, and only write them back to $variables if render==
TRUEFALSE? The performance difference is tiny though, so maybe it's not worth worrying about.Speed claims based on: https://gist.github.com/shanethehat/5516588
Edit: had a 50% change of getting it right.
Comment #7
shanethehat CreditAttribution: shanethehat commentedurl key added
Comment #8
thedavidmeister CreditAttribution: thedavidmeister commentedY'know what's weird? Accessing an array once and setting a variable twice is faster than setting an array once, ie:
Is faster than:
Regardless, yeah we should profile #7 before we go off micro-optimising setting variables in different ways. It looks RTBC to me already but without performance stats who knows?
Comment #9
thedavidmeister CreditAttribution: thedavidmeister commented@Fabianx asked for tests in IRC so I'm tagging.
Comment #10
Fabianx CreditAttribution: Fabianx commentedI think any micro-optimization is not worth it here:
200 links (first baseline vs. baseline); 2nd with #7 applied:
Little more memory used and no measurable difference (maybe even slightly faster) ...
=> RTBC (from a performance perspective)
Still needs tests
Comment #11
thedavidmeister CreditAttribution: thedavidmeister commentedwriting tests..
Comment #12
thedavidmeister CreditAttribution: thedavidmeister commentedTests.
Comment #13
thedavidmeister CreditAttribution: thedavidmeister commentedComment #14
Fabianx CreditAttribution: Fabianx commentedRTBC per #10:
Has nice test coverage, makes it possible to use l() to return the structure to re-use for theme_more_link and theme_link, keeps things fast and makes markup-utility functions possible.
=> RTBC
Comment #15
shanethehat CreditAttribution: shanethehat commentedNice looking tests @thedavidmeister, thanks for stepping in with those.
Comment #16
c4rl CreditAttribution: c4rl commentedI find it problematic that this introduces a change to a common API function that returns a different variable type depending on the arguments *as an API.*
The only other place in PHP/Drupal where this really happens is things like strpos(), node_load()/entity_load(), and old-style iterators like mysql_fetch_object() to indicate some sort of exception.
I'd prefer to see this as a helper returning structure for all three l(), theme_link(), theme_more_link() etc, rather than a first-class API function.
Comment #17
joelpittet@c4rl re #16 could you explain your 'helper' idea a bit more?
The problem you explained seems clear, though the solution does not.
A concrete example would probably be best because 'helper' has about as much meaning as 'block' or 'container' does. Also, I heard l() was a 'helper' from a few people... so for me that made things extra confusing.
Comment #18
thedavidmeister CreditAttribution: thedavidmeister commentedI'm having a look at this over the weekend. I think I have a good idea of what @c4rl is talking about in #16.
Comment #19
thedavidmeister CreditAttribution: thedavidmeister commentedI was playing around with this, and I wasn't sure that what I was doing was even useful until a patch is committed elsewhere to formally enable separate prepare/render phases in drupal_render() - essentially the whole "drillability" issue.
All I discovered is that rather than l() returning structured output, it is likely that #type link will have its current internal logic prepended to the logic inside l() inside some "drupal_prepare_link()" style function (excluding the final line that concatenates the link markup together) and that l() will stay the same as it is in HEAD with an extra comment along the lines of "This logic is the same as drupal_prepare_link() but is in-lined for performance reasons".
Comment #20
thedavidmeister CreditAttribution: thedavidmeister commented#2044105: Introduce #alters for \Drupal\Core\Render\Renderer::render() this could be relevant to #type 'link', the "optional structured data" thing could be just running the alter hooks for #type 'link'.
Call me crazy, but l() could one day just be a wrapper around a #type 'link' render array.
Comment #20.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #21
star-szrJust want to bump this and see if it's still relevant in the 'new world' of l() being changed so much.
Comment #22
joelpittetDon't think we have the steam or need to get this in to 8.x.x so postponing to D9 or possibly closing as there may no longer be a need for this.
Comment #23
Fabianx CreditAttribution: Fabianx as a volunteer commentedI think we close that, l() is dead ...