Issue #2152209 by steveoliver, joelpittet, hussainweb, shanethehat, jenlampton, kpa, AnythonyR, EVIIILJ, kgoel, Cottser, dsdeiz, hanpersand: Convert theme_fieldset() to Twig

Task

Convert theme_fieldset() to a Twig template.

Remaining tasks

  • Patch
  • Patch review
  • Manual testing
  • Profiling

Steps to test

@todo

Files: 
CommentFileSizeAuthor
#17 2152209-twig-fieldset-17.patch6.56 KBlongwave
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,094 pass(es). View

Comments

Cottser’s picture

Issue summary: View changes

Adding a commit message to the issue summary so the folks who already worked on #1898480: [meta] form.inc - Convert theme_ functions to Twig and in the Twig sandbox get credit.

joelpittet’s picture

Status: Active » Needs review
FileSize
4.58 KB
PASSED: [[SimpleTest]]: [MySQL] 63,132 pass(es). View

Split from form.inc twig conversion.

michamilz’s picture

Cottser’s picture

Issue tags: +Twig conversion
sun’s picture

Status: Needs review » Needs work

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

Cottser’s picture

Issue tags: +Needs reroll

Thanks, tagging so the reroll task can be found more easily.

joelpittet’s picture

Assigned: Unassigned » joelpittet

Re-rolling.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.5 KB
PASSED: [[SimpleTest]]: [MySQL] 64,569 pass(es). View

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.

sun’s picture

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?

joelpittet’s picture

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.

sun’s picture

Status: Needs review » Reviewed & tested by the community

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 :)

joelpittet’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

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

longwave’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,831 pass(es). View

Rerolled.

sun’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

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

longwave’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
6.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,094 pass(es). View

Rerolled.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

joelpittet’s picture

Assigned: joelpittet » Unassigned