Problem/Motivation

Replace theme_*_form() with theme suggestion for #theme => form. This eliminates the registration of templates that do nothing except render {{ form }} children. @see #2151821: Convert theme_system_config_form() to Twig

Proposed resolution

#theme_wrapper => 'form' is used as a theme_wrapper which doesn't have suggestions so that will need to move to a #theme property for this to work.

Delete

core/modules/system/templates/confirm-form.html.twig

Remaining tasks

  1. Change '#theme_wrapper' => 'form' to '#theme' => 'form'.
  2. Then all the theme_*_form() calls will be suggestions for form--system-config.html.twig for example and don't need to be implemented.
  3. For extra brownie points use a twig block like we have for block.html.twig to avoid the suggestions from providing the form tags as well when implemented.

Theme functions to convert:

  • theme_system_config_form()
  • theme_confirm_form() already in core as config-form.html.twig
  • theme_forum_form()
  • Maybe views_exposed_form and a few others but implemented suggestions.

User interface changes

n/a

API changes

n/a

CommentFileSizeAuthor
#7 replace_theme_form-2265991-7.patch20.55 KBjoelpittet
#3 2265991-theme-form--sugestions-3.patch21.83 KBjoelpittet
#3 interdiff.txt3.43 KBjoelpittet
#1 2265991-theme-form--sugestions-1.patch18.94 KBjoelpittet

Comments

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 2265991-theme-form--sugestions-1.patch18.94 KB

Ok this will likely fail, but I thought I'd give a direction to which I'm thinking this issue should go.

Locally the forms are loading, so at least I can run the tests in the UI:)

There may be a better way to deal with the form theme suggestion defaults because I know there was an array('form', 'form') in #theme, which isn't the end of the world but that would be nice to get as clean as possible.

I left the word _form() at the end of the forms even though I prefixed them with theme_form__, which is a bit redundant but that may be for another issue.

This isn't necessary the way I wrote the FormBuilder changes but it's explicit, opinions?

-      '#theme' => 'language_negotiation_configure_form',
+      '#theme' => 'form__language_negotiation_configure_form',

This was added but if they are still theme functions to wrap the results in a form template. It isn't needed for a twig template, just done to get the patch rolling. Twig template just extends @system/

  // Temporary pass through to form.html.twig until template conversion.
  $output = _theme('form', $variables = array('form' => $output));
  return $output;

Did a rough conversion of theme_views_ui_expose_filter_form to twig template to show it's use.

The big win is no more theme functions or templates for empty theme_*_form(), form content wrappers. And you can totally override the form template if you want to or just extend it's contents with a block(the way we would be doing @see theme_views_ui_expose_filter_form() conversion in the patch())

Status: Needs review » Needs work

The last submitted patch, 1: 2265991-theme-form--sugestions-1.patch, failed testing.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review
Issue tags: +Template consolidation
StatusFileSize
new interdiff.txt3.43 KB
new 2265991-theme-form--sugestions-3.patch21.83 KB

I don't think the template suggestions are picking up, see the form__block_list.

Status: Needs review » Needs work

The last submitted patch, 3: 2265991-theme-form--sugestions-3.patch, failed testing.

Status: Needs work » Needs review

almul0 queued 3: 2265991-theme-form--sugestions-3.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 3: 2265991-theme-form--sugestions-3.patch, failed testing.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review
StatusFileSize
new replace_theme_form-2265991-7.patch20.55 KB

Just a re-roll.

Status: Needs review » Needs work

The last submitted patch, 7: replace_theme_form-2265991-7.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

The basic proposal makes sense to me, but I don't understand why this issue attempts to change #theme_wrappers into #theme?

The #theme of a form renders the inner HTML of the form only, because there's not really a use-case for customizing/overriding the wrapping <form> markup itself. Therefore, the wrapping form markup is output through a separate #theme_wrapper.

I don't see a value in embedding the wrapping form markup into form templates - doing so will only make templates more complex and cause bugs IMO.

m1r1k’s picture

m1r1k commented

Should we convert theme_menu_overview_form() as well?

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

@sun I think it was because theme_wrappers don't have suggestions, I could be wrong on this but that was the impression I was under. (also wrote that in the Issue Summary but likely in the wrong place)

@m1r1k Yes that should be on this list as well.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Title: Replace theme_*_form() with theme suggstion for #theme => form. » Replace theme_*_form() with theme suggestion for #theme => form.
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Closed (won't fix)

@andypost seemed to fix this in a much more direct way here #1987400: forum.module - Convert theme_ functions to Twig

Let's do that, closing this experiment down for now.