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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

Status: Active » Needs review

New 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.

damiankloip’s picture

Oh, the patches, of course :)

Status: Needs review » Needs work

The last submitted patch, 1921996-with-1913856.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
63.64 KB
1.5 KB

Sorry, both of those patches are out of date and not right. These two are.

The basic and advanced settings forms use this.

Status: Needs review » Needs work

The last submitted patch, 1921996-3-with-1913856.patch, failed testing.

damiankloip’s picture

Hmm, I'm sure those tests passed locally. I will have to check this out later on.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
5.06 KB

Let'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.

Status: Needs review » Needs work

The last submitted patch, 1921996-7.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
6.24 KB
7.74 KB

#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.

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

This looks sane to me.

Eclipse

catch’s picture

Status: Reviewed & tested by the community » Needs review

Can we get the form additions out of the base class in system_config_form() at all? Would save the code duplication.

tim.plunkett’s picture

There 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.

tim.plunkett’s picture

FileSize
613 bytes
8.34 KB

Opened #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)

damiankloip’s picture

I 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.

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +FormInterface

I will.

larowlan’s picture

#13: config-1921996-13.patch queued for re-testing.

larowlan’s picture

Sorry to re-test but this doesn't apply cleanly form me against 8.x HEAD anymore

Status: Reviewed & tested by the community » Needs work

The last submitted patch, config-1921996-13.patch, failed testing.

larowlan’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
7.74 KB

Looks 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

tim.plunkett’s picture

RTBC +1!

webchick’s picture

Title: Convert system_config_form() to implement FormInterface as a base class. » Change notice: Convert system_config_form() to implement FormInterface as a base class.
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

I 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().

tim.plunkett’s picture

Title: Change notice: Convert system_config_form() to implement FormInterface as a base class. » Convert system_config_form() to implement FormInterface as a base class.
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Needs change record

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