Currently l(), '#theme' => link and '#type' => 'link' all behave differently with respect to renderable arrays being passed as "content" of the link.

Ensuring that this behaviour is both consistent and handled by l() rather than the theme/type functions that call l() is an important prerequisite to implementing #1986116: Improve performance by replacing very small and simple templates and theme function with "Markup Utility Functions".

I'm proposing that we add the following to l() and remove it from theme_link():

if(!is_string($text)) {
  $text = drupal_render($text);
}
Files: 
CommentFileSizeAuthor
#4 1987114-1-4-interdiff.txt657 bytesthedavidmeister
#4 1987114-4.patch3.25 KBthedavidmeister
PASSED: [[SimpleTest]]: [MySQL] 55,628 pass(es).
[ View ]
#1 1987114-1.patch3.25 KBthedavidmeister
FAILED: [[SimpleTest]]: [MySQL] 55,598 pass(es), 0 fail(s), and 435 exception(s).
[ View ]

Comments

thedavidmeister’s picture

Status:Active» Needs review
Issue tags:+Needs profiling, +Twig
StatusFileSize
new3.25 KB
FAILED: [[SimpleTest]]: [MySQL] 55,598 pass(es), 0 fail(s), and 435 exception(s).
[ View ]

Patch attached, it has the extra drupal_render() in l() with a test and updated docs. The extra is_string() call might need profiling.

Fabianx’s picture

Status:Needs work» Reviewed & tested by the community

Benchmarks using 200 links on node/1 page:

* Baseline vs. Baseline to show general difference

=== head-l-200-links-1-node..head-l-200 compared (51865e7532e2c..51865f3c9032e):

ct  : 38,430|38,430|0|0.0%
wt  : 162,683|162,704|21|0.0%
mu  : 16,615,560|16,615,560|0|0.0%
pmu : 16,734,576|16,734,576|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51865e7532e2c&...

Aaand now with #1 applied:

=== head-l-200-links-1-node..render-array-l--1 compared (51865e7532e2c..51865f6e876d3):

ct  : 38,430|38,635|205|0.5%
wt  : 162,683|162,913|230|0.1%
mu  : 16,615,560|16,618,056|2,496|0.0%
pmu : 16,734,576|16,737,024|2,448|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51865e7532e2c&...

0.1% more or 230 ms is well within measuring range and should be an acceptable tradeoff given that we saved more in optimizing l() in "gut l()".

We can probably make theme_link a MUF (markup utility function), which again will help performance then.

Has nice test coverage, removes superfluous check in theme_link, consolidates #type link and #theme link.

=> RTBC (if tests pass)

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

The last submitted patch, 1987114-1.patch, failed testing.

thedavidmeister’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new3.25 KB
PASSED: [[SimpleTest]]: [MySQL] 55,628 pass(es).
[ View ]
new657 bytes

checking for array-ness instead of string-ness to avoid exceptions when drupal_render() chokes on basic PHP data types.

Fabianx’s picture

Status:Needs review» Reviewed & tested by the community

RTBC per #2:

=== head-l-200-links-1-node..render-array-l--4 compared (51865e7532e2c..51867a69ed6b7):

ct  : 38,430|38,635|205|0.5%
wt  : 162,683|163,048|365|0.2%
cpu : 164,011|164,011|0|0.0%
mu  : 16,615,560|16,618,056|2,496|0.0%
pmu : 16,734,576|16,737,024|2,448|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51865e7532e2c&...

A little more, but that is not a surprise as I am just testing strings here ... But still well within measuring error ...

=> RTBC

thedavidmeister’s picture

A little more, but that is not a surprise as I am just testing strings here ...

With the existing codebase this will be the case 100% of the time, so we should optimise for the case where $text is a string, even if we allow arrays.

I opened #1989964: drupal_render() should be able to handle scalar input without throwing exceptions. which would provide a way to use the patch in #1 and keep the testbots happy.

catch’s picture

I like the code here but I'm concerned about the combination of this and the 'return structured output' that it's going to complicate the API for l() a fair bit. Are we sure we want all of this in one function or is it worth looking at splitting it out?

thedavidmeister’s picture

#7 - sure thing, although I don't think this patch complicates the API nearly as much as having l() return structured output (as this patch is only about accepting a wider range of input and doesn't change the return value). Could we move the discussion to #1985974: Make l() optionally return structured output and come back here if needed?

thedavidmeister’s picture

#7 - I think to help discussion though, some example complicated use-cases would be good.

One example I could think of: should we render text with drupal_render() if '#render' (from the other issue) is FALSE? maybe we're supposed to leave that nested structure alone in that case?

thedavidmeister’s picture

I think what I'm trying to say is that l() can't completely replace what theme_link() does currently without this patch, so if we want to get rid of theme_link() and avoid functionality regression then we have to do this (or equivalent).

The l() returning a structure thing might warrant further discussion though, as you said @catch.

alexpott’s picture

Assigned:Unassigned» catch

Just assigning to catch as I feel architectural discussion going on here...

thedavidmeister’s picture

this very loosely (conceptually) ties in with the new efforts at #1843798: [meta] Refactor Render API to be OO

catch’s picture

Status:Reviewed & tested by the community» Fixed

OK I agree the overall API discussion belongs more to #1985974: Make l() optionally return structured output and that this change by itself is OK, committed/pushed this one to 8.x

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