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
PatchPatch reviewManual 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.
Comments
Comment #1
star-szrAdding 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.
Comment #2
joelpittetThis one looks like it should be a theme suggestion on 'container' with no markup... or something else?
Comment #3
star-szrThe body of the function is also identical to #2151097: Convert theme_confirm_form() to Twig, maybe we should combine these two issues?
Comment #4
rteijeiro commentedNot sure about what I'm doing :(
Comment #5
star-szrComment #6
star-szrI'm thinking a theme suggestion on 'form', like form__system_config or something.
Comment #7
joelpittetTo quote from FormBuilder.php
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:
This should Likely say "Default theme implementation for a system settings form."
@See docblock standards https://drupal.org/node/1823416
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
Comment #8
jjcarrionComment #9
jjcarrionI have made a reroll of the patch #4 and I have added the changes of the comment #7.
Here is the patch
Comment #10
joelpittetThanks 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.
Comment #11
rteijeiro commented@joelpittet: What do you think about:
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)
Comment #12
joelpittet@rteijeiro I'm all for creating a separate issue for theme_*_form()!
Just for illustration this is the structure:
Comment #13
joelpittet@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.
Comment #14
jjcarrionI 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.
Comment #15
jjcarrionComment #16
joelpittetI 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.
Comment #17
joelpittetremoving tags
Comment #19
joelpittetHmm needs a re-roll.
Comment #20
jacobsanfordRe-Rolled!
Comment #21
steinmb commentedComment #22
joelpittetThanks @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.
Comment #23
star-szrThis docblock would be made incorrect with this patch applied, this should still say that it implements hook_path_update().
Comment #24
star-szrUpdating suggested commit message also.
Comment #25
joelpittetFixed the accidental comment revert. Back to RTBC.
Comment #26
joelpittetComment #28
star-szr25: 2151821-convert-system-config-form-25.patch queued for re-testing.
Comment #29
star-szrLooked like a testbot client issue, missing hash_salt in settings.php.
Comment #30
webchickWell that was easy. :)
Committed and pushed to 8.x. Thanks!