Convert theme_fieldset() to Twig

Split from form.inc twig conversion.

This needs a re-roll due to #2192419: Use a WCAG-compliant fieldset (fieldgroup) for #type radios/checkboxes

Would be great to move forward here, since this blocks a sane implementation of #1493324: Inline form errors for accessibility and UX

Re-rolling.

Re-rolled. Added the XSS admin filter that we are doing for form_element's label. Because IMO they should both have it or neither. @see [#8448461-17]

Also opted out of the translation of title+required because that should likely not be translated together in the first place and they are both variables and no string so it can't be good.

Needs some manual testing still, let's see if testbot agrees.

Yay, thanks! :-)

Some questions/remarks:

1. +++ b/core/includes/form.inc
   @@ -999,68 +1000,59 @@ function form_get_options($element, $key) {
   +  $variables['required'] = '';
   ...
   +  $variables['legend'] = array();
   ...
   +  $variables['description'] = array();
   

   There's a mismatch between the (fully-)empty variants of required vs. legend + description.

   Speaking of, neither an empty string, nor an empty array really makes sense to me...

   → Why are we not using NULL?

2. +++ b/core/includes/form.inc
   @@ -999,68 +1000,59 @@ function form_get_options($element, $key) {
   +  $variables['legend']['title'] = (isset($element['#title']) && $element['#title'] !== '') ? Xss::filterAdmin($element['#title']) : '';
   ...
   -  if ((isset($element['#title']) && $element['#title'] !== '') || !empty($element['#required'])) {
   
   +++ b/core/modules/system/templates/fieldset.html.twig
   @@ -0,0 +1,42 @@
   +  {% if legend.title is not empty or required -%}
   

   Hm.

   Do we really want to shift that essential non-empty logic into the theme template?

   To me, that looks like something many themers will likely implement/override wrongly?

Thanks for the review! The empty arrays are because of the way I implemented the if statements in twig, checking for their content/title values to group them and make them pretty, I could use NULLs just need to change what get's checked in the template for that. The reason I did this was to be declarative of what the template should expect. Let me know if you'd prefer the alternative and I can roll that in?

My aim is, if possible, to not use preprocess functions at all in core. This is obviously not possible but it's a goal. This would mean you have hook_theme as definition of the variables the template expects, the render arrayproviding the values, and preprocess is a step only needed to manipulate/alter the values and I'd prefer to leave that to contrib if they need to. Then those variables get passed to the template that expects them. The template can deal with the if/for logic, though I agree it needs to be as straight forward as possible. The "is not empty" is a bit strange but it's for 0 values.

I wouldn't worry about themers implementing them wrongly... not because it won't happen, but because it will. I know, because I broke a number of them already. I tried changing theme_button a button element in d7 and it broke panels IPE ajax;) And I'm likely to break more stuff, so not worth the effort. The theme_ function didn't deter me just left a sour taste with markup in PHP strings.

OK, while I don't think that #10 really addressed my concerns, at the same time, my concerns were very minor to begin with.

If necessary, stuff like that can still be changed in the upcoming ~12 months to release.

→ RTBC :)

This is now related and conflicting with #2152217: Remove theme_form_required_marker() from the theme system - use CSS instead

2152209-twig-theme_fieldset-8.patch no longer applies.

error: patch failed: core/includes/form.inc:999
error: core/includes/form.inc: patch does not apply

Rerolled.

2152209-twig-fieldset-14.patch no longer applies.

error: patch failed: core/includes/form.inc:8
error: core/includes/form.inc: patch does not apply

Rerolled.

Committed and pushed to 8.x. Thanks!