theme_container can't be used with non-form elements [#988760]

We need a generic function to wrap a renderable array in a div and slap some classes on it. I asked in IRC, and jacine pointed out that theme_container() would be just the thing, except that it's hard-coded to work with form elements. So chx suggested using the #array_parents property to determine whether the element is part of a form, and skipping the special form element handling if #array_parents isn't present. It's a backwards-compatible approach that doesn't break anything. It'll also keep us from having 8,000 different theme functions in contrib that do nothing but wrap an element in a div.

Comment	File	Size	Author
#1	988760-1-theme_container.patch	2.16 KB	ksenzee

Comments

Comment #1

ksenzee

she/her

commented 4 December 2010 at 00:58

Status:

Active

» Needs review

Status	File	Size
new	988760-1-theme_container.patch	2.16 KB

This patch moves the form-specific stuff in theme_container into an if clause, so it only applies to form elements. It's using #array_parents to determine whether the element is part of a form or just a regular renderable array.

As it happens, the top-level form element doesn't currently have an #array_parents property. In IRC chx said that's a nasty bug worth fixing on its own. This patch adds #array_parents = array() to the top-level form element, which lets us correctly identify all form elements. So we get to fix a themer WTF and a form WTF at the same time.

Comment #2

jacine

she/her

New York

commented 5 December 2010 at 19:18

I really, really would like to see this happen because I'm very worried about the render API's potential to totally wreck the way of things in the theme layer. As @ksenzee mentions, having many theme functions that simply wrap an element in a <div> would be a waste, not to mention totally confusing and aggravating to a themer trying to maintain consistency for them. It would also be a sin and a pretty big regression to see #prefix and #suffix stuffed with <div class="clearfix"> all over the place, but unless this is fixed and advertised as the standard accepted "Drupal way" of doing this, it's bound to happen.

There is so much that can be done without writing theme functions now, that I'd be willing to bet that when faced with this problem, lots of developers will just bypass implementing a hook_theme(), (a) because it's crazy to have to do one just form something this small and (b) because it's just easier to hard-code it and forget about it. I also wonder what they will call these theme functions, if they do create them? There's only so many ways to name a function that adds a wrapper <div>, so the thought of it makes me cringe.

We really need to establish some best practices here. This is probably the most common task a developer will face when using the render API. There are so any ways to accomplish this, and the Render API, while awesome, is not the easiest thing for themers to grok in the first place. It took me quite a few weeks and stupid questions to get a handle on it, and I'm not at all afraid of digging into Drupal's APIs. Having to look through a good 3-5 ways "acceptable" ways this <div> could have been added and figuring out how to get rid of or change it in each of those ways is enough to drive a themer totally insane.

I also think that theme_container() as a form-element-only wrapper was a really bad idea in the first place. There is no reason that such a generic namespace should be tied up for form-elements only, especially since we ditched theme_box(). Also, any other implementation that a developer creates, no matter how it is done is essentially adding a "container", so the fact that we don't make it easy and standardized is wrong.

Changing this before it gets out of hand in contrib just makes sense. It's also nice that this patch fixes 2 WTF's at once. :)