Remove usages of #theme_wrappers

What is the plan? We are just removing the usages of them in core, but not removing the functionality?

So the tricky bit - implementation wise - is that all theme wrappers expect data in $elements['#children'].

However for #theme the expectation is that children have been rendered already.

My approach (still thinking) idea was to add a #pre_render function that just renders the children.

e.g.

#type => 'container'
'x' => 'foo',
'y' => 'bar',

became

#theme_wrappers => ['container'],

and will become

#theme => 'container',
#pre_render => 'drupal_pre_render_children', // Not sure that does exist.
'content' => [
  'x' => 'foo',
  'y' => 'bar',
]

That is equivalent then.

Likely we would need to extend the capabilities of Type a little for that though - for this case e.g..

Another possibility is - because theme hooks know they are containers to add auto-rendering of children - if empty.

That could probably be added via a helper function to all such preprocess functions and templates.

Or in case we already have components as a ThemeWrapperTrait.

Initial thoughts are that ...

Yes. This so needs to go. It's just another inherited artifact from the old theme system. There are so many people that simply don't understand how #theme_wrappers actually work.

One thing that should be deprecated ASAP is theme_wrappers as you can always use an upper level instead

Actually no, the way it currently works.
The "upper level" won't supply the rendered child elements to the theme hook.

In the current system even if you do use an "upper level" you still need '#theme_wrappers', or otherwise the theme hook or element type needs to render the children. (@FabianX is already saying this, so no real disagreement here)

Another problem: The 'upper level' approach falls flat if the wrapper should be added as part of the element type defaults.
E.g. '#theme_wrappers' => ['form_element'] in Textarea.php.

We would want a single-level element with '#type' => 'textarea' to be turned into something like this:

$element = [
  '#title' => $title,
  '#theme' => 'form_element'
  '#pre_render' => $render_children,
  'inner' => [
    '#theme' => 'textarrea',
    '#value' => $value,
  ],
];

Not so easy with the common "defaults merging".

----------

