Issue #2152201 by steveoliver, joelpittet, burgerboydaddy, katy5289, hussainweb, shanethehat, jenlampton, kpa, AnythonyR, EVIIILJ, kgoel, Cottser, dsdeiz, hanpersand: Convert theme_checkboxes() to Twig

Task

Convert theme_checkboxes() to a Twig template.

Remaining tasks

  • Patch
  • Patch review
  • Manual testing
  • Profiling

Steps to test

  1. Copy the form_test module from /core/modules/system/tests/modules/form_test/ to /modules/form_test
  2. Comment out the last line in form_test.info.yml # hidden: true
  3. Enable the module drush en -y form_test
  4. Visit /form-test/checkboxes-zero/ and compare markup with and without the patch.
Files: 
CommentFileSizeAuthor
#23 checkboxes-before-after-23.png64.97 KBManuel Garcia
#17 interdiff.txt1.06 KBjoelpittet
#17 2152201-theme_checkboxes-17.patch2.85 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,731 pass(es). View
#8 checkboxes-code-comparison.png38.82 KBkaty5289
#5 2152201-theme_checkboxes.patch2.95 KBSalah Messaoud
PASSED: [[SimpleTest]]: [MySQL] 63,059 pass(es). View
#2 2152201-2-theme_checkboxes.patch2.95 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 59,351 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
2.95 KB
PASSED: [[SimpleTest]]: [MySQL] 59,351 pass(es). View

split from twig conversion form meta.

Status: Needs review » Needs work

The last submitted patch, 2: 2152201-2-theme_checkboxes.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
Salah Messaoud’s picture

FileSize
2.95 KB
PASSED: [[SimpleTest]]: [MySQL] 63,059 pass(es). View

This patch applies fine on HEAD as at today's date, I am just getting the testbot to rerun so people can see it is OK

Cottser’s picture

@Salah Messaoud - thank you for all the rerolls and such :) if you just want to have testbot re-test the patch, please click "Retest" on the appropriate patch. Thanks!

MartiMcFlight’s picture

5: 2152201-theme_checkboxes.patch queued for re-testing.

katy5289’s picture

Checkboxes for Custom Display Settings are displaying properly. HTML markup generated by the patch looks good. I've attached a screenshot.checkboxes-twig-OK

joelpittet’s picture

Issue tags: -Needs profiling

Scenario:
Ran the test at path /form-test/checkboxes-zero/

=== 8.x..8.x compared (52e48e674e7b6..52e48ec52134e):

ct  : 39,243|39,243|0|0.0%
wt  : 339,071|338,388|-683|-0.2%
cpu : 335,190|334,563|-627|-0.2%
mu  : 26,483,056|26,483,056|0|0.0%
pmu : 26,556,384|26,556,384|0|0.0%

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

=== 8.x..2152201-twig-theme-checkboxes compared (52e48e674e7b6..52e48ef364ce3):

ct  : 39,243|39,405|162|0.4%
wt  : 339,071|339,389|318|0.1%
cpu : 335,190|335,587|397|0.1%
mu  : 26,483,056|26,526,960|43,904|0.2%
pmu : 26,556,384|26,600,040|43,656|0.2%

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

joelpittet’s picture

This has an RTBC from me, but it's my patch so... anybody else can second it:)

Thanks for joining the sprint this weekend @katy5289 and reviewing the patch.

burgerboydaddy’s picture

Scenario for profiling
1. login as admin
2. select structure; contact form categories
3. select website feedback, click edit
4. go to manage fields
5. add new field
label: some check boxes
field type: list (integer)
6. click on save
7. enter that user must select at least 2 options
list of options:
10|check me 10
11|check me 11
12|check me 12
13|check me 13
14|check me 14
8. Save; go to manage display. select "checkboxes/radio" for display
9. now add another field
label: some radio buttons
field type: list (integer)
10. click on save
11. enter that user must select just 1 option
list of options:
1|check me 1
2|check me 2
3|check me 3
4|check me 4
5|check me 5
12. Save; go to manage display. select "checkboxes/radio" for display
13. go to system settings and enter as default page: /contact
14. now open site, contact form with checkboxes and radio buttons will be displayed.
15. now is good moment to backup database since it can be used again later for radio buttons

