In renderable arrays, #type should provide a #theme suggestion, similar to how drupal_prepare_form() works

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

#1 - Ah yeah, Fabianx and I were having some chats in IRC about possible futures that may or may not happen. I didn't want to preclude them here.

http://drupal.org/node/169815 says:

(note: in Drupal 7, you do need to add #theme to hook_element_info())

I thought it important to mention that '#theme' is always assumed to have some value but if it is set to FALSE you can explicitly bypass theme(). This is not how drupal_render() currently works as it only checks isset() for '#theme'.

For example, currently theme() has preprocess and process phases, for which there's a proposal to refactor a bit here #2004872: [meta] Theme system architecture changes. Fabianx explained to me though that *anything* that can be teased out into discrete "input -> processed variables -> altered + processed variables -> string" can be "drillable" by Twig if that workflow is structured so that Twig can invoke the processing without the string concatenation at the end. I was suggesting that drupal_render() could be responsible for making rendered elements "drillable", with the process phases and then theme() is only responsible for invoking the right theme/template function (which couldn't happen until #2006152: [meta] Don't call theme() directly anywhere outside drupal_render()) and so we could do the things like what '#type' => 'link' does and inline rendering inside drupal_alter() for speed.

Even if that doesn't happen though, if you had '#type' => 'link' which renders a string into '#markup' in the '#pre_process' you don't want drupal_render() to call theme_link() so you would set '#theme' => FALSE in hook_element_info() as otherwise '#theme' => 'link' would be assumed.

What would be examples where #type and #theme do not have a 1-to-1 association?

Well, probably nothing at the moment. Potentially though, if #type determined the preprocessing and not #theme, in contrib/custom you might want to do something like '#type' => 'link', '#theme' => 'my_crazy_link' to send the "base" processed/sanitized variables off to your own crazy link theme function/template.

A question I have is if we envision a world where #theme goes away entirely? (I think it might). Do we feel that hook_element_info() and hook_theme() are substantively different in purpose to remain separate callbacks?

Possibly not but I feel like that line of enquiry should be a followup issue as trying to do that here could derail this first step.

#4 - if they're legitimately two different things, why don't we split them into two types rather than trying to force them to be merged?

#6 - sure... I think I'll update the issue summary because that's just another example of why we *should* make the change.

I wonder if it would be possible/easier to give '#type' => 'table' a pre_render callback that "magically" transforms the current type syntax to theme syntax and always passes it off to theme - at least as a stopgap measure. We could file a followup task to actually refactor everywhere tables are called and remove the magic later.

Updated issue summary.

Wow, I just realised that for '#theme' => 'item_list', '#type' means "type: The type of list to return (e.g. "ul", "ol")."

Updated issue summary.

Updated issue summary.

Based on c4rl's git repo I'm going to add in the proposal that if #type is not set in our array we simply concatenate the values and return. This allows partial compatibility with existing array('#markup' => 'foo') style arrays and gives us some kind of simple default fallback behaviour.

Currently if theme($elements['#theme'], $elements) returns an empty string then drupal_render() tries to render $elements['#children']. I don't understand the reasoning behind that, it looks like a bug to me so I'm going to add to the issue summary:

If theme() is called then theme() is 100% responsible for rendering both the element and it's children, drupal_render() will not interfere with or "fallback" to attempting to render #children once theme() has taken this responsibility.

Updated issue summary.

And currently if '#theme' is set, the behaviour of drupal_render() interpreting '#markup' changes as well! That's something that goes against this issue too. Updating summary with:

'#markup' is a reserved key for drupal_render() and its value will *always* be included in the output regardless of whether '#theme' is set, theme functions are therefore advised to not define a 'markup' parameter. The final output is always $prefix . $markup . $rendered . $suffix.

Updated issue summary.

Updated issue summary.

Here's a patch that just puts the current proposal into drupal_render() without attempting to address any inevitable fallout or update much documentation. Setting to needs review just so we can see the damage ;)

Followup/related - #2009152: The default behaviour for drupal_render() when theme() is not called could be better at representing HTML as structured data

Updated issue summary.

Updated issue summary.

Updated issue summary.

#16 - Fair point.

@Fabianx was the one telling me he was looking for a simple and fast way to check for "is renderable array" (ie. without calling an drupal_is_renderable_array() function) so I'd like to hear if he's happy with if (isset($foo['#type']) || isset($foo['#markup']) outside of drupal_render().

What about this?

If #type is not set then markup is assumed and #theme is set to FALSE. This allows the existing simple array('#markup' => 'foo') style renderable arrays but enforces that any renderable array wanting to use theme() has to set a base #type and provide #theme only as an override for the default callback.

Updated issue summary.

also, if anyone is wondering about the new elseif logic, #render_children is set by theme()

I added:

  // #theme with no #type is deprecated but lets assume #type = #theme for now
  // when we see an array like this.
  if (!isset($elements['#type']) && !empty($elements['#theme'])) {
    $elements['#type'] = $elements['#theme'];
  }

in an effort to reduce the immediate impact on the API of this issue and reduce the amount of work required here.

I also started cleaning up hook_element_info() implementations that I could find.

Updated issue summary.

Updated issue summary.

I opened #2010212: convert all #theme => table with #type => table and #2010210: Convert all item_list #theme to #type

Updated issue summary.

fix for a fatal in #23

mark does the same thing as item_list by using #type for input. Updated summary.

Updated issue summary.

Updated issue summary.

I split a small part of this patch out into its own issue #2012800: \Drupal\Core\Render\Renderer::doRender() could be lazier about calling Element::children()

another small part of the patch I'm splitting out #2012812: drupal_render() can't distinguish between empty strings from theme() and no hook being matched

splitting out a third bit #2012818: Remove #type 'markup'. Whittling the scope of this patch down :)

#32 - Yes, this issue is looking to change how things work. To make #type required in renderable arrays and make it determine how drupal_render() behaves (ie. stop doing special casing for renderable arrays based on #markup and #theme beyond picking a theme function) - and therefore make it part of its API.

Lets disregard Doxygen bugs and various implementations that do things wrong.

I'm not sure I totally understand which things in core you'd consider are "doing things wrong" and which things you think are working as intended.

When #item_list uses #type, it does so correctly because #type is not a reserved word.

What about when #mark does it? I believe this throws errors - #2009578: Replace theme() with drupal_render() in history module. Part of this issue is a proposal to make #type into a reserved word so that the API doesn't allow us to make theme functions that don't have robust renderable array representations.

For example, I could define a valid theme function that expects to be called like this currently:

theme('foo', array('type' => 'table'));

type here expects 'table' or 'item_list' in this theme function. How would you represent this with a renderable array with the current system without the incorrect defaults being loaded by drupal_render()?

#theme is responsible for turning the array into HTML

That's not 100% true. #prefix, #suffix, #markup are all handled by drupal_render(), not the theme() function called. You can have a #type that doesn't require #markup to be set when drupal_render() is called, and then it is built during the pre_render "phase" without using theme functions -> see #type => 'link'. You can also set #children in the array that you pass in to drupal_render() and theme() won't be touched unless you have #theme_wrappers set.

#type is used to merge in defaults from hook_element_info()

That would still be the case, but I'm also proposing that if #theme isn't set then it is assumed to be the same as #type which would mean that for the majority of existing array('#theme' => 'foo') type renderable arrays the conversion would be as simple as changing it to array('#type' => 'foo') and leaving the rest as-is.

I'd be happy to do a phone call on drupal_render() topics if thats helpful.

Sure. I'm not sure if timezones would be an issue, I'm in Melbourne. An IRC chat is also an option, I'm usually in #drupal-twig when I'm on :)

Updated issue summary.

Updated issue summary.

I added #2015177: Update documentation around hook_element_info() to not say/imply element types are limited to the Form API and removed things from the issue summary that have not been split into their own issues. The patch in #26 now needs a re-roll with reduced scope.

Updating summary to more accurately reflect what is being proposed here.

updating patch to not cover the issues opened elsewhere.

Updated issue summary.

I've been thinking about this a bit more, really trying to reduce it down to something both more achievable for a single patch and with a more palatable scope of API changes.

This issue is still trying to fix two or three different things (down from like 7 but still more than one) which is bad.

1. Wanting to enforce #type for all renderable arrays to make other ideas possible (like a #type based drupal_alter(), for example).
2. Trying to make an explicit workaround around for a bug in drupal_render() where it's relatively easy to call theme() and also the not-theme() fallback, when the intended behaviour is to only ever choose one or the other.
3. Trying to make it both easier to do and easier to learn for developers using the render API to have rich, robust and consistent behaviour across *all* their renderable arrays.

1:

I just don't think we're ready for this kind of change yet and won't be for months, maybe years. #theme isn't even being used everywhere instead of theme() yet, there'd be at least one more complete sweep of core to get everything using #type over #theme, looking at 'table' as an example of this we can see that this can be a "non trivial effort" as @tstoeckler put it.

Additionally, I'm not aware of any of the ideas that require #type being set even having a proof of concept patch available, let alone something ready for review that's complaining about being blocked on this API change.

When/if the time comes to enforce #type, we can just open a new issue for that one thing.

2:

Let's fix that bug elsewhere, there's already tickets open for it #2012812: drupal_render() can't distinguish between empty strings from theme() and no hook being matched, #2015311: If #theme suggestions are set as an array for theme() but none are implemented #markup is treated incorrectly by drupal_render(). It's a little gnarly as it will probably require at least a minor API change to theme() to fix, but the solution will hopefully be better than having to explicitly set #theme = FALSE for over half the #types currently in core.

3:

This is something that could be done here. What about if we just aim to add #type as a theme suggestion for all renderable arrays if #theme is not set? If the theme suggestion isn't available then theme() will just return early and if the bug in 2 is fixed, drupal_render() should know at this point that we're supposed to fallback to the non-theme way of processing this array without needing to be told with #theme = FALSE style declarations.

Anyway, just wanted to write down what I was thinking about somewhere..

updating issue title to reflect #38

just whittling away...

I uploaded the wrong patch >.< it's missing a few modifications to existing hook_element_info() implementations.

This can't be done until #2012812: drupal_render() can't distinguish between empty strings from theme() and no hook being matched is resolved, postponing.

#40: 2005970-40.patch queued for re-testing.

#40: 2005970-40.patch queued for re-testing.

hopefully #1985470: Remove theme_link() being closed means less than 147 fails.

this just incorporates the latest from #2012812: drupal_render() can't distinguish between empty strings from theme() and no hook being matched.

This is the only part of #49 that is still relevant.

hurrrrrrr.

This avoids setting #theme from #type if #theme_wrappers is set - so we don't break #type elements that only set #theme_wrappers, like the radio elements in the installer. I don't know why this wasn't already breaking in previous patches.

The exceptions are from drupal_get_css() causing the theme to initialise on ajax requests when it calls drupal_render(), when it can't because $theme is empty.

This patch avoids that by adding a check for empty $css arrays in drupal_get_css() and hopefully makes what is happening to #type a little more legible.

This could add a lot of calls to theme(), which almost immediately return when the suggestion doesn't match, but I think it should be profiled. We could statically cache hooks that don't match or something if it's bad.

i just opened #2032967: AssetResolver::getCssAssets() should not try to sort and optimise if $css is empty. so the patches here can stay focussed.

I had a bit more of a think about this, and I wanted to try changing $theme_is_implemented to start with #theme being !empty() rather than isset(). This way, if profiling showed that a particular #type was generating too much of an overhead by calling theme() a lot of times you could simply set #theme FALSE in hook_element_info() and that would be that.

Updated issue summary.

Ok, cool. If you want to do profiling, can you let us know what hooks are the slowest? I'm happy to drop a #theme => FALSE in hook_element_info() for anything that's noticeably slow.

So, in preparation for some profiling, I put the following code inside the new block of logic that adds #theme:

      if (!isset($counts[$elements['#type']])) {
        $counts[$elements['#type']] = 1;
      }
      else {
        $counts[$elements['#type']]++;
      }

I used devel to create 50 nodes with up to 4 comments each and displayed them all in the "full" view mode with comments displayed in the front page view.

The result was this:

contextual_links_placeholder (Integer) 34
value (Integer) 203
link (Integer) 7
html_tag (Integer) 67
styles (Integer) 1
scripts (Integer) 2

So, I'm going to re-roll the patch with #theme => FALSE in the element info for "contextual_links_placeholder", "value" "link" and "html_tag" before going through the process of profiling this as I think it's obvious enough that we don't need to be spamming theme() for elements with those #types.

After clicking around the site a bit with the counts on, here's the patch that I think should be profiled.

needs review just for the testbots, still needs profiling.

Here's my profiling results for the patch in #69, 50 nodes displayed with up to 4 comments each on the front page:

tdm:d8html thedavidmeister$ ./xhprof-kit/benchmark-branch.sh 8.x && say 'done'
Password:
loop time: |22.060801s|51f3746558d24|drupal-perf|8.x|<a href="http://127.0.0.1:8888/xhprof-kit/xhprof/xhprof_html/index.php?source=drupal-perf&url=%2F&run=51f3746558d24&extra=8.x" target="_blank">Profiler output</a>
tdm:d8html thedavidmeister$ ./xhprof-kit/benchmark-branches.sh 51f3746558d24 baseline-8.x 8.x 2005970-69 && say 'done'
Password:
=== baseline-8.x..8.x compared (51f3746558d24..51f3790f24ac9):

ct  : 4,158,319|4,158,319|0|0.0%
wt  : 22,061,397|22,032,353|-29,044|-0.1%
cpu : 21,952,414|21,917,056|-35,358|-0.2%
mu  : 28,152,760|28,152,760|0|0.0%
pmu : 29,516,360|29,516,360|0|0.0%

<a href="http://127.0.0.1:8888/xhprof-kit/xhprof/xhprof_html/index.php?source=drupal-perf&url=%2F&run1=51f3746558d24&run2=51f3790f24ac9&extra=baseline-8.x..8.x">Profiler output</a>

Password:
=== baseline-8.x..2005970-69 compared (51f3746558d24..51f3835a9b3e5):

ct  : 4,158,319|4,158,319|0|0.0%
wt  : 22,061,397|22,058,663|-2,734|-0.0%
cpu : 21,952,414|21,960,305|7,891|0.0%
mu  : 28,152,760|28,159,232|6,472|0.0%
pmu : 29,516,360|29,522,672|6,312|0.0%

<a href="http://127.0.0.1:8888/xhprof-kit/xhprof/xhprof_html/index.php?source=drupal-perf&url=%2F&run1=51f3746558d24&run2=51f3835a9b3e5&extra=baseline-8.x..2005970-69">Profiler output</a>

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

Seems fine to me...

This might be more useful, I got an API key from Fabianx to get the runs uploaded to lionsad.de:

=== baseline-8.x..8.x compared (51f3746558d24..51f3790f24ac9):

ct  : 4,158,319|4,158,319|0|0.0%
wt  : 22,061,397|22,032,353|-29,044|-0.1%
cpu : 21,952,414|21,917,056|-35,358|-0.2%
mu  : 28,152,760|28,152,760|0|0.0%
pmu : 29,516,360|29,516,360|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51f3746558d24&run2=51f3790f24ac9&source=drupal-perf-thedavidmeister&extra=baseline-8.x..8.x

Run 51f3746558d24 uploaded successfully for drupal-perf-thedavidmeister.
Run 51f3835a9b3e5 uploaded successfully for drupal-perf-thedavidmeister.
=== baseline-8.x..2005970-69 compared (51f3746558d24..51f3835a9b3e5):

ct  : 4,158,319|4,158,319|0|0.0%
wt  : 22,061,397|22,058,663|-2,734|-0.0%
cpu : 21,952,414|21,960,305|7,891|0.0%
mu  : 28,152,760|28,159,232|6,472|0.0%
pmu : 29,516,360|29,522,672|6,312|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51f3746558d24&run2=51f3835a9b3e5&source=drupal-perf-thedavidmeister&extra=baseline-8.x..2005970-69

Needs tests though.

yeaaaaah, I waited over an hour for those results. I'll re-profile, but I'm looking at tests now.

Added some tests.

#78 - In the current codebase it does because there are types in core that have the name of theme hooks intended as #theme_wrappers rather than #theme.

See comment #56 where I discovered that #type 'radios' sets #theme_wrappers array('radios').

The intention of #2025629: [meta] Ensure that all #theme/#theme_wrappers hooks provided by core are associated with a sensible, re-usable element #type is to clean up anything confusing about the existing #type/#theme/#theme_wrappers namespaces and is well beyond the scope of this issue, so for now we can't support this "auto theme" behaviour on elements that define #theme_wrappers.

As I said on that issue, this is a bit "chicken or the egg" as getting this patch in makes it easier/neater to clean up the existing element/theme definitions while cleaning up the existing element/theme definitions may allow us to support #theme_wrappers here.

There's also this outstanding issue #2042285: #theme_wrappers should be able to have a unique set of variables per wrapper hook that could easily lead to unexpected/undesirable behaviour when adding #theme into an element that already has #theme_wrappers set.

In the end I didn't see the #theme_wrappers support as critical to what this patch was trying to achieve and right now it causes more problems than it solves with various conflicts. I'm sure this is something we could revisit in D9 if/when we get our theme system structured a bit better overall but for now I think we can play by the 80/20 rule :)

