Issue #2151093 by joelpittet, InternetDevels, c4rl, mark.labrecque, IshaDakota, pplantinga, gnuget, longwave, jeanfei, sbudker1, quietone | Cottser: Convert theme_admin_block_content() to Twig

Task

Convert theme_admin_block_content() to a Twig template.

Remaining tasks

  • Patch
  • Patch review
  • Manual testing
  • Profiling

Steps to test

/admin/config is a good place to test.

It is important to ensure that if your current administration theme does not override the core theme function. The best way to ensure this is to switch you adminstration theme to a "vanilla" theme, such as Stark.

Files: 
CommentFileSizeAuthor
#23 2151093-23.patch3.73 KBquietone
PASSED: [[SimpleTest]]: [MySQL] 64,371 pass(es). View
#20 interdiff.txt1.08 KBCottser
#20 2151093-20.patch3.73 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 64,222 pass(es). View
#15 twig-theme_admin_block_content-2151093-15.patch3.78 KBInternetDevels
PASSED: [[SimpleTest]]: [MySQL] 63,330 pass(es). View
#10 before-twig-conversion-6.png18.9 KBmark.labrecque
#10 after-twig-conversion.png29.7 KBmark.labrecque

Comments

joelpittet’s picture

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

Split from the system module to twig meta.

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.

Cottser’s picture

Issue summary: View changes

Fix up issue summary.

Status: Needs review » Needs work

The last submitted patch, 1: 2151093-1-twig-theme_admin_block_content.patch, failed testing.

The last submitted patch, 1: 2151093-1-twig-theme_admin_block_content.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: 2151093-1-twig-theme_admin_block_content.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review

1: 2151093-1-twig-theme_admin_block_content.patch queued for re-testing.

Edit: I ran both tests locally through the UI and run-tests.sh and got some fails on Drupal\views\Tests\DefaultViewsTest but the same fails with or without the patch. So trying once more.

Last run's test failures:

  • Drupal\views\Tests\DefaultViewsTest
  • Drupal\comment\Tests\Views\DefaultViewRecentComments
mark.labrecque’s picture

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

The patch has been manually tested/verified. The markup provided by the template is identical pre- and post-patch. See screenshots provided for proof.

mark.labrecque’s picture

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

Issue summary: View changes
joelpittet’s picture

Issue tags: -Needs profiling

I created the patch so I can't RTBC on this one but it's +1 for me. Thanks @mark.labrecque for joining in on the sprint!

=== 8.x..8.x compared (52e49caf3c7f2..52e49d4d3a1f8):

ct  : 70,927|70,927|0|0.0%
wt  : 490,144|489,189|-955|-0.2%
cpu : 483,664|482,751|-913|-0.2%
mu  : 31,721,872|31,721,872|0|0.0%
pmu : 31,761,256|31,761,256|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52e49caf3c7f2&...

=== 8.x..2151093-twig-theme_admin_block_content compared (52e49caf3c7f2..52e49d73f02cb):

ct  : 70,927|71,645|718|1.0%
wt  : 490,144|492,664|2,520|0.5%
cpu : 483,664|485,710|2,046|0.4%
mu  : 31,721,872|31,786,488|64,616|0.2%
pmu : 31,761,256|31,826,592|65,336|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52e49caf3c7f2&...

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

InternetDevels’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3.78 KB
PASSED: [[SimpleTest]]: [MySQL] 63,330 pass(es). View

Here is the re-roll.

Cottser’s picture

Issue tags: +Twig conversion
mdrummond’s picture

Status: Needs review » Reviewed & tested by the community

Tested the reroll in #15. It shows the appropriate markup as shown in #10. Looks good to go.

Cottser’s picture

Issue summary: View changes
Cottser’s picture

Issue summary: View changes

Updating the suggested commit message.

Cottser’s picture

Issue summary: View changes
FileSize
3.73 KB
PASSED: [[SimpleTest]]: [MySQL] 64,222 pass(es). View
1.08 KB

is_compact_mode is not available in the Twig template so removing from the docblock and making another very minor doc tweak in line with https://drupal.org/node/1354#themepreprocess. Looks good to me.

Cottser’s picture

Just making a note that this may conflict with #2191845: theme_admin_block_content() documentation is out of date.

Cottser’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice
+++ b/core/modules/system/templates/admin-block-content.html.twig
@@ -0,0 +1,27 @@
+ * - attributes: HTML attributes to be aded to the element.

'aded' is a typo here, should be changed to 'added' :)

Good novice task for practice/learning, so tagging to create a new patch and interdiff with this change.

quietone’s picture

FileSize
3.73 KB
PASSED: [[SimpleTest]]: [MySQL] 64,371 pass(es). View

Here's the patch with 'added' instead of 'aded'.

quietone’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 23: 2151093-23.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review

23: 2151093-23.patch queued for re-testing.

Cottser’s picture

Status: Needs review » Reviewed & tested by the community

I manually diffed the two patches, the change looks good. Thanks @quietone!

Cottser’s picture

Issue summary: View changes

Amending suggested commit message.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed f674cd2 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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