Issue #2151107 by joelpittet, longwave, c4rl, IshaDakota, pplantinga, gnuget, jeanfei, sbudker1: Convert theme_system_compact_link() to Twig

Task

Convert theme_system_compact_link() to a Twig template.

Remaining tasks

  • Patch
  • Patch review
  • Manual testing
  • Profiling

Steps to test

Visit /admin/index with patch applied.

Comments

joelpittet’s picture

Status: Active » Needs review
FileSize
2.22 KB
PASSED: [[SimpleTest]]: [MySQL] 59,314 pass(es). View

Split from system modules twig conversions meta.

Cottser’s picture

Issue summary: View changes

Adding a commit message to the issue summary so the folks who already worked on #1987410: [meta] system.module - Convert theme_ functions to Twig and #1898454: system.module - Convert PHPTemplate templates to Twig (comment #41 and above aka @c4rl) get credit.

Cottser’s picture

Issue tags: +Twig conversion
Cottser’s picture

Profiling for this one can happen as part of #2151105-13: Convert theme_system_admin_index() to Twig.

joelpittet’s picture

Issue summary: View changes
Issue tags: -Needs profiling

Profile of all 3 patches and confirmed they were all on the page.
#2151107-1: Convert theme_system_compact_link() to Twig
#2151105-7: Convert theme_system_admin_index() to Twig
#2151089-11: Convert theme_admin_block() to Twig

Scenario

  • /admin/index as front page.
  • Full permissions to all users.
=== 8.x..8.x compared (53195a48b6f09..53195b6957e56):

ct  : 141,571|141,571|0|0.0%
wt  : 699,947|697,654|-2,293|-0.3%
cpu : 694,237|691,839|-2,398|-0.3%
mu  : 32,653,568|32,653,568|0|0.0%
pmu : 32,751,144|32,751,144|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=53195a48b6f09&...

=== 8.x..2151089-theme_system compared (53195a48b6f09..53195bb0cc668):

ct  : 141,571|143,024|1,453|1.0%
wt  : 699,947|703,391|3,444|0.5%
cpu : 694,237|697,723|3,486|0.5%
mu  : 32,653,568|33,003,928|350,360|1.1%
pmu : 32,751,144|33,095,584|344,440|1.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=53195a48b6f09&...

joelpittet’s picture

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Novice
  1. +++ b/core/modules/system/system.module
    @@ -2749,21 +2750,25 @@ function theme_system_powered_by() {
    + * Prepare variables for system compact link templates.
    

    "Prepare" should be "Prepares" according to the Preprocess function documentation standards.

  2. +++ b/core/modules/system/templates/system-compact-link.html.twig
    @@ -0,0 +1,12 @@
    + * Default theme implementation for a link to show or hide inline help
    + *   descriptions.
    

    Let's make the summary line a summary line (80 characters or less, could use similar wording as the preprocess docblock) and explain that it's a link to show or hide inline help in a line/paragraph below. https://drupal.org/node/1354#file

longwave’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
2.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,286 pass(es). View
1.03 KB

Fixed #7. Seems like this might be better if the preprocess simply set a boolean for on/off (and perhaps the URLs) and the template contained the link text?

joelpittet’s picture

RTBC++

Cottser’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @longwave! Agreed with @joelpittet. For #8 can we please open a follow-up issue to discuss that? I'm a bit on the fence at this point, the drupal_get_destination() certainly complicates things and we'll probably want that link to be a route too at some point.

Cottser’s picture

Issue summary: View changes
catch’s picture

Status: Reviewed & tested by the community » Needs review

This looks like it could use theme_container + suggestions?

joelpittet’s picture

Well if we are to go that way, maybe we should create a new #type ala #2031301: Replace theme_more_link() and replace with #type 'more_link'?
With a #theme_wrapper container.

longwave’s picture

Didn't see #1833932: Convert theme_system_compact_link() into a #type link until now, shouldn't this just be closed in favour of that issue? However, looking at that issue, I'm not sure which is the best approach any more.

joelpittet’s picture

Status: Needs review » Closed (duplicate)

Thanks @longwave I wasn't following that one either. Let's close this as a duplicate and put the efforts over there.

Duplicate of #1833932: Convert theme_system_compact_link() into a #type link and hopefully we can do something more along the lines of #type 'system_compact_link' or something so that it's still themable.

Cottser’s picture

I was following the other issue but blissfully unaware. Thanks for digging that up @longwave!