Problem/Motivation

Several Drupal theme functions result in exactly the same markup structures.

<input class="{{ attributes.class }}"{{ attributes }} />{{ children }}

As we convert to twig we should take the opportunity to consolidate what can be consolidated, and add theme_hook suggestions where particular instances of these structures may need to be overridden independently.

Proposed resolution

Implement theme('input') wherever possible, and add theme_hook_suggestions for all variations, including:

  • radio
  • checkbox
  • button
  • image_button
  • hidden
  • number
  • range
  • password
  • file
  • textarea
  • email
  • tel

Remaining tasks

- fix #939462: Specific preprocess functions for theme hook suggestions are not invoked
- fix #1751194: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter()
- consolidate and add theme hook suggestions

Related tasks

#1804614: [meta] Consolidate theme functions and properly use theme suggestions in core

User interface changes

None.

API changes

TBD

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jenlampton’s picture

Issue summary: View changes

added code

jenlampton’s picture

Issue summary: View changes

added code samples

jenlampton’s picture

Issue summary: View changes

added email

jenlampton’s picture

Issue summary: View changes

remove funcs

jenlampton’s picture

Issue summary: View changes

cleanup

jenlampton’s picture

Title: Consolidate all form element theme functions / twig templates » [meta] Consolidate all form element theme functions

update title.

jenlampton’s picture

Title: [meta] Consolidate all form element theme functions » [meta] Consolidate all form element templates and add theme_hook_suggestons
Assigned: Unassigned » jenlampton

updating title, again.

jwilson3’s picture

Issue summary: View changes

added children to example twig

jenlampton’s picture

Issue summary: View changes

update

jenlampton’s picture

Issue summary: View changes

update

jwilson3’s picture

Would it make sense to use theme subpattern suggestions here, and just leave them unimplemented in core, but allowing a themer to easily override a specific type?

eg theme_input__radio, theme_input__checkbox, etc.

Update: I'm sorry, I totally missed "and add theme_hook_suggestions for all variations" in the issue summary. #facepalm

catch’s picture

The new theme functions really annoyed me when committing all the HTML5 form elements, it'd be lovely to rip them out again.

steveoliver’s picture

Priority: Normal » Major

This needs love. It's first on the list for #1757550: [Meta] Convert core theme functions to Twig templates

tim.plunkett’s picture

Priority: Major » Normal

There is already another major meta with almost an identical description: #1804614: [meta] Consolidate theme functions and properly use theme suggestions in core

steveoliver’s picture

Title: [meta] Consolidate all form element templates and add theme_hook_suggestons » Consolidate all form element templates and add theme_hook_suggestons
Assigned: jenlampton » steveoliver

Yeah, good call. This is an actual patch issue (linked from there), so removing meta tag. Assigning to myself as I'm working on it currently.

steveoliver’s picture

The attached patch attempts to use template_preprocess_input__checkbox and input--checkbox.tpl.php by setting '#theme' => 'input__checkbox' for all checkbox form elements.

Throughout your local test site after applying this patch you should notice that input--checkbox.tpl is being used, but template_preprocess_input__checkbox is not being called.

I would expect #939462: Specific preprocess functions for theme hook suggestions are not invoked (patch in comment #157) to resolve the issue of template_preprocess_input__checkbox not running, but it doesn't.

1. Is the approach in this patch the direction we should be heading for this consolidation issue?
2. Is the patch in the other issue expected to facilitate this behavior?

steveoliver’s picture

Status: Active » Needs review

Marking needs review.

sun’s picture

