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

Task

Convert theme_form() to a Twig template.

Remaining tasks

  • Patch
  • Patch review
  • Manual testing
  • Profiling

Steps to test

Look at any form.

CommentFileSizeAuthor
#16 2152211-twig-theme-form-6-reroll.patch2.2 KBjoelpittet
#13 form-without-patch.txt32 KBjoelpittet
#13 form-with-patch.txt32.39 KBjoelpittet
#13 Kaleidoscope_–_form-without-patch_txt___form-with-patch_txt.png153.57 KBjoelpittet
#6 2152211-convert-theme-form-6.patch2.2 KBIshaDakota
#6 interdiff.txt1.3 KBIshaDakota
#2 2152211-convert-theme-form-2.patch2.29 KBrteijeiro
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.

rteijeiro’s picture

rteijeiro CreditAttribution: rteijeiro commented
Status: Active » Needs review
FileSize
2152211-convert-theme-form-2.patch2.29 KB

Let's see how I did it :)

mikl’s picture

mikl CreditAttribution: mikl commented
Issue tags: -Needs manual testing

Works great.

If you want to test this patch, be aware that app forms disappear after applying it, until you can clear the cache – so if you're not logged in and have loaded the clear cache button already, you'll have to use drush 7 to clear it – a simple `drush cache-rebuild` will do.

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
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Novice

Docs need some tweaking, otherwise this is looking good so far!

  1. +++ b/core/includes/form.inc
@@ -2552,16 +2552,15 @@ function form_pre_render_color($element) {
- * Returns HTML for a form.
+ * Prepares variables for form templates.

    This should also add the 'Default template' line per https://drupal.org/node/1354#themepreprocess. 

  2. +++ b/core/includes/form.inc
@@ -2552,16 +2552,15 @@ function form_pre_render_color($element) {
  *
- * @ingroup themeable

    We need to remove the line above @ingroup themeable as well :) 

  3. +++ b/core/modules/system/templates/form.html.twig
@@ -0,0 +1,17 @@
+ * - children: The rendered checkboxes.

    This documentation seems wrong, this should probably be referencing the contents or child elements of the form. 

  4. +++ b/core/modules/system/templates/form.html.twig
@@ -0,0 +1,17 @@
+ @todo: remove this file once http://drupal.org/node/1819284 is resolved.
+ This is identical to core/modules/system/templates/container.html.twig

    These lines should be removed.

IshaDakota’s picture

IshaDakota CreditAttribution: IshaDakota commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.3 KB
2152211-convert-theme-form-6.patch2.2 KB

Edited patch per comment #5.

Status: Needs review » Needs work

The last submitted patch, 6: 2152211-convert-theme-form-6.patch, failed testing.

star-szr’s picture

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

6: 2152211-convert-theme-form-6.patch queued for re-testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue tags: -Novice

Actual render of form is happens in form builder not sure it's possible to move the whole form render to theme layer, but worst to try
This needs review of someone who knows internals of Form builder|render

+++ b/core/includes/form.inc
@@ -2571,8 +2571,8 @@ function theme_form($variables) {
+  $variables['children'] = $element['#children'];

+++ b/core/modules/system/templates/form.html.twig
@@ -0,0 +1,15 @@
+ * - children: The child elements of the form.

This always a rendered string!!!
suppose we need to delegate Form api render of children to theme layer

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Needs work
Issue tags: +Novice 
+++ b/core/includes/form.inc
@@ -2553,16 +2553,16 @@ function form_pre_render_color($element) {
  * @param $variables

Needs array type: @param array $variables.

star-szr’s picture

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

Regarding #9 while I support the idea in general I highly recommend that be a separate/follow-up issue.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Assigned: Unassigned » joelpittet

I agree with @Cottser here, the idea would be good, the change may not even be that hard but it's not the goal of this issue and should be a follow-up.

I'm putting this on my plate for profiling. We need to just get this in so we can get suggestions/consolidation/remove the div/get real children, etc.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs work » Needs review
Issue tags: -Novice
FileSize
Kaleidoscope_–_form-without-patch_txt___form-with-patch_txt.png153.57 KB
form-with-patch.txt32.39 KB
form-without-patch.txt32 KB

Just for posterity here's another markup comparison.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Assigned: joelpittet » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -needs profiling

Scenario:

3 of each User and Search form blocks added to the front page.

=== 8.x..8.x compared (530ed1c5bdc6a..530ed20e39905):

ct  : 55,542|55,542|0|0.0%
wt  : 410,750|410,175|-575|-0.1%
cpu : 407,035|406,287|-748|-0.2%
mu  : 32,986,248|32,986,248|0|0.0%
pmu : 33,024,768|33,024,768|0|0.0%

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

=== 8.x..2152211-twig-theme-form compared (530ed1c5bdc6a..530ed2569a2c7):

ct  : 55,542|55,809|267|0.5%
wt  : 410,750|409,802|-948|-0.2%
cpu : 407,035|408,237|1,202|0.3%
mu  : 32,986,248|33,028,768|42,520|0.1%
pmu : 33,024,768|33,066,432|41,664|0.1%

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

2152211-convert-theme-form-6.patch no longer applies.

error: patch failed: core/includes/form.inc:2553
error: core/includes/form.inc: patch does not apply
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
2152211-twig-theme-form-6-reroll.patch2.2 KB

Patch applies with 3-way merge, here's a re-roll. Back to RTBC, no changes.

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.