Are html_tag and value really commonly called, as the comment suggests. html_tag happens once per page. That quite infrequent, given how many theme() calls happen on a page. value is only used for forms? I'm not sure what value means in a drupal_render context, to be honest.

See comment 68.

html_tag is called more than once per page. Why do you think it happens only once per page?

You can do a very simple experiment yourself. Apply the patch in #62 so you don't have any of the modifications I made to element_info() definitions and then change this:

+    if (!isset($elements['#theme']) && !isset($elements['#theme_wrappers'])) {
+      $elements['#theme'] = array($elements['#type']);
+    }

so that it contains print $elements['#type'];

You should see 'html_tag' appear quite a few times, as well as a few other types.

From memory, I believe that I was seeing 'value' most on pages with forms, but that includes comment forms (wywiwygs, etc..)

The changes I made to the hook_element_info() were made based on measuring which #types would hit theme() the most, using the method outlined in #68 - this experiment should be entirely repeatable :)

Updated issue summary.

Updated issue summary.

I have updated the issue summary to use the standard template as per #82.

Setting back to RTBC as per #82 and #81.

Updated issue summary.

#76: 2005970-76.patch queued for re-testing.

I'm not sure how to actually test that from outside drupal_render(), I'll have to think about that more :/

I don't know how to test exactly what @alexpott is asking for tests on. I started with this:

      // #theme is FALSE, #markup should be rendered.
      array(
        'name' => '#theme is FALSE, #markup is set',
        'value' => array(
          '#theme' => FALSE,
          '#markup' => 'foo',
        ),
        'expected' => 'foo',
      ),

