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

joelpittet’s picture

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

Extracted from meta, removed reference to template_preprocess_admin_block() in twig docblock because that preprocess doesn't exist.

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.

mr.baileys’s picture

Assigned: Unassigned » mr.baileys
Issue tags: +SprintWeekend

Assigning to me for review during the Drupal Global Sprint Weekend 2014.

Cottser’s picture

Issue tags: +Twig conversion
Cottser’s picture

Assigned: mr.baileys » Unassigned

Unassigning since it's been a while. Profiling can happen as part of #2151105-13: Convert theme_system_admin_index() to Twig.

joelpittet’s picture

Status: Needs review » Needs work
Issue tags: +Novice
  1. +++ b/core/modules/system/templates/admin-block.html.twig
    @@ -0,0 +1,28 @@
    + * - block: An array of information about the block, including:
    + *   - show: A boolean flag indicating if the block should be displayed.
    

    Don't use types in twig docblocks. @see https://drupal.org/node/1823416#datatypes

  2. +++ b/core/modules/system/templates/admin-block.html.twig
    @@ -0,0 +1,28 @@
    + *     (description should only be output if content is not available).
    

    Should be a capital D in description.

longwave’s picture

Status: Needs work » Needs review
FileSize
3 KB
PASSED: [[SimpleTest]]: [MySQL] 64,499 pass(es). View
841 bytes

Fixed 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?

joelpittet’s picture

@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.

Cottser’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +API change, +Needs reroll

I 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.

longwave’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,722 pass(es). View

Reroll, this does not remove 'show' yet though.

longwave’s picture

FileSize
4.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,724 pass(es). View
2.48 KB

Removed 'show' by not trying to render the block in the first place if it ends up empty.

joelpittet’s picture

+++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.php
@@ -137,12 +137,10 @@ public function overview() {
 
           if (!empty($block['content']['#content'])) {
-            $block['show'] = TRUE;
+            // Prepare for sorting as in function _menu_tree_check_access().
+            // The weight is offset so it is always positive, with a uniform 5-digits.
+            $blocks[(50000 + $item['weight']) . ' ' . $item['title'] . ' ' . $item['mlid']] = $block;
           }
-
-          // Prepare for sorting as in function _menu_tree_check_access().
-          // The weight is offset so it is always positive, with a uniform 5-digits.
-          $blocks[(50000 + $item['weight']) . ' ' . $item['title'] . ' ' . $item['mlid']] = $block;
         }

Will this negatively effect sorting of empty content items? Is there a way we can test this?

longwave’s picture

This 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.

joelpittet’s picture

Here'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

joelpittet’s picture

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

Status: Needs review » Needs work

Profiling 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.

+++ b/core/modules/system/templates/admin-block.html.twig
@@ -0,0 +1,26 @@
+ *   - show: A flag indicating if the block should be displayed.

Remove this docblock variable.

joelpittet’s picture

Issue tags: +Novice

Tagging, anybody can pick this up.

longwave’s picture

Status: Needs work » Needs review
FileSize
4.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,340 pass(es). View
587 bytes
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @longwave! Sorry for the nitpick, let's see if this flies!

  1. Markup Compared in #2151089-14: Convert theme_admin_block() to Twig
  2. Profiled in #2151089-15: Convert theme_admin_block() to Twig
  3. Patch looks good to go!
Cottser’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

Added 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:

if (!empty($block['content']['#content'])) {
  $block['show'] = TRUE;
}
longwave’s picture

Status: Needs work » Needs review

@Cottser: I made that change already in #11? See the interdiff there for details.

Cottser’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice

Oops, thought I had applied the patch when I grepped through but looks like that wasn't the case! Back to RTBC with my apologies.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

webchick’s picture

Incidentally, nice catch on the bogus $show variable.

Cottser’s picture

Thanks! Just published that change record.

Status: Fixed » Closed (fixed)

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

alimac’s picture

Issue tags: -SprintWeekend +SprintWeekend2014

Minor tag cleanup - please ignore.