+++ b/core/includes/form.inc
@@ -3190,6 +3190,63 @@ function theme_checkbox($variables) {
+function theme_input($variables) {
...
+  return '<input' . new Attribute($element['#attributes']) . ' />';
...
+function template_preprocess_input(&$variables) {
...
+  $variables['attributes'] = new Attribute($element['#attributes']);

These lines expose a problem that hasn't really been addressed by the introduction of the new Attribute object; namely the question of:

At which exact point in the rendering/theming pipeline are #attributes turned into an Attribute object?

Right now, all of core is a hodge-podge of inconsistency with regard to this question - sometimes, the conversion happens in template preprocess functions already, sometimes it happens in theme functions, and I think I've even seen instances that are completely outside of the theme process already. It's entirely not clear at which point #attributes should be converted to an Attribute object, and we badly need to standardize that.

+++ b/core/includes/form.inc
@@ -3190,6 +3190,63 @@ function theme_checkbox($variables) {
+  $variables['theme_hook_suggestions'][] = 'input__' . $element['#type'];

The suggestion is already expressed as #theme input__[type] in the element type definition, so this additional theme hook suggestion should be removed.

+++ b/core/includes/form.inc
@@ -3190,6 +3190,63 @@ function theme_checkbox($variables) {
+function template_preprocess_input__checkbox(&$variables) {

I wonder whether the majority of this preprocessing code doesn't actually belong into a #pre_render callback?

In fact, this nicely exposes an architectural problem caused by the co-existence of render arrays and theme functions in Drupal:

Technically, it is very weird that a theme preprocess function has any knowledge about render array properties in the first place. That tight binding essentially disallows anyone to use the theme function as a pure theme function, since you have to pass a fully processed render array into it instead of theme variables.

We do not have a clear stance on that separation currently. However, when taking a step back, then one could reasonably argue for this sane separation:

1. Render array.
2. #pre_render to prepare and convert the element for #theme.
3. #theme preprocessing
4. #theme

which in turn would allow everyone to skip 1. and 2. and just do:

theme('input', array(
  'attributes' => array(
    'type' => 'textfield',
    'id' => 'edit-foo',
    'name' => 'foo',
    'value' => 'Bar',
  ),
));

As a result, you get what you expect: A themed input element. However, there's no form involved, and there's also no render array involved. It's just a plain input HTML element, themed by Drupal.

It's exactly this what currently prevents you from using form elements outside of a form context - e.g., if you simply want a textfield or even details element to appear somewhere, where no form is involved (e.g., for JS-driven widgets, etc).

I'm not saying that moving the preprocessing into #pre_render is the ultimate and right answer, but I am very confident that a considerable amount of our theme system problems are caused by a missing "translation layer" from render arrays into theme functions/variables.

Attached patch demonstrates what I mean.

+++ b/core/themes/stark/templates/form.inc/input--checkbox.tpl.php
@@ -0,0 +1,21 @@
+<?
+/**
+ * @file
+ * Template for checkbox input form element.

+++ b/core/themes/stark/templates/form.inc/input.tpl.php
@@ -0,0 +1,36 @@
+<?
+/**
+ * @file
+ * Default theme implementation for an input form element.

1) For now, I'd keep the templates out of this patch. Converting every input field into a template will have a measurable performance impact, and in any case, isn't really within the scope of this issue.

2) Btw, the short opening PHP tags are not supported and should not be used. Always use <?php instead of <?.

steveoliver’s picture

Thanks, sun, for the input. This brings me back to c4rl's axioms and other thoughts on improving renderapi, specifically:

Axiom vii: For a given #type, the internal structure should be well-defined

I can't follow up yet, but wanted to say thanks and I'm stewing on all this still.

sun’s picture

1) Oy! I didn't really expect #10 to pass the entire test suite ;) Apparently, the patch actually puts the new theme functions directly into place and makes all of core use them. (i.e., to clarify, most of the code in #8 was only added, but unused.) That's nice, to say the least. :)

2) I might have to disagree with @c4rl there. Render element #types are for developers, not themers, and they represent very abstract Processable & Themeable Thingies™ to begin with. (Other) Modules can swap out #type definitions entirely with different ones, enhance them in significant ways, or even destruct them. Therefore, by design, the internal structure of a given #type is very dynamic. What requires consistency is #theme, so any kind of #theme means the exact same #theme:

However, as I already alluded to in #10, we currently have an "indefinition" for certain #theme functions, because they take a render element as base, which essentially - and wrongly - circles back into #type. It gets very apparent in the hook_theme() definitions:

function mymodule_theme() {
  $theme['foo'] = array(
    'render element' => 'element',
  );
  $theme['bar'] = array(
    'variables' => array('title' => '', 'attributes' => array(), ...),
  );
  return $theme;
}

One of both is explicitly and clearly defined, and thus reliable, and thus cleanly preprocessable, and thus cleanly alterable, and thus easy and sane to work with. Everything that needs preprocessing gets preprocessed, and the final theme function or template gets exactly what it ought to get.

The other one is "implicitly anything" and has lost its entire clear definition of what's contained and what's not. It's unclear what exact properties and variables are available at any point of time, it's unclear what needs to be preprocessed, and at which point of time, and ultimately, the raw, random data structure is passed into the theme layer in order to render something undefined.

Obviously, this is hitting a much larger problem space than the scope of this issue. However, with the patch in #10, I tried to find a reasonable separation and differentiation between the #type and the #theme, so that both work for their intended audiences and at the appropriate API layers respectively, which apparently not only solves the concrete goal and scope of this issue, but in addition, also a good chunk of some epic tasks and bug reports about certain #types not working outside of a particular [Form] API context.

That said, I'm not married to the #pre_render proposal of #10 and I've mainly put it up as a concrete proposal for discussion. In any case (to sorta answer the two questions in #8), the approach of #10 is definitely the cardinal direction to go, as it accomplishes the consolidation of INPUT element theme functions into a single one by leveraging theme hook suggestions, and still retains the form element #type-specific preparation + massaging that needs to happen prior to invoking the #theme hook.

steveoliver’s picture

Status: Needs review » Active

1. I know, nice! :)

2. Yeah, I confusingly used #type out of context of c4rl's argument for cleaning up render. In any case, I'll save that discussion for appropriate meta issue(s).

I'm almost done with a patch that follows on your work in #10 of splitting responsibility more appropriately between #theme and #pre_render.

steveoliver’s picture

Assigned: steveoliver » Unassigned
Status: Active » Needs review
FileSize
24.03 KB

This seems like it should work, but is failing locally in a few ways. Just want to get it up before I return to "work". Unassigning from myself for now. Will take it back on when I have time if no one beats me to it. This week is pretty jammed up.

steveoliver’s picture

Only thing I thought I should point out:

+++ b/core/includes/form.inc
@@ -4087,34 +4084,49 @@ function form_process_autocomplete($element, &$form_state) {
-function theme_submit($variables) {

I wonder if it'll be OK to remove theme_submit(). It seems like it should be fine.

sun’s picture

That looks very good to me already. Some comment/phpDoc hiccups, and all the registered but obsolete theme hooks aren't removed, but otherwise, I really like it.

Regarding #type submit, there are actually two different INPUT type values, type="button" and type="submit", so it would make most sense to use #theme input__button and #theme input__submit respectively. Piece of cake with this consolidation! :)

Lastly, I think we actually want to change the registration for the 'input' theme hook to use 'variables' instead of 'render element'; i.e.:

  'input' = array(
    'variables' => array('attributes' => array(), 'children' => array()),
  ),

We'd then change template_preprocess_input() to prepare both of the variables:

function template_preprocess_input(&$variables) {
  $variables['attributes'] = new Attribute($variables['attributes']);
  $variables['children'] = drupal_render_children($variables['children']);
}

function theme_input($variables) {
  return '<input' . $variables['attributes'] . ' />' . $variables['children'];
}

KISS. :)

And in the end, we might want to consider to move those two functions from form.inc into theme.inc, so as to further clarify the clean separation.

Status: Needs review » Needs work

The last submitted patch, drupal8-theme-input-1812724-14.patch, failed testing.

steveoliver’s picture

Status: Needs work » Needs review

Looks like a testbot issue?

steveoliver’s picture

Assigned: Unassigned » steveoliver
Status: Needs review » Active

re: #16:

Don't we still need access to 'element', which has all the default element properties we turn in to attributes?

steveoliver’s picture

Status: Active » Needs review
Issue tags: -Twig, -theme system cleanup

Status: Needs review » Needs work
Issue tags: +Twig, +theme system cleanup

The last submitted patch, drupal8-theme-input-1812724-14.patch, failed testing.

sun’s picture

Don't we still need access to 'element', which has all the default element properties we turn in to attributes?

The translation from render element to theme #variables happens in the #pre_render functions already.

That said, my suggestion for the 'children' variable doesn't work with HEAD, since $element is no longer available, and drupal_render() will not populate #children if a #theme function is defined. I think I bitched about that when talking to @effulgentsia already and believe that we should change drupal_render()/theme() to retain + supply _all_ original $variables that were passed into theme(), even if they are not part of the 'variables' declaration of a theme hook.

sun’s picture

Status: Needs work » Needs review
FileSize
29.02 KB

The tests failed, since we forgot to convert #type 'token' according to #type 'hidden'.

Since that relationship wasn't entirely clear, I've moved the type definition of token right below hidden, and also double-checked all other type definitions.

I also removed all obsolete theme hook registrations.

FWIW, I'd consider this patch RTBC already.

steveoliver’s picture

+++ b/core/includes/form.inc
@@ -4129,24 +4141,23 @@ function theme_button($variables) {
 /**
- * Returns HTML for an image button form element.
+ * Prepares a #type 'image_button' render element for theme_input().
  * ...
- * @ingroup themeable
+ * @return array
+ *   The $element with prepared variables ready for theme_input().

Yeah, these title and @return are way better.

+++ b/core/includes/theme.inc
@@ -3253,9 +3256,6 @@ function drupal_common_theme() {
-    'radio' => array(
-      'render element' => 'element',
-    ),
     'radios' => array(
       'render element' => 'element',
     ),
@@ -3265,57 +3265,15 @@ function drupal_common_theme() {

@@ -3265,57 +3265,15 @@ function drupal_common_theme() {
     'exposed_filters' => array(
       'render element' => 'form',
     ),
-    'checkbox' => array(
-      'render element' => 'element',
-    ),
     'checkboxes' => array(
       'render element' => 'element',
     ),
-    'button' => array(
-      'render element' => 'element',
-    ),
-    'image_button' => array(
-      'render element' => 'element',
-    ),
-    'hidden' => array(
-      'render element' => 'element',
-    ),
-    'textfield' => array(
-      'render element' => 'element',
-    ),
-    'tel' => array(
-      'render element' => 'element',
-    ),
-    'email' => array(
-      'render element' => 'element',
-    ),
-    'url' => array(
-      'render element' => 'element',
-    ),
-    'number' => array(
-      'render element' => 'element',
-    ),
-    'range' => array(
-      'render element' => 'element',
-    ),
-    'color' => array(
-      'render element' => 'element',
-    ),
     'form' => array(
       'render element' => 'element',
     ),
     'textarea' => array(
       'render element' => 'element',
     ),
-    'search' => array(
-      'render element' => 'element',
-    ),
-    'password' => array(
-      'render element' => 'element',
-    ),
-    'file' => array(
-      'render element' => 'element',
-    ),
     'tableselect' => array(
       'render element' => 'element',

so nice to see these go :)

sun’s picture

So we need someone to RTBC this :)

steveoliver’s picture

Status: Needs review » Reviewed & tested by the community

yeah, let's do it.

webchick’s picture

Title: Consolidate all form element templates and add theme_hook_suggestons » Change notice: Consolidate all form element templates and add theme_hook_suggestons
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Yay, glad to see Twig patches back on the RTBC list. :D

The patch itself looks fairly straight-forward once your eyes stop crossing from all the render arrays. :D I like that it provides some nice consistency around when things are render arrays and how they're decorated.

I asked effulgentsia to look at this just for a second set of eyes and he gave it a thumbs up. I clicked around a bit and the forms I found all look ok.

The only thing I find somewhat confusing is that looking at the drupal_common_theme() hunk in #24, I'm left wondering why more of the "native" elements (e.g. select, textarea) aren't converted, too. I didn't see discussion of it in the issue, unless I missed it. Any reason for that?

However, this seems like a good starting point and it helps speed along Twig conversions which I know has been stressing catch and I out a lot. ;)

Committed and pushed to 8.x. Thanks!

This will need a change notice.

sun’s picture

I got confused by that, too, but the reason is simple — only the converted ones are INPUT elements; all the others are different HTML elements; e.g., SELECT, TEXTAREA, etc.pp.

webchick’s picture

Oh, duh. :) Right, makes sense.

jenlampton’s picture

*happydance*

steveoliver’s picture

Just to clarify: there's no Twig implementations in this patch (we're not even using any templates), but this consolidation makes Twig conversion work much simpler for form elements.

There may be other form elements to consolidate, but I'm pretty sure there was just 'input'.

Also, as sun mentioned in #16, we may want to consider moving some of this code into theme.inc.

sun’s picture

Title: Change notice: Consolidate all form element templates and add theme_hook_suggestons » Consolidate all form element templates and add theme_hook_suggestons
Assigned: steveoliver » Unassigned
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Needs change record

Change notice: http://drupal.org/node/1886178

In writing that, I actually needed to add a last section for module developers, and concluded that we probably want to do something about that, so I've created a follow-up issue:

#1886186: Introduce a new #theme_translate callback for render element #types

webchick’s picture

This apparently broke autocomplete: #1893400: Autocomplete is busted Some confirmation on the fix there would be good.

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

Anonymous’s picture

Issue summary: View changes

related