Issue #2151821 by JacobSanford, jjcarrion, rteijeiro, joelpittet, c4rl, IshaDakota, pplantinga, gnuget, longwave, jeanfei, sbudker1: Convert theme_system_config_form() to Twig

Task

Convert theme_system_config_form() to a Twig template.

Remaining tasks

  • Patch
  • Patch review
  • Manual testing

Steps to test

Visit any settings form on the site with twig debug turned on and this patch applied and you should see right after the <form> tags the inclusion of this template.

Example admin pages to check:
/admin/appearance/settings
/admin/config/development/performance

Anything that extends from ConfigFormBase but doesn't override #theme in it's buildForm method.

Files: 
CommentFileSizeAuthor
#25 2151821-convert-system-config-form-25.patch1.77 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,295 pass(es). View
#20 2151821-convert-system-config-form-20.patch1.86 KBJacobSanford
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,848 pass(es). View
#14 screenshot-2151821-after-patch_second-example.png98.68 KBjjcarrion
#14 screenshot-2151821-after-patch-using-my-template.png86.2 KBjjcarrion
#14 screenshot-2151821-after-patch.png91.44 KBjjcarrion
#14 screenshot-2151821-before-patch.png88.76 KBjjcarrion
#9 2151821-convert-system-config-form-9.patch1.86 KBjjcarrion
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2151821-convert-system-config-form-9.patch. Unable to apply patch. See the log in the details link for more information. View
#4 2151821-convert-system-config-form-2.patch1.6 KBrteijeiro
PASSED: [[SimpleTest]]: [MySQL] 59,543 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 #1987410: [meta] system.module - Convert theme_ functions to Twig and #1898454: system.module - Convert PHPTemplate templates to Twig (comment #41 and above aka @c4rl) get credit.

joelpittet’s picture

This one looks like it should be a theme suggestion on 'container' with no markup... or something else?

Cottser’s picture

The body of the function is also identical to #2151097: Convert theme_confirm_form() to Twig, maybe we should combine these two issues?

rteijeiro’s picture

Status: Active » Needs review
FileSize
1.6 KB
PASSED: [[SimpleTest]]: [MySQL] 59,543 pass(es). View

Not sure about what I'm doing :(

Cottser’s picture

Issue tags: +Twig conversion
Cottser’s picture

I'm thinking a theme suggestion on 'form', like form__system_config or something.

joelpittet’s picture

Status: Needs review » Needs work
Issue tags: +Novice

To quote from FormBuilder.php

    // If no #theme has been set, automatically apply theme suggestions.
    // theme_form() itself is in #theme_wrappers and not #theme. Therefore, the
    // #theme function only has to care for rendering the inner form elements,
    // not the form itself.
    if (!isset($form['#theme'])) {
      $form['#theme'] = array($form_id);
      if (isset($form_state['build_info']['base_form_id'])) {
        $form['#theme'][] = $form_state['build_info']['base_form_id'];
      }
    }

So yes, that should be the case that is 'form__system_config'. That would be a bigger change with it's own issue.
More like theme_wrapper suggestions for theme_form to replace this. And maybe all that needs is a test to say it works and a change record for all these theme_*_form functions?

This is the last theme function from system.module! #1987410: [meta] system.module - Convert theme_ functions to Twig

Can we just get this in RTBC and open up a follow up for that?

In the mean time, this needs the following doc changes:

  1. +++ b/core/modules/system/system.module
    @@ -2865,22 +2866,6 @@ function theme_confirm_form($variables) {
    - * Returns HTML for a system settings form.
    
    +++ b/core/modules/system/templates/system-config-form.html.twig
    @@ -0,0 +1,16 @@
    + * Template for system config form.
    

    This should Likely say "Default theme implementation for a system settings form."
    @See docblock standards https://drupal.org/node/1823416

  2. +++ b/core/modules/system/templates/system-config-form.html.twig
    @@ -0,0 +1,16 @@
    + * as its #theme callback.  Otherwise, by default, system config forms will be
    + * themed by theme_form().
    

    theme_form will still be used with this theme as a wrapper, it is always the wrapper for forms, this just deals provides a place to wrap the children of theme_form.

    Also should probably have the message from the theme function

    * By default this does not alter the appearance of a form at all,
    * but is provided as a convenience for themers."
jjcarrion’s picture

Assigned: Unassigned » jjcarrion
jjcarrion’s picture

Assigned: jjcarrion » Unassigned
Status: Needs work » Needs review
FileSize
1.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2151821-convert-system-config-form-9.patch. Unable to apply patch. See the log in the details link for more information. View

I have made a reroll of the patch #4 and I have added the changes of the comment #7.

Here is the patch

joelpittet’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs profiling

Thanks for the quick turn around on those changes @jjcarrion. I still think it needs to be clear that theme_form is the theme_wrapper and that this doesn't replace theme_form(otherwise you'd see
tags in it). This is wrapping the children of theme_form.