test results:
Run 52e49acfb0eb8 uploaded successfully for drupal-perf-drupalcon.
Run 52e49b28a90e1 uploaded successfully for drupal-perf-drupalcon.

=== 8.x..8.x compared (52e49acfb0eb8..52e49b28a90e1):

ct  : 41,877|41,877|0|0.0%
wt  : 169,090|169,376|286|0.2%
cpu : 157,381|157,806|425|0.3%
mu  : 14,621,216|14,621,216|0|0.0%
pmu : 14,735,680|14,735,680|0|0.0%

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

Run 52e49acfb0eb8 uploaded successfully for drupal-perf-drupalcon.
Run 52e49b394aea8 uploaded successfully for drupal-perf-drupalcon.

=== 8.x..2152201-7-checkboxes-twig compared (52e49acfb0eb8..52e49b394aea8):

ct  : 41,877|42,023|146|0.3%
wt  : 169,090|170,199|1,109|0.7%
cpu : 157,381|158,196|815|0.5%
mu  : 14,621,216|14,647,072|25,856|0.2%
pmu : 14,735,680|14,762,096|26,416|0.2%

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

burgerboydaddy’s picture

DAM Joel, you are so fast ;-).
ok, we have 2 sets of result.

Please note that my test was run with twig debugger turned on.

Cottser’s picture

Issue tags: +Twig conversion
mdrummond’s picture

Status: Needs review » Reviewed & tested by the community

I tested patch in #5 using the same procedure as descried in #11, and the markup for the checkboxes and radio button is the same before and after the patch.

Looks good to go.

Cottser’s picture

Issue summary: View changes

Updating suggested commit message to add profilers and manual testers :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/includes/form.inc
@@ -1225,29 +1225,32 @@ function form_pre_render_checkbox($element) {
+  // Remove the disabled attribute on the wrapper.
+  unset($variables['attributes']['disabled']);

Why is this set? And why is it necessary? Perhaps the comment could be improved?

joelpittet’s picture

Issue tags: +Needs manual testing
FileSize
2.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,731 pass(es). View
1.06 KB

@alexpott, the reason behind the unset is that we took away the attribute input clobbering that was done before... but that change probably shouldn't be in this patch and will likely be removed when consolidation with container.html.twig happens. So I added the clobbering back in, removed. Also removed the empty() check on $element['#children'] because it's always initiated in drupal_render().

Setting this back to needs manual testing.

Status: Needs review » Needs work

The last submitted patch, 17: 2152201-theme_checkboxes-17.patch, failed testing.

joelpittet’s picture

joelpittet’s picture

joelpittet’s picture

Status: Needs work » Needs review
joelpittet’s picture

Issue summary: View changes
Issue tags: +Novice

Added steps to test and tagging as novice.

Manuel Garcia’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
64.97 KB

This looks fine to me, patch applies correctly, and the markup before and after is identical @ /form-test/checkboxes-zero/

checkboxes-before-after-23

joelpittet’s picture

Thank you @Manuel Garcia! The before got cut off a bit in the screenshot for the wrapper for the checkboxes but it is identical, this is taken from 8.x HEAD to compare against your screenshot in #23
<div id="edit-checkbox-zero-default" class="form-checkboxes fieldgroup form-composite">

The rest of the screenshot shows the inside checkboxes which aren't modified with this patch.

Thank you, for the RTBC again:)

Manuel Garcia’s picture

Oops! thank you for completing it joelpittet =)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • Commit c6e469e on 8.x by webchick:
    Issue #2152201 by Salah Messaoud, Manuel Garcia, steveoliver, joelpittet...

Status: Fixed » Closed (fixed)

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