But then, FALSE is not an implemented theme hook, so it's an identical test to:

      // Theme suggestion is not implemented, #markup should be rendered.
      array(
        'name' => '#markup fallback for #theme suggestion not implemented',
        'value' => array(
          '#theme' => array('suggestionnotimplemented'),
          '#markup' => 'foo',
        ),
        'expected' => 'foo',
      ),

Which already exists.

The only external difference between theme(FALSE, ... ) and theme(array('suggestionnotimplemented'), ... ) is that the former results in a watchdog log:

watchdog('theme', 'Theme hook %hook not found.', array('%hook' => $hook), WATCHDOG_WARNING);

I could try doing something with watchdog, but then if this log call is ever removed we will silently lose test coverage.

I'd love some advice on how to move forward here if anyone could lend a hand.

#94 - ooooh, is *that* what I'm testing. Yeah, I can do that. I think I misinterpreted the requirements.

I added tests for:

- #type is set and is an implemented theme hook, #theme is FALSE -> should render #markup
- #type is set and is an implemented theme hook, #theme is a theme hook -> should use #theme
- #type is set and is an implemented theme hook, #theme_wrappers is set -> should render #markup wrapped in #theme_wrappers

Hope that helps :)

soz, interdiff attached.

slight updated on a comment.

Note to self:

