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.

CommentFileSizeAuthor
#8 interdiff-2151107.txt1.03 KBlongwave
#8 2151107-twig-system_compact_link-8.patch2.26 KBlongwave
#6 theme_system_compact_link.png44.9 KBjoelpittet
#6 theme_system-unpatched-whitespace-normalized.txt35.19 KBjoelpittet
#6 theme_system-patched-whitespace-normalized.txt41.22 KBjoelpittet
#6 theme_system-unpatched.txt35.39 KBjoelpittet
#6 theme_system-patched.txt42.07 KBjoelpittet
#1 2151107-1-twig-theme_system_compact_link.patch2.22 KBjoelpittet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Active » Needs review
FileSize
2151107-1-twig-theme_system_compact_link.patch2.22 KB

Split from system modules twig conversions meta.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
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.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue tags: +Twig conversion
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented

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

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
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

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Issue tags: -Needs manual testing
FileSize
theme_system-patched.txt42.07 KB
theme_system-unpatched.txt35.39 KB
theme_system-patched-whitespace-normalized.txt41.22 KB
theme_system-unpatched-whitespace-normalized.txt35.19 KB
theme_system_compact_link.png44.9 KB

Here's the manual testing.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
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

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented
Status: Needs work » Needs review
Issue tags: -Novice
FileSize
2151107-twig-system_compact_link-8.patch2.26 KB
interdiff-2151107.txt1.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

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

RTBC++

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
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.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue summary: View changes
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs review

This looks like it could use theme_container + suggestions?

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

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

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented

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

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
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.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented

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