Convert theme_form_element() to Twig

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.

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.

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

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

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.

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

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.

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

@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'?

Ok so this needs another review.

14: 2152213-twig-theme_form_element-14.patch queued for re-testing.

14: 2152213-twig-theme_form_element-14.patch queued for re-testing.

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

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.

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.

Great, thank you. :)

Committed/pushed to 8.x, thanks!