thedavidmeister
4:40 webchick: so, I will:
4:40 add an explanation of #theme_wrappers not being touched to the issue summary
4:40 remove existing #theme => #type lines from element_info() hooks
4:40 samhasse1l left the room (quit: Ping timeout: 245 seconds).
4:41 samhassell left the room (quit: Ping timeout: 246 seconds).
thedavidmeister
4:41 workaround the AJAX thing rather than try to fix it and put a comment linking to the proposed fix issue
4:41 typhoniu_ left the room (quit: Remote host closed the connection).
webchick
4:42 Yeah, something like // @todo This needs more investigation as to why it causes exceptions. http://...?

ok, back to "needs profiling" for the patch without the #theme => FALSE to ballpark gauge "worst case" regression here.

Following on from #103, anything defining #theme, #theme_wrappers or #markup will still not be affected by any performance regression.

It concerns me how often this hunk is in the patch, and how random the elements it's placed on are (contextual_links_placeholder? Really? That would *never* in a million years enter my mind as one of the top 5 theme functions called on a page.).

It isn't, it's one of the top 5 renderable types that requires #pre_render to be rendered. I don't think it's that "random" if you consider that most renderable element types do not require #pre_render to create #markup, so most element types are not candidates for #theme => FALSE here. The performance of all theme functions that actually exist (#theme or #theme_wrappers) is unaffected by this patch.

