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

Task

Convert theme_admin_page() to a Twig template.

Remaining tasks

  • Patch
  • Patch review
  • Manual testing
  • Profiling

Steps to test

Visit /admin/config

Files: 
CommentFileSizeAuthor
#12 interdiff.txt1.13 KBjoelpittet
#12 2151095-twig-theme_admin_page-23.patch3.43 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,348 pass(es). View
#9 interdiff.txt1.04 KBlongwave
#9 2151095-theme_admin_page-twig-9.patch3.59 KBlongwave
PASSED: [[SimpleTest]]: [MySQL] 64,422 pass(es). View
#6 2151095-6-after.png23.47 KBCottser
#6 2151095-6-before.png18.22 KBCottser

Comments

joelpittet’s picture

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

Split from the system module twig conversion.

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.

mark.labrecque’s picture

Assigned: Unassigned » mark.labrecque
mark.labrecque’s picture

Assigned: mark.labrecque » Unassigned
Cottser’s picture

Issue tags: +Twig conversion
Cottser’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
18.22 KB
23.47 KB

The output here is not quite right, I suspect we might need to keep the drupal_render() for the individual blocks at least until #953034: [meta] Themes improperly check renderable arrays when determining visibility is resolved.

Added testing steps to the issue summary.

Before

After

Cottser’s picture

Issue tags: +Novice

And I think fixing this and posting a new patch and interdiff would be a good task for a new contributor.

Cottser’s picture

+++ b/core/modules/system/templates/admin-page.html.twig
@@ -0,0 +1,27 @@
+ * Available variables:
+ * - system_compact_link: Themed link to toggle compact view.
+ * - containers: An list of administrative blocks keyed by position: left or
+ *   right. Contains:
+ *   - blocks: A list of blocks within a container.
+ *
+ *
+ * @see template_preprocess_admin_page()

While making the changes to get the output right here we can also get rid of an extra line containing ' *' here, there should only be one ' *' line between the variables and the @see.

longwave’s picture

Status: Needs work » Needs review
FileSize
3.59 KB
PASSED: [[SimpleTest]]: [MySQL] 64,422 pass(es). View
1.04 KB
Cottser’s picture

Assigned: Unassigned » Cottser

Going to review/test/profile this one.

Cottser’s picture

Assigned: Cottser » Unassigned
Issue tags: -Needs profiling

Profiling

Scenario: /admin/config
Enabled all permissions for anonymous user.

=== 8.x..8.x compared (531ccb2cb8e3d..531ccba053790):

ct  : 62,032|62,032|0|0.0%
wt  : 352,442|352,384|-58|-0.0%
cpu : 334,614|334,673|59|0.0%
mu  : 36,021,040|36,021,040|0|0.0%
pmu : 36,100,504|36,100,504|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=531ccb2cb8e3d&...

=== 8.x..twig-admin-page-2151095-9 compared (531ccb2cb8e3d..531ccbd4518b6):

ct  : 62,032|61,978|-54|-0.1%
wt  : 352,442|352,540|98|0.0%
cpu : 334,614|334,875|261|0.1%
mu  : 36,021,040|36,059,512|38,472|0.1%
pmu : 36,100,504|36,139,944|39,440|0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=531ccb2cb8e3d&...

Profiling looks great here and markup matches up. Some questions about the code:

  1. +++ b/core/modules/system/system.admin.inc
    @@ -169,40 +171,28 @@ function theme_admin_block_content($variables) {
    +    $block = (array) $block;
    

    Why do we have this array cast? If it's needed for some reason it should be documented. I'd hope it can be removed though.

  2. +++ b/core/modules/system/system.admin.inc
    @@ -169,40 +171,28 @@ function theme_admin_block_content($variables) {
    +    if ($block['show'] = !empty($block['content']['#content'])) {
    

    Can we maybe remove the $block['show'] and postpone on #2151089: Convert theme_admin_block() to Twig?

joelpittet’s picture

FileSize
3.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,348 pass(es). View
1.13 KB
  1. Not needed at all, the MenuLink object implements ArrayAccess so the cast is not even needed for any convenience array lovers(like me, because it's a cheap hash) may want.
  2. This fix is doable without the postpone because MenuLink conveniently defaults show to true:)

Also I removed the unnecessary if statement for the array building. I tested that it no undefined errors were thrown with 0, 1, 2 blocks created and indeed there were none.

Cottser’s picture

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

Thanks @joelpittet, I like that much better! Manually tested and profiled again, removing that array cast the performance looks even better. It looks like the performance boost here comes from removing the drupal_render() call.

=== 8.x..8.x compared (531ee5f3237b0..531ee626b79e4):

ct  : 62,901|62,901|0|0.0%
wt  : 361,628|361,486|-142|-0.0%
cpu : 342,931|342,575|-356|-0.1%
mu  : 36,012,800|36,012,800|0|0.0%
pmu : 36,096,608|36,096,608|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=531ee5f3237b0&...

=== 8.x..twig-admin-page-2151095-12 compared (531ee5f3237b0..531ee657ba49e):

ct  : 62,901|62,878|-23|-0.0%
wt  : 361,628|359,109|-2,519|-0.7%
cpu : 342,931|339,911|-3,020|-0.9%
mu  : 36,012,800|36,089,160|76,360|0.2%
pmu : 36,096,608|36,174,872|78,264|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=531ee5f3237b0&...

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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