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.
Files: 
CommentFileSizeAuthor
#28 2152203-theme_container-28.patch6.91 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,060 pass(es). View
#28 interdiff.txt1.67 KBjoelpittet
#21 2152203-theme_container-21.patch5.7 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,056 pass(es), 6 fail(s), and 0 exception(s). View
#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
PASSED: [[SimpleTest]]: [MySQL] 64,604 pass(es). View
#17 interdiff.txt441 bytesjoelpittet
#9 2152203-8-theme_container.patch5.57 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 63,058 pass(es). View

Comments

Cottser’s picture

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

Status: Active » Needs review
FileSize
3.25 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/theme.inc. View

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

FileSize
3.24 KB
FAILED: [[SimpleTest]]: [MySQL] 59,283 pass(es), 5 fail(s), and 0 exception(s). View

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

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

@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

Status: Needs work » Needs review
FileSize
5.57 KB
PASSED: [[SimpleTest]]: [MySQL] 63,058 pass(es). View

Fixing whitespace in tests.

joelpittet’s picture

FileSize
2.47 KB
jlbellido’s picture

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

InternetDevels’s picture

FileSize
9.56 KB
PASSED: [[SimpleTest]]: [MySQL] 63,668 pass(es). View
4.94 KB

Fixed comments standards.

Cottser’s picture

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

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

Cottser’s picture

Issue tags: +Twig conversion
Cottser’s picture

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

Issue summary: View changes
Issue tags: -Needs manual testing
FileSize
441 bytes
5.55 KB
PASSED: [[SimpleTest]]: [MySQL] 64,604 pass(es). View
73.53 KB
78.82 KB
322.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

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&...

Cottser’s picture

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!

Cottser’s picture

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

joelpittet’s picture

Status: Needs work » Needs review
FileSize
803 bytes
5.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,056 pass(es), 6 fail(s), and 0 exception(s). View

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

Cottser’s picture

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

Status: Needs work » Reviewed & tested by the community

Whoa ghost in the machine much. Back to RTBC!

joelpittet’s picture

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

Status: Needs work » Reviewed & tested by the community
FileSize
1.67 KB
6.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,060 pass(es). View

Oh some more tests needed whitespace help.

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.