@markcarver (#10):

Yes. This so needs to go. It's just another inherited artifact from the old theme system. There are so many people that simply don't understand how #theme_wrappers actually work.

You may be right about people not understanding it. But removing it would leave a gap in the API.
I have used it a lot. But can see how it can go wrong.

@FabianX (#4):

So the tricky bit - implementation wise - is that all theme wrappers expect data in $elements['#children'].

Yes.

However for #theme the expectation is that children have been rendered already.

I don't think so, from looking at the code.
The expectation is that the theme hook should render the children by itself, instead of having the render system do it. Or that the theme hook is not meant to be used with children.

In Drupal 7 drupal_render() it is/was a little different: theme() is called first, and later the rendered children are appended to the output.
So in the end you would get $theme_output . $rendered_children.

In Drupal 8 / 9, Renderer::doRender() will only do one OR the other. Either the theme hook in '#theme' is executed, OR the child elements are rendered, but never both.

My approach (still thinking) idea was to add a #pre_render function that just renders the children.

This would cause inconsistency in the order of '#pre_render' callbacks and possibly other steps in the render process.

--------

Personally I have used '#theme_wrappers' successfully, but I can see how it can go wrong.

(more in next comment)

Perhaps one solution could be an option either in the theme hook definition or in the render element, that would cause the children to be rendered first, and then to be sent to '#theme'.

So, in Renderer::doRender():

Case 1: Just '#theme', no rendering of child elements.
Case 2: Just rendering of child elements, no '#theme' call.
Case 3: Render child elements, then call '#theme' with the rendered children (imploded) in '#children'.

Perhaps the distinct snippets from rendered children could even be made available in array format, e.g. in '#children_array'.

I see kinda a common thread of "prerender"/"pre-preprocess" in Don and Fabian's comments which feels right to me. An observation is this sort of feels like a shared, special and single purpose preprocess method. Form elements definitely have an inheritance vibe. When I hear inheritance I see the double edged complexity sword of code reuse but that's honestly the way things are and other than being confusing it mostly works fine.

This also definitely aligns more with what a theme_wrapper does. I commonly find myself in documentation thinking "oh great a wrapper for wrapping the output of a theme function would be really handy right now" but then have to remind myself its basically the exact opposite of that and I just have to hack with prefix and suffix and hope they work or override the template entirely or something. In that sense its more of a naming problem but this idea feels like a better solution for the problem.

Perhaps the distinct snippets from rendered children could even be made available in array format, e.g. in '#children_array'.

Neat idea. It does make for the possibility of making some already large array structures much bigger though. No facts but my gut worries about triggering array memory allocation costs if we do it wrong. On the other hand I can totally remember times the blob of children has been a frustration and I would have killed for something like that where I could have make a quick fix instead of completely refactoring some part of a theme's architecture. On the other other hand, it might open an entire can of works in complexity in what themers try to do in templates with child arrays. Oh the string munging!!!!!

There's a patch in #3178202: Render blocks later, so they can be placed individually in a region template which removes the usage of #theme_wrappers in regions. Reviews are welcome!

I see kinda a common thread of "prerender"/"pre-preprocess" in Don and Fabian's comments which feels right to me.

I don't know, is this what I said?
For me '#theme_wrappers' are something I want to do _after_ the main content of an element (via #theme or via children) is rendered, not before.
The main problem seems to me that attributes and other properties for a theme wrapper are mixed into the same array that may also contain a '#theme' setting.
We need better isolation.

So, here is another proposal.
Actually two ideas put together:

1. A setting to write rendered children into an element property before calling '#theme'.
2. A '#decorators' pipeline to isolate wrapper properties without ever more nesting levels.

Rendered children into property:

$element = [
  'link' => [
    '#type' => 'link',
    [..]
  ],
  '#render_children_into' => '#value',
  '#type' => 'html_tag',
  '#tag' => 'h2',
  '#attributes' => ['class' => ['title']],
];

The exact logic could be discussed.

Decorator pipeline:

$element = [
  '#type' => 'link',
  [..]
];
// Isolation of wrapper properties.
$element['#decorators'][] = [
  '#type' => 'html_tag',
  '#tag' => 'h2,
  '#attributes' => ['class' => ['title']],
];
// Additional wrapper layers.
$element['#decorators'][] = [
  '#type' => 'html_tag',
  '#tag' => 'div,
  '#attributes' => ['class' => ['col']],
  // '#decorators' acts as if the parent element was actually a child element.
  '#render_children_into' => '#value',
];

Example: View mode link wrapper

// Render an entity.
$element = $view_builder->view($entity, 'teaser');
// Wrap into a link.
$element['#decorators'][] = [
  '#type' => 'link',
  '#render_children_into' => '#title',
];

Nothings is moving on this? This theme_wrapper is so annoying, it make the theme rendering feel like going backward by "wrapping" stuff after childrens are rendered. This is a major problem of the current theme system which is already complicated enough.

Then we can add documentation to mark the feature deprecated.

What about marking it deprecated now, and fix it function by function as things goes?

Normally I detest deprecations but if this actually simplifies things without breaking anything (or much) then I'd be ok with it.

With that said, this change seems to be like it will be a bit on the large side.

Checked the 10.1.x branch as follows:
grep theme_wrappers * -R| wc -l
several occurences in
core/tests/Drupal/Tests/Core/Render/RendererTest.php
also occurs in:
core/lib/Drupal/Core/Render/Element/*.php
and several other places including
core/themes
core/modules
core/tests
In 10.1.x 'theme_wrappers' occurs in 138 different occurences.

I don't really understand what theme_wrappers are or what they do and what would have to be done to remove them without really digging in. Hopefully someone already familiar with this part of core could at least start a patch and allow us to finish it.

What is the proposed alternative here.

E.g. considering \Drupal\Core\Render\Element\CompositeFormElementTrait::preRenderCompositeFormElement that is used to wrap e.g. Radios elements in a fieldset. In that case we have #theme_wrappers of radios as well as #theme_wrappers fieldset

Are we proposing we'd combine both of those into a theme hook like fieldset__radios or just radios and duplicate any preprocessing and consolidate the templates too?

The decorator pattern proposal in #20 is interesting. It would provide similar functionality by using a widely used software design pattern. As explained by #20, the problem isn't the wrapping itself, but it's more that the way it's done right now is pretty confusing.