Only module developers implementing the #pre_render -> #markup pattern (that is a bit of a hack anyway, see #2052253: [META] Add #render property to drupal_render() and convert #type "#pre_render -> #markup" calls to use it) would be affected and only module developers that are already concerned about the performance of calling theme() should even be considering #pre_render -> #markup as a way to implement rendering some markup - you would hope that someone this "deep" already can read the docs and see that setting #theme => FALSE is explicitly recommended for the exact outcome they're trying to achieve.

I will still get some profiling happening, but I want to ensure that the scope of who/what the performance overhead could possibly effect is not misunderstood or exaggerated - I suppose that's exactly what profiling is though ;)

I have opened #2067759: theme() could statically cache some of it's internals to avoid making function calls in an attempt to get the overhead of calling theme() with an unimplemented theme hook down to a bare minimum.

I'm glad that @webchick asked for this patch to actually remove existing hook_element_info() implementations that are made redundant by the API addition in this patch.

What this uncovered was that there's a bunch of element types that are defined like this:

$types['foo'] => array(
  '#theme' => 'foo',
  '#theme_wrappers' => array('not_foo'),
);

So I would like to have a look at a patch where drupal_render() only avoids setting #type in #theme if #type is actually not found in #theme_wrappers, rather than backing out the moment #theme_wrappers is set - which is likely a harmless 'container' or 'form_element'.

