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

Task

Convert theme_form_element() to a Twig template.

Remaining tasks

  • Patch
  • Patch review
  • Manual testing
  • Profiling

Steps to test

@todo

Files: 
CommentFileSizeAuthor
#21 interdiff.txt1.1 KBjoelpittet
#21 2152213-twig-theme_form_element-21.patch9.13 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 63,682 pass(es). View
#14 interdiff.txt10.59 KBjoelpittet
#14 2152213-twig-theme_form_element-14.patch9.32 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 63,527 pass(es). View
#4 2152213-twig-theme_form_element-4.patch13.09 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 63,216 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.

joscartesar’s picture

Issue tags: +#D8SVQ, +#SprintWeekend2014
joscartesar’s picture

Issue tags: -#D8SVQ, -#SprintWeekend2014 +D8SVQ, +SprintWeekend2014
joelpittet’s picture

Status: Active » Needs review
FileSize
13.09 KB
PASSED: [[SimpleTest]]: [MySQL] 63,216 pass(es). View

This includes the form_element_label because a separate theme function for the label is resource wasteful and this theme function is the only one to call it with duplicate logic.

jessebeach’s picture

Status: Needs review » Reviewed & tested by the community

This conversion helps us get past sticking points in #2177653: Replace theme() with drupal_render() in form.inc, which is itself blocking #2006152: [meta] Don't call theme() directly anywhere outside drupal_render(), so I'd like to see it get in.

From a code standpoint, the changes are straightforward shifting from theme() to preprocess for a template. Tests pass and some manual clicking around on forms surfaces no display regressions.

joelpittet’s picture

Just so we don't get this bumped down to needs work as quick;)

=== 8.x..8.x compared (52e96dacba2bc..52e96ef41921e):

ct  : 70,536|70,536|0|0.0%
wt  : 667,373|704,914|37,541|5.6%
cpu : 654,309|692,703|38,394|5.9%
mu  : 38,973,352|38,973,352|0|0.0%
pmu : 39,122,912|39,122,912|0|0.0%

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

=== 8.x..2152213-twig-theme_form_element compared (52e96dacba2bc..52e96f602fe1a):

ct  : 70,536|71,337|801|1.1%
wt  : 667,373|655,305|-12,068|-1.8%
cpu : 654,309|644,002|-10,307|-1.6%
mu  : 38,973,352|39,142,128|168,776|0.4%
pmu : 39,122,912|39,291,496|168,584|0.4%

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

joelpittet’s picture

Here's a better one, form pages profiling get a bit tricky because of PDOExecute statements get larger as key_value_expire table fills up.

=== 8.x..8.x compared (52e97b490de91..52e97bda33da9):

ct  : 70,536|70,536|0|0.0%
wt  : 634,350|634,740|390|0.1%
cpu : 623,280|621,787|-1,493|-0.2%
mu  : 38,973,352|38,973,352|0|0.0%
pmu : 39,122,912|39,122,912|0|0.0%

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

=== 8.x..2152213-twig-theme_form_element compared (52e97b490de91..52e97c22a1cdf):

ct  : 70,536|71,337|801|1.1%
wt  : 634,350|633,674|-676|-0.1%
cpu : 623,280|624,346|1,066|0.2%
mu  : 38,973,352|39,142,128|168,776|0.4%
pmu : 39,122,912|39,291,496|168,584|0.4%

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

sun’s picture

Hm. Is there a way to retain theme_form_element_label() has a separate theme function/template?

E.g., as a separate label.html.twig template, which is included from the form-element.html.twig template?

I'm working on some accessibility issues elsewhere, and I need the currently existing standalone label theme function for that.

joelpittet’s picture

@sun there is, but I it seems wasteful resource wise to have tag based templates. The most common use case and the only use case in core is to have a label in a form_element. if you need a label.html.twig it would be simple enough to do in a contrib theme to make a tag based twig template. You can also do an extend easier this way with a template override, or include.