If my brain could work today I could give you what to write but maybe you have some ideas on how it could be better written.

Also @jjcarrion what do you think about moving the consolidation to another issue for all theme_*_form() type functions that wrap the children of theme_from() to suggestions and just get this issue in?

Removing the need for profiling as this is admin facing and super simple. Just need a manual test to confirm this template is being used (though there should be no markup changes other than whitespace). Added steps to reproduce in the Issue Summary.

rteijeiro’s picture

@joelpittet: What do you think about:

This template will be used when a system config form specifies 'config_form'
as its #theme callback. It's used to wrap theme_form() children. This does not
alter the appearance of a form at all, but is provided as a convenience for themers.

I'm mentoring @jjcarrion and he's interested in continue helping with this and the consolidation issue. So, should he create an issue for the rest theme_*_form() type functions and do the same work like in this issue? (Maybe I'm not understanding it well, sorry)

joelpittet’s picture

@rteijeiro I'm all for creating a separate issue for theme_*_form()!

Just for illustration this is the structure:

<form><-- START #theme_wrapper => theme_form

<-- START #theme => theme_system_config_form (empty space for wrappers around form children)

<label><input><-- form element children from {{ form }}
<label><textarea><-- form element children from {{ form }}

<-- ENDS theme_system_config_form

</form><-- ENDS theme_form
joelpittet’s picture

@rteijeiro and @jjcarrion here's an issue for you guys to go to town.
#2265991: Replace theme_*_form() with theme suggestion for #theme => form.

I hope I gave enough instructions for you to play with.

jjcarrion’s picture

I have been testing this patch and I think that works ok. I have attached some screenshots:

/admin/appearance/settings
- without the patch (screenshot-2151821-before-patch.png)
- patch applied (screenshot-2151821-after-patch.png)
- patch applied with a custom template (screenshot-2151821-after-patch-using-my-template.png)

/admin/config/development/performance
- patch applied (screenshot-2151821-after-patch_second-example.png)

If it's needed more testing, please, let me know.

jjcarrion’s picture

Status: Needs work » Needs review
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

I think that is all we need. My other issue can still delete this template if it can get through reviews. But in the mean time this is RTBC. Thanks for the review.

#2265991: Replace theme_*_form() with theme suggestion for #theme => form.

joelpittet’s picture

Issue tags: -Needs manual testing, -Novice

removing tags

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2151821-convert-system-config-form-9.patch, failed testing.

joelpittet’s picture

Issue tags: +Needs reroll, +Novice

Hmm needs a re-roll.

JacobSanford’s picture

Issue tags: -Needs reroll
FileSize
1.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,848 pass(es). View

Re-Rolled!

steinmb’s picture

Status: Needs work » Needs review
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @JacobSanford and @steinmb. As I mentioned in #16 this can be consolidated later in #2265991: Replace theme_*_form() with theme suggestion for #theme => form.
But for now this will do.

Cottser’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/system.module
@@ -1700,23 +1701,7 @@ function system_block_view_system_help_block_alter(array &$build, BlockPluginInt
- * Implements hook_path_update().
+ * Implements hook_admin_paths().
  */
 function system_path_update() {

This docblock would be made incorrect with this patch applied, this should still say that it implements hook_path_update().

Cottser’s picture

Issue summary: View changes

Updating suggested commit message also.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Novice
FileSize
1.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,295 pass(es). View

Fixed the accidental comment revert. Back to RTBC.

joelpittet’s picture

Issue summary: View changes

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 25: 2151821-convert-system-config-form-25.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
Cottser’s picture

Status: Needs review » Reviewed & tested by the community

Looked like a testbot client issue, missing hash_salt in settings.php.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Well that was easy. :)

Committed and pushed to 8.x. Thanks!

  • Commit b20ae79 on 8.x by webchick:
    Issue #2151821 by joelpittet, JacobSanford, jjcarrion, rteijeiro, c4rl,...

Status: Fixed » Closed (fixed)

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