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.

CommentFileSizeAuthor
#23 2151093-23.patch3.73 KBquietone
#1 2151093-1-twig-theme_admin_block_content.patch3.45 KBjoelpittet
#20 interdiff.txt1.08 KBstar-szr
#20 2151093-20.patch3.73 KBstar-szr
#15 twig-theme_admin_block_content-2151093-15.patch3.78 KBInternetDevels
#10 before-twig-conversion-6.png18.9 KBmark.labrecque
#10 after-twig-conversion.png29.7 KBmark.labrecque
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Active » Needs review
FileSize
2151093-1-twig-theme_admin_block_content.patch3.45 KB

Split from the system module to twig meta.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
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.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
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

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs work » Needs review

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

Status: Needs review » Needs work

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

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
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

mark.labrecque
he/him
Primary language English
Location Vancouver
CreditAttribution: mark.labrecque commented
Assigned: Unassigned » mark.labrecque
mark.labrecque’s picture

mark.labrecque
he/him
Primary language English
Location Vancouver
CreditAttribution: mark.labrecque commented
Issue tags: -Needs manual testing
FileSize
after-twig-conversion.png29.7 KB
before-twig-conversion-6.png18.9 KB

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

mark.labrecque
he/him
Primary language English
Location Vancouver
CreditAttribution: mark.labrecque commented
Assigned: mark.labrecque » Unassigned
mark.labrecque’s picture

mark.labrecque
he/him
Primary language English
Location Vancouver
CreditAttribution: mark.labrecque commented
Issue summary: View changes
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
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&...

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

InternetDevels’s picture

InternetDevels CreditAttribution: InternetDevels commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
twig-theme_admin_block_content-2151093-15.patch3.78 KB

Here is the re-roll.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue tags: +Twig conversion
RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray commented
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.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue summary: View changes
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue summary: View changes

Updating the suggested commit message.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue summary: View changes
FileSize
2151093-20.patch3.73 KB
interdiff.txt1.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.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented

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

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
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

quietone CreditAttribution: quietone commented
FileSize
2151093-23.patch3.73 KB

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

quietone’s picture

quietone CreditAttribution: quietone commented
Status: Needs work » Needs review

Status: Needs review » Needs work

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

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs work » Needs review

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

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs review » Reviewed & tested by the community

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

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue summary: View changes

Amending suggested commit message.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
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.