Issue #2152203 by steveoliver, joelpittet, hussainweb, shanethehat, jenlampton, kpa, AnythonyR, EVIIILJ, kgoel, Cottser, dsdeiz, hanpersand: Convert theme_container() to Twig

Task

Convert theme_container() to a Twig template.

Remaining tasks

  • Patch
  • Patch review
  • Manual testing
  • Profiling

Steps to test

  1. Turn on twig debug in your settings.php
  2. Visit /admin/structure/views/view/content
  3. compare the HTML source before and after applying the patch.
  4. Remember to rebuild the cache before and after applying the patch.
CommentFileSizeAuthor
#28 2152203-theme_container-28.patch6.91 KBjoelpittet
#28 interdiff.txt1.67 KBjoelpittet
#21 2152203-theme_container-21.patch5.7 KBjoelpittet
#21 interdiff.txt803 bytesjoelpittet
#17 container-patched_txt___container-unpatched_txt.png322.74 KBjoelpittet
#17 container-patched.txt78.82 KBjoelpittet
#17 container-unpatched.txt73.53 KBjoelpittet
#17 2152203-theme_container-17.patch5.55 KBjoelpittet
#17 interdiff.txt441 bytesjoelpittet
#12 interdiff-2152203-8-12.txt4.94 KBinternetdevels
#12 theme-container-2152203-12.patch9.56 KBinternetdevels
#10 interdiff.txt2.47 KBjoelpittet
#9 2152203-8-theme_container.patch5.57 KBjoelpittet
#4 2152203-2-theme_container-4.patch3.24 KBrteijeiro
#2 2152203-2-theme_container.patch3.25 KBjoelpittet

Comments

star-szr’s picture

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

Adding a commit message to the issue summary so the folks who already worked on #1898480: [meta] form.inc - Convert theme_ functions to Twig and in the Twig sandbox get credit.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Active » Needs review
StatusFileSize
new 2152203-2-theme_container.patch3.25 KB

Split from form.inc twig conversion.

Status: Needs review » Needs work

The last submitted patch, 2: 2152203-2-theme_container.patch, failed testing.

rteijeiro’s picture

rteijeiro commented
StatusFileSize
new 2152203-2-theme_container-4.patch3.24 KB

Fixed following issues:

  1. +++ b/core/includes/theme.inc
@@ -1923,7 +1920,8 @@ function theme_container($variables) {
+  $variables['attributes'] = $element['#attributes']

    Missing semicolon. 

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Common/RenderElementTypesTest.php
@@ -50,7 +50,7 @@ function testContainer() {
+        'expected' => '<div>foo</div>' . "\n",,

    Removed extra comma. 

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/Common/RenderElementTypesTest.php
@@ -62,7 +62,7 @@ function testContainer() {
+        'expected' => '<div class="bar">foo</div>' . "\n",,

    Removed extra comma. 

  4. +++ b/core/modules/system/lib/Drupal/system/Tests/Common/RenderElementTypesTest.php
@@ -73,7 +73,7 @@ function testContainer() {
+        'expected' => '<div>foo</div>' . "\n",,

    Removed extra comma.

rteijeiro’s picture

rteijeiro commented
Status: Needs work » Needs review

Let see what says the testbot

Status: Needs review » Needs work

The last submitted patch, 4: 2152203-2-theme_container-4.patch, failed testing.

The last submitted patch, 4: 2152203-2-theme_container-4.patch, failed testing.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

@rteijeiro thank you for the patch, that looks much better... I must have had my fingers on the wrong keys or something.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review
StatusFileSize
new 2152203-8-theme_container.patch5.57 KB

Fixing whitespace in tests.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
StatusFileSize
new interdiff.txt2.47 KB
jlbellido’s picture

jlbellido
Primary language Spanish
commented

9: 2152203-8-theme_container.patch queued for re-testing.

internetdevels’s picture

internetdevels commented
StatusFileSize
new theme-container-2152203-12.patch9.56 KB
new interdiff-2152203-8-12.txt4.94 KB

Fixed comments standards.

star-szr’s picture

star-szr
he/him
Primary language English
commented

Those changes are really out of scope here, almost doubled the patch size. Adding unrelated changes like that makes patches harder to review and harder to reroll. I know it feels good to clean things up, but please create a separate issue (with a priority of minor) to correct coding standards issues :)

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

@InternetDevels Could you make a Document cleanup followup patch with the tags: Documentation and Quick Fix? For the interdiff changes?

star-szr’s picture

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

star-szr
he/him
Primary language English
commented

Case in point: #12 no longer applies but #9 still does. @InternetDevels it would be great if you could create the small follow-up issue if you don't mind :)

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue summary: View changes
Issue tags: -Needs manual testing
StatusFileSize
new interdiff.txt441 bytes
new 2152203-theme_container-17.patch5.55 KB
new container-unpatched.txt73.53 KB
new container-patched.txt78.82 KB
new container-patched_txt___container-unpatched_txt.png322.74 KB

Using number #9 but with that extra space removed.

Reviewed the markup in /admin/structure/views/view/content which seemed to use it a lot (25 times).

Attached is the markup from that page before and after the patch with twig debug turned on. As well as a diff screenshot.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue tags: -needs profiling

Scenario:

  • Full permissions
  • /admin/structure/views/view/content as homepage
  • Bartik
=== 8.x..8.x compared (530ad1a439784..530ad1ecad192):

ct  : 97,190|97,190|0|0.0%
wt  : 618,202|616,518|-1,684|-0.3%
cpu : 611,501|610,030|-1,471|-0.2%
mu  : 40,144,864|40,144,864|0|0.0%
pmu : 40,305,864|40,305,864|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=530ad1a439784&...

=== 8.x..2152203-twig-theme_container compared (530ad1a439784..530ad2358a7c5):

ct  : 97,190|98,268|1,078|1.1%
wt  : 618,202|617,481|-721|-0.1%
cpu : 611,501|611,259|-242|-0.0%
mu  : 40,144,864|40,176,488|31,624|0.1%
pmu : 40,305,864|40,343,032|37,168|0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=530ad1a439784&...

star-szr’s picture

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

Thanks @joelpittet for that big push forward!

+++ b/core/includes/theme.inc
@@ -1889,20 +1889,16 @@ function theme_indentation($variables) {
- * Returns HTML to wrap child elements in a container.
+ * Prepares variables for container templates.
  *
- * Used for grouped form items. Can also be used as a #theme_wrapper for any
- * renderable element, to surround it with a <div> and add attributes such as
- * classes or an HTML id.
+ * Default template: container.html.twig.

I think we should keep the "Used for grouped form items…" docs here. Tagging Novice to add those docs back and upload a new patch and interdiff.

Otherwise this is looking very good, profiling included!

star-szr’s picture

star-szr
he/him
Primary language English
commented

https://drupal.org/node/1354#themepreprocess are the documentation standards for preprocess functions.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt803 bytes
new 2152203-theme_container-21.patch5.7 KB

How about we move that to the twig template like so?

star-szr’s picture

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

Yep that sounds like a better idea to me, thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: 2152203-theme_container-21.patch, failed testing.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Reviewed & tested by the community

Whoa ghost in the machine much. Back to RTBC!

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

21: 2152203-theme_container-21.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: 2152203-theme_container-21.patch, failed testing.

The last submitted patch, 21: 2152203-theme_container-21.patch, failed testing.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new interdiff.txt1.67 KB
new 2152203-theme_container-28.patch6.91 KB

Oh some more tests needed whitespace help.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
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.