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 #2151089 by longwave, joelpittet, c4rl, IshaDakota, pplantinga, gnuget, jeanfei, sbudker1: Convert theme_admin_block() to Twig
Task
Convert theme_admin_block() to a Twig template.
Remaining tasks
- Patch
- Patch review
- Manual testing
- Profiling
Steps to test
Navigate to /admin/config
API changes
Removed the 'show' variable from the admin_block theme hook. The same functionality can be accomplished by using #access => FALSE in a render array.
Comments
Comment #1
joelpittetExtracted from meta, removed reference to template_preprocess_admin_block() in twig docblock because that preprocess doesn't exist.
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
mr.baileysAssigning to me for review during the Drupal Global Sprint Weekend 2014.
Comment #4
star-szrComment #5
star-szrUnassigning since it's been a while. Profiling can happen as part of #2151105-13: Convert theme_system_admin_index() to Twig.
Comment #6
joelpittetDon't use types in twig docblocks. @see https://drupal.org/node/1823416#datatypes
Should be a capital D in description.
Comment #7
longwaveFixed the points in #6, but why do we pass 'show' to the template at all? When would the template be useful if 'show' is FALSE?
Comment #8
joelpittet@longwave because 'show' was in the theme function before conversion. I see you're point though, maybe you could open up a follow-up issue to remove that? Likely they could use '#access' => FALSE or something to prevent the output better. Core doesn't use 'show' other that setting it to TRUE and maybe if you ask @webchick nicely she can grep contrib if there are any use-cases.
Comment #9
star-szrI think in this case we should just go ahead and make that slight API change. If
'show' => TRUE
is passed nothing will break, if someone is using'show' => FALSE
they can find the change record we'll write showing how to replace it with'#access' => FALSE
:)This needs a reroll first though.
Comment #10
longwaveReroll, this does not remove 'show' yet though.
Comment #11
longwaveRemoved 'show' by not trying to render the block in the first place if it ends up empty.
Comment #12
joelpittetWill this negatively effect sorting of empty content items? Is there a way we can test this?
Comment #13
longwaveThis change means empty content items don't end up in $blocks at all, and the order of the remaining blocks is the same. I don't see the point in putting a structure in $blocks that is tested for display in the theme function; it seems much simpler to exclude it as soon as we know it's empty.
Comment #14
joelpittetHere's a markup comparison. All looks good! @longwave, if that get's deamed an API change we may have to revert it... but otherwise I'm glad to see it removed. Going to do a quick profiling with those other two and RTBC this.
#2151105: Convert theme_system_admin_index() to Twig
#2151107: Convert theme_system_compact_link() to Twig
Comment #15
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 #16
joelpittetProfiling and Markup are good. If the API change isn't ok we can just RTBC #10. Just a quick fix needed for the docblock here.
Remove this docblock variable.
Comment #17
joelpittetTagging, anybody can pick this up.
Comment #18
longwaveComment #19
joelpittetThanks @longwave! Sorry for the nitpick, let's see if this flies!
Comment #20
star-szrAdded the API change to the issue summary and drafted a change record: https://drupal.org/node/2213737
I think there's some code in \Drupal\system\Controller\SystemController::overview() that should be removed because it calls admin_page and passes the block information on to admin_block and the 'show' key is no longer necessary.
core/modules/system/lib/Drupal/system/Controller/SystemController.php:
Comment #21
longwave@Cottser: I made that change already in #11? See the interdiff there for details.
Comment #22
star-szrOops, thought I had applied the patch when I grepped through but looks like that wasn't the case! Back to RTBC with my apologies.
Comment #23
webchickCommitted and pushed to 8.x. Thanks!
Comment #24
webchickIncidentally, nice catch on the bogus $show variable.
Comment #25
star-szrThanks! Just published that change record.
Comment #27
alimac CreditAttribution: alimac commentedMinor tag cleanup - please ignore.