What this means is that you would have problems if:

- #type is 'foo'
- #theme is not set
- #theme_wrappers is not set or does not include 'foo'
- theme_foo is actually implemented, but is not intended to be used with #type 'foo'

It is possible, but downright confusing to define a #type 'foo' and #theme 'foo' that are by design incompatible. IMO, if we find an instance of this we should actually clean that up.

It's not necessarily that we can't do the API change, but we need to understand the impact it has and make sure the DX win here is enough to tip the scales.

Just as a yardstick on where the DX sits against HEAD, I turned every core module on and there are 55 items returned by Drupal::moduleHandler()->invokeAll('element_info')

There are 8 current clear duplicates, #2025707: Remove unused #theme "file_widget" would make that 9 so, ~ 15% of all types.

A lot of the #types that aren't affected are either FAPI elements using theme suggestions (19 types), like input__textfield for example, or those using just #theme_wrappers and not #theme at all.

This patch:

- Removes #theme => FALSE from element_info() definitions, we can profile the overhead of this change
- Will still use #type as a theme suggestion if #theme_wrappers is set, provided #type is not already included in #theme_wrappers
- Improved the documentation by incorporating @webchick's suggestions in #101, with minor modifications to ensure accuracy
- Removes the now-redundant #theme declarations in hook_element_info
- Adds tests for the case that #theme_wrappers contains #type already as either a key or value
- Replaces the usage of #2032967: AssetResolver::getCssAssets() should not try to sort and optimise if $css is empty. with a #theme => FALSE + comment for the type definition of #type 'styles'

