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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

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 #1898480: [meta] form.inc - Convert theme_ functions to Twig and in the Twig sandbox get credit.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Active » Needs review
FileSize
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 CreditAttribution: rteijeiro commented
FileSize
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 CreditAttribution: 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
Primary language English
Location Vancouver
CreditAttribution: joelpittet 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
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs work » Needs review
FileSize
2152203-8-theme_container.patch5.57 KB

Fixing whitespace in tests.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
FileSize
interdiff.txt2.47 KB
jlbellido’s picture

jlbellido
Primary language Spanish
CreditAttribution: jlbellido commented

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

InternetDevels’s picture

InternetDevels CreditAttribution: InternetDevels commented
FileSize
theme-container-2152203-12.patch9.56 KB
interdiff-2152203-8-12.txt4.94 KB

Fixed comments standards.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr 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
Primary language English
Location Vancouver
CreditAttribution: joelpittet 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
CreditAttribution: star-szr commented
Issue tags: +Twig conversion
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr 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
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Issue summary: View changes
Issue tags: -Needs manual testing
FileSize
interdiff.txt441 bytes
2152203-theme_container-17.patch5.55 KB
container-unpatched.txt73.53 KB
container-patched.txt78.82 KB
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
Primary language English
Location Vancouver
CreditAttribution: joelpittet 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
CreditAttribution: star-szr 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
CreditAttribution: star-szr commented

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

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs work » Needs review
FileSize
interdiff.txt803 bytes
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
CreditAttribution: star-szr 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
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs work » Reviewed & tested by the community

Whoa ghost in the machine much. Back to RTBC!

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet 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
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs work » Reviewed & tested by the community
FileSize
interdiff.txt1.67 KB
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 🇨🇦
CreditAttribution: webchick 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.