Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#8 | interdiff-2151107.txt | 1.03 KB | longwave |
#8 | 2151107-twig-system_compact_link-8.patch | 2.26 KB | longwave |
#1 | 2151107-1-twig-theme_system_compact_link.patch | 2.22 KB | joelpittet |
Comments
Comment #1
joelpittetSplit from system modules twig conversions meta.
Comment #2
star-szrAdding 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.
Comment #3
star-szrComment #4
star-szrProfiling for this one can happen as part of #2151105-13: Convert theme_system_admin_index() to Twig.
Comment #5
joelpittetProfile 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.http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=53195a48b6f09&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=53195a48b6f09&...
Comment #6
joelpittetHere's the manual testing.
Comment #7
star-szr"Prepare" should be "Prepares" according to the Preprocess function documentation standards.
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
Comment #8
longwaveFixed #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?
Comment #9
joelpittetRTBC++
Comment #10
star-szrThanks @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.Comment #11
star-szrComment #12
catchThis looks like it could use theme_container + suggestions?
Comment #13
joelpittetWell 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.
Comment #14
longwaveDidn'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.
Comment #15
joelpittetThanks @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.
Comment #16
star-szrI was following the other issue but blissfully unaware. Thanks for digging that up @longwave!