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
- 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
Comment | File | Size | Author |
---|---|---|---|
#23 | drupal8.theme-input.23.patch | 29.02 KB | sun |
#14 | drupal8-theme-input-1812724-14.patch | 24.03 KB | steveoliver |
#10 | drupal8.theme-input.10.patch | 3.24 KB | sun |
#8 | drupal-consolidate-form-element-template-1812724-8--do-not-test.patch | 4.64 KB | steveoliver |
Comments
Comment #0.0
jenlamptonadded code
Comment #0.1
jenlamptonadded code samples
Comment #0.2
jenlamptonadded email
Comment #0.3
jenlamptonremove funcs
Comment #0.4
jenlamptoncleanup
Comment #1
jenlamptonupdate title.
Comment #2
jenlamptonupdating title, again.
Comment #2.0
jwilson3added children to example twig
Comment #2.1
jenlamptonupdate
Comment #2.2
jenlamptonupdate
Comment #3
jwilson3Would 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
Comment #4
catchThe new theme functions really annoyed me when committing all the HTML5 form elements, it'd be lovely to rip them out again.
Comment #5
steveoliver CreditAttribution: steveoliver commentedThis needs love. It's first on the list for #1757550: [Meta] Convert core theme functions to Twig templates
Comment #6
tim.plunkettThere is already another major meta with almost an identical description: #1804614: [meta] Consolidate theme functions and properly use theme suggestions in core
Comment #7
steveoliver CreditAttribution: steveoliver commentedYeah, 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.
Comment #8
steveoliver CreditAttribution: steveoliver commentedThe attached patch attempts to use
template_preprocess_input__checkbox
andinput--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, buttemplate_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?
Comment #9
steveoliver CreditAttribution: steveoliver commentedMarking needs review.
Comment #10
sunThese 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.
The suggestion is already expressed as #theme input__[type] in the element type definition, so this additional theme hook suggestion should be removed.
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:
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.
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<?
.Comment #11
steveoliver CreditAttribution: steveoliver commentedThanks, sun, for the input. This brings me back to c4rl's axioms and other thoughts on improving renderapi, specifically:
I can't follow up yet, but wanted to say thanks and I'm stewing on all this still.
Comment #12
sun1) 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: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.
Comment #13
steveoliver CreditAttribution: steveoliver commented1. 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.
Comment #14
steveoliver CreditAttribution: steveoliver commentedThis 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.
Comment #15
steveoliver CreditAttribution: steveoliver commentedOnly thing I thought I should point out:
I wonder if it'll be OK to remove theme_submit(). It seems like it should be fine.
Comment #16
sunThat 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.:
We'd then change
template_preprocess_input()
to prepare both of the variables: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.
Comment #18
steveoliver CreditAttribution: steveoliver commentedLooks like a testbot issue?
Comment #19
steveoliver CreditAttribution: steveoliver commentedre: #16:
Don't we still need access to 'element', which has all the default element properties we turn in to attributes?
Comment #20
steveoliver CreditAttribution: steveoliver commented#14: drupal8-theme-input-1812724-14.patch queued for re-testing.
Comment #22
sunThe 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 intotheme()
, even if they are not part of the 'variables' declaration of a theme hook.Comment #23
sunThe 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.
Comment #24
steveoliver CreditAttribution: steveoliver commentedYeah, these title and @return are way better.
so nice to see these go :)
Comment #25
sunSo we need someone to RTBC this :)
Comment #26
steveoliver CreditAttribution: steveoliver commentedyeah, let's do it.
Comment #27
webchickYay, 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.
Comment #28
sunI 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.
Comment #29
webchickOh, duh. :) Right, makes sense.
Comment #30
jenlampton*happydance*
Comment #31
steveoliver CreditAttribution: steveoliver commentedJust 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.
Comment #32
sunChange 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
Comment #33
webchickThis apparently broke autocomplete: #1893400: Autocomplete is busted Some confirmation on the fix there would be good.
Comment #34.0
(not verified) CreditAttribution: commentedrelated