The current helper function system_config_form() we use to add to the form structure could instead implement the FormInterface, and be used as a SystemConfigForm base class that can easily be extended to provide the same functionality.
I think there are currently about 25 calls to the function in core, and these forms will most probably be converted to implement the FormInterface. When they do, they can just extend this class.
Here is just an initial patch, just posting some quick work.
| Comment | File | Size | Author |
|---|---|---|---|
| #19 | system-config-1921996-18.patch | 7.74 KB | larowlan |
| #13 | config-1921996-13.patch | 8.34 KB | tim.plunkett |
| #13 | interdiff.txt | 613 bytes | tim.plunkett |
| #9 | confif-1921996-9.patch | 7.74 KB | tim.plunkett |
| #9 | interdiff.txt | 6.24 KB | tim.plunkett |
Comments
Comment #1
damiankloip commentedNew patch with a couple of changes so the above concept will actually work. I have also included a second patch that includes Tim's patch from #1913856: Convert Views UI forms to use FormInterface to show this being used with the views settings forms.
Comment #2
damiankloip commentedOh, the patches, of course :)
Comment #4
damiankloip commentedSorry, both of those patches are out of date and not right. These two are.
The basic and advanced settings forms use this.
Comment #6
damiankloip commentedHmm, I'm sure those tests passed locally. I will have to check this out later on.
Comment #7
damiankloip commentedLet's take a step back and add some tests first. We may want to move the location of these and possibly implement build/validate/submitForm methods on the form test class.
Comment #9
tim.plunkett#1913856: Convert Views UI forms to use FormInterface went in, and I'd like to see this used, so rerolled to switch those views classes.
Comment #10
eclipsegc commentedThis looks sane to me.
Eclipse
Comment #11
catchCan we get the form additions out of the base class in system_config_form() at all? Would save the code duplication.
Comment #12
tim.plunkettThere are 22 usages of system_config_form(). I'll be converting a straightforward one as an example, and I'll create some issues for the rest. We should just remove system_config_form() from D8 since it wasn't in D7 anyway.
Comment #13
tim.plunkettOpened #1924990: [meta] Convert all of system_config_form() to SystemConfigFormBase, and added a deprecated message to system_config_form() (replacing the obsolete link to #1324618: Implement automated config saving for simple settings forms)
Comment #14
damiankloip commentedI won't rtbc as I created and wrote alot of this :) The changes Tim has made look fine to me though.
Those conversions will make some nice novice issues too.
Comment #15
eclipsegc commentedI will.
Comment #16
larowlan#13: config-1921996-13.patch queued for re-testing.
Comment #17
larowlanSorry to re-test but this doesn't apply cleanly form me against 8.x HEAD anymore
Comment #19
larowlanLooks like the @todo in system.module was removed by #1599554: Tutorial/guidelines for how to convert variables into configuration in 05673e
Don't give me any commit credit here, just re-rolled it.
Re-roll
Comment #20
tim.plunkettRTBC +1!
Comment #21
webchickI asked Tim and Dave Reid what the rationale of this patch was since it's not clear from the issue summary. This buys us a few things, namely:
a) Better consistency with FormInterface, which we're starting to use more in core since it more clearly groups related submit/validate/etc. together rather than them being specially named functions.
b) They can now work more cleanly with the new routing system, rather than a silly function redirect that just does drupal_get_form(new FooForm())
c) Because they're classes, you don't need to track which file they live in and module_load(); just call it when you need it.
Therefore... Committed and pushed to 8.x. Thanks!
I think this needs a change notice methinks, or more probably an adjustment to whatever change notice is already out there for system_settings_form() => system_config_form().
Comment #22
tim.plunkettI've updated http://drupal.org/node/1910694