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.

Files: 
CommentFileSizeAuthor
#16 2152211-twig-theme-form-6-reroll.patch2.2 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,086 pass(es). View
#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
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,068 pass(es). View
#6 interdiff.txt1.3 KBIshaDakota
#2 2152211-convert-theme-form-2.patch2.29 KBrteijeiro
PASSED: [[SimpleTest]]: [MySQL] 59,395 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.

rteijeiro’s picture

Status: Active » Needs review
FileSize
2.29 KB
PASSED: [[SimpleTest]]: [MySQL] 59,395 pass(es). View

Let's see how I did it :)

mikl’s picture

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.

Cottser’s picture

Issue tags: +Twig conversion
Cottser’s picture

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

Status: Needs work » Needs review
FileSize
1.3 KB
2.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,068 pass(es). View

Edited patch per comment #5.

Status: Needs review » Needs work

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

Cottser’s picture

Status: Needs work » Needs review
andypost’s picture

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

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.

Cottser’s picture

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

joelpittet’s picture

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

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
153.57 KB
32.39 KB
32 KB

Just for posterity here's another markup comparison.

joelpittet’s picture

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

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

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
2.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,086 pass(es). View

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

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.