Point to the issue in question and we can help you out.

sun’s picture

To clarify, I'm working on D8 core, not on contrib.

My plan is to introduce a new #type label to resolve an accessibility problem with the tableselect facility of #type table, which was discovered in #1938926: Convert simpletest theme tables to table #type and thus currently happens in that issue (although I might be going to split that into a separate issue).

I'm able to achieve what I need to do with current HEAD, because I can use the existing theme_form_element_label() for #type label, which is just a label and does not need the wrapping form element output.

This patch here removes that theme function, which presents a problem/regression for the code I'm working on.

When I last discussed some Twig/templating improvements with @mortendk and others at DrupalCamp Vienna last November, we actively used the include, extend, and block facilities of Twig within core templates already, which worked like a charm.

IIRC, template variables are correctly forwarded already, so all you'd need to do is to add a new label.html.twig template file and include it from the form element template, like this:

{% if label... %}
  {% include label.html.twig %}
{% endif %}

Doing so would allow us to continue to use the label theme function/template as a standalone element.

joelpittet’s picture

@sun don't have to clarify, you are interCore famous:) ala interweb.

webchick found lots of contrib using that theme function too, so to keep things simple I'll split them back up. Maybe we can look at better ways to deal with those things in d9 land.

Cottser’s picture

Status: Reviewed & tested by the community » Needs review

So back to needs review at least. For the record core Twig templates don't use block, include, or extend. Not yet anyway.

joelpittet’s picture

@jessebeach you're other patch helped a lot in this next patch, thank you! So far I've not had to deal with theme functions with 'render element' and now with the help of @tim.plunkett and that remove theme() calls patch I see why this didn't work as expected.

$variables['label'] = array(
    '#theme' => 'form_element_label',
    '#element' => $element,
 );

which is what the theme function pretends to expect... and instead it had to be:

  $form_element_label = array(
    '#theme' => 'form_element_label',
    '#required' => isset($element['#required']) ? $element['#required'] : '',
    '#title' => isset($element['#title']) ? $element['#title'] : '',
    '#title_display' => isset($element['#title_display']) ? $element['#title_display'] : '',
    '#id' => isset($element['#id']) ? $element['#id'] : '',
  );

Which is kinda what I'd prefer anyways to write... but wish the function didn't have that whole '#element' key deal... anybody know if I could just change them all to 'variables' instead of 'render element'?

joelpittet’s picture

FileSize
9.32 KB
PASSED: [[SimpleTest]]: [MySQL] 63,527 pass(es). View
10.59 KB

Ok so this needs another review.

Status: Needs review » Needs work

The last submitted patch, 14: 2152213-twig-theme_form_element-14.patch, failed testing.

joelpittet’s picture

The last submitted patch, 14: 2152213-twig-theme_form_element-14.patch, failed testing.

joelpittet’s picture

joelpittet’s picture

@jessebeach would you like to re-RTBC? Or anybody for that matter?

sun’s picture

Thank you! Only one remark:

+++ b/core/includes/form.inc
@@ -2743,64 +2746,57 @@ function theme_form_element($variables) {
+    '#required' => isset($element['#required']) ? $element['#required'] : '',
+    '#title' => isset($element['#title']) ? $element['#title'] : '',
+    '#title_display' => isset($element['#title_display']) ? $element['#title_display'] : '',
+    '#id' => isset($element['#id']) ? $element['#id'] : '',

Given that the values are not altered in any way, and because I think the values should be NULL when not set, an easier + more accurate approach would be:

$variables['label'] += array_intersect_key($element, array_flip(array('#id', '#required', '#title', '#title_display')));

That takes over all of the properties exactly as-is from $element - but only if they are set.

joelpittet’s picture

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

Yeah that is nice and keeps the NULL as they were which is also good since we fixed that in twig:)

Thank you for that cleaner code snippet. Passes tests locally so here it is again.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Great, thank you. :)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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