Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Cameron Tod’s picture

Title: Convert variable 'theme_link' to state system » Convert variable 'theme_link' to config system
Cameron Tod’s picture

Component: base system » theme system
Status: Active » Needs review
FileSize
1.49 KB

Here we go.

aspilicious’s picture

What does theme_link do?

Cameron Tod’s picture

l() can either render links inline, or pass through theme_link() Rendering a link inline in l() is faster than using theme_link(). Because l() is so heavily used, using theme_link() can add a lot of overhead to page views, so by setting theme_link to FALSE you can disable theme_link entirely.

Here's the inline comment:

// Determine if rendering of the link is to be done with a theme function
// or the inline default. Inline is faster, but if the theme system has been
// loaded and a module or theme implements a preprocess or process function
// or overrides the theme_link() function, then invoke theme(). Preliminary
// benchmarks indicate that invoking theme() can slow down the l() function
// by 20% or more, and that some of the link-heavy Drupal pages spend more
// than 10% of the total page request time in the l() function.
if (!isset($use_theme) && function_exists('theme')) {
    // Allow edge cases to prevent theme initialization and force inline link
    // rendering.
    if (variable_get('theme_link', TRUE)) {

You can see this comment for effulgentsia's numbers on this.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Ok for me this is good to go in that case.

catch’s picture

Status: Reviewed & tested by the community » Needs review

I'm not sure this should go in the same place as the list of enabled themes, it seems a bit mis-placed there. Bumping back to CNR.

Cameron Tod’s picture

Moved into system.performance.

Cameron Tod’s picture

Um, don't know what is up with that interdiff :/

catch’s picture

Status: Needs review » Reviewed & tested by the community

system.performance is a great idea for this one!

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Configuration system

The last submitted patch, 1824920-cmi_convert-theme-link-variable-7.patch, failed testing.

Cameron Tod’s picture

OK, will reroll.

Cameron Tod’s picture

Status: Needs work » Needs review
FileSize
1.58 KB

Status: Needs review » Needs work

The last submitted patch, 1824920-cmi_convert-theme-link-variable-rebase.patch, failed testing.

justafish’s picture

Status: Needs work » Needs review
FileSize
622 bytes
1.58 KB

Bumping system update number

aspilicious’s picture

Status: Needs review » Needs work

Number is way higher again! Srry!

Cameron Tod’s picture

Status: Needs work » Needs review
FileSize
1.57 KB

Rerolled against current head, bumped update number even higher.

Status: Needs review » Needs work

The last submitted patch, 1824920-cmi_convert_theme_link_variable-17.patch, failed testing.

Cameron Tod’s picture

Status: Needs work » Needs review
FileSize
1.57 KB

The same test is failing here, but I have rerolled with a new system update in the meantime. Investigating.

Status: Needs review » Needs work

The last submitted patch, 1824920-cmi_convert-theme-link-19.patch, failed testing.

Cameron Tod’s picture

Status: Needs work » Needs review
FileSize
2.17 KB

Thanks to timplunkett for the help!

Cameron Tod’s picture

FileSize
618 bytes

Here's an interdiff of the changes to the test that was failing before.

ACF’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to go, agreed it should be in performance.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

sun’s picture

Title: Convert variable 'theme_link' to config system » [HEAD BROKEN] Convert variable 'theme_link' to config system
Category: task » bug
Priority: Normal » Critical
Status: Fixed » Reviewed & tested by the community
FileSize
455 bytes
xjm’s picture

Assigned: Cameron Tod » Unassigned
Status: Reviewed & tested by the community » Needs review
FileSize
455 bytes

Missed an update hook addition.

xjm’s picture

lol. Crosspost.

catch’s picture

Category: bug » task
Priority: Critical » Normal
Status: Needs review » Fixed

Whoops. I credited both of yer.

xjm’s picture

Title: [HEAD BROKEN] Convert variable 'theme_link' to config system » Convert variable 'theme_link' to config system

Thanks!

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

Anonymous’s picture

Issue summary: View changes

was linking to wrong meta issue