Setting to "needs review" for testbot review only.

function form_process_vertical_tabs($element, &$form_state) {
  // Inject a new details as child, so that form_process_details() processes
  // this details element like any other details.
  $element['group'] = array(
    '#type' => 'details',
    '#theme_wrappers' => array(),
    '#parents' => $element['#parents'],
  );

I have no idea why this is setting #theme_wrappers to an empty array instead of letting #type => 'details' provide '#theme_wrappers' => array('details')

Looking at the commit history, the empty array used to be for #type 'fieldset', nothing to do with #type 'details', so it looks like it could be a mistake?

Anyway, I'll see what happens if I use empty() instead of isset().

ignore, #110, I did it wrong.

this patch should be a little faster, not using array_key_exists(), using isset() instead.

Improved docs, added a test.

Updated issue summary.

Updated issue summary.

Added the following to the issue summary:

There is a slight issue of how to handle #theme_wrappers. For example, consider #type 'radios' that sets #theme_wrappers => array('radios') already as its default - in this case we don't want to add #theme 'radios' as this would lead to theme('radios') being called twice, inappropriately.

We also don't want to touch a render element that has an empty array set for #theme_wrappers, this is because currently core sets an empty array for #theme_wrappers when it wants to use the defaults provided by a #type sans #theme and #theme_wrappers. See form_process_vertical_tabs() for an example of this where vertical tabs use #type 'details' but forcing the call to theme() to be bypassed while still inheriting the #pre_render calls for 'details'.

This means that as well as avoiding adding #type as a default suggestion for #theme when #theme is already set, we also want to avoid using #type as a default #theme if #theme_wrappers is set and empty OR #theme_wrappers is set and already contains #type as a key/value (we need to check the key and value due to the two different syntaxes for #theme_wrappers).

Updated issue summary.

I'm declaring #type miotypo and because it's complicated, I have to set #theme to FALSE, to prevent theme_miotypo() from being used.

Well, part of the proposal is to discourage having #type and #theme with the same name if they're not compatible with each other.

I'm declaring #type miotypo and because I need a special #theme_wrapper, I have to manually set #theme to miotypo, since the automatic default #theme suggestion will not be applied because #theme_wrappers is set.

I'd prefer for this not to be the case, I don't *want* this behaviour and early versions of the patch did not include it. The reason it works this way is because of the blurred, poorly documented line between what's appropriate as a "theme wrapper" and what should be a "theme hook". Things in core like #type 'radios', that adds a 'radios' theme wrapper while setting no #theme make the situation complicated.

I did open #2053671: [meta] Name all theme functions that are only compatible with drupal_render() when nested within #theme_wrappers "foo_wrapper" but I don't even believe that's a good solution either because the naming becomes so awkward :/

Suggestions are welcome >.<