Re-implement system_settings_form() for the configuration system

Ehhh. That approach sounds like it'll make authoring the form as a module developer insanely more difficult.

What about a form_values_smoosh() function or something? :P

I agree with chx that separating the field definitions from the display makes sense, but I think we can make it simpler and not require the display information to be injected in a hook. How much work would it be to just change the way forms are defined so that the two pieces are separate? Something like this

I'm not really familiar with form api but in theory couldn't something along those lines work without too much trouble?

In general, $form_state['values'] is not flat. It only is when nothing sets #tree, which is usually the case for settings forms, but not for complex forms, like ones with Field API fields, which include necessary depth within $form_state['values'] for field name, language, item number (delta), and component (column).

That $form and $form_state['values'] both have depth, and often have *differing* tree structures is one of the things that introduces complexity and confusion for a lot of developers. Form elements contain a #parents property (once form_builder() is complete) for mapping from the element to its value location in $form_state['values'], but there's currently no facility to go the other way, from $form_state['values'] to the element, which is what this issue is about.

chx and I did discuss that there would be benefits to simplifying things by requiring that $form be structured the same as $form_state['values'] and introduce other patterns for providing render structure to $form, but that's just an idea at this point; it's something that needs to be worked out, which can take a while, so we should not let it impede the CMI initiative.

My first reaction to this issue is that there's something architecturally wrong with needing to go from data in $form_state['values'] to the form element that provided the UI for that data element. I see the use-case here for system_settings_form_submit(), but I wonder if it's because our architecture for system_settings_form() is flawed. Is there any other use-case from CMI where this comes up, or is it just about handling system settings forms?

So here is system_settings_form_submit() as it stands now

So given what effulgentsia said in #6, it seems like we should be able to rewrite this to iterate the $form elements and map them back to $form_state['values'] instead of the other way around right? I'm curious what the implications of this would be, in theory it wouldn't break any existing forms right?

Actually, I'm still not clear on why we have this:

Why not:

In other words, what is 'path' actually needed for?

It makes sense to me that, in general, you want to support $form_state['values']['cache'] going into some XML file at the path /foo/bar/cache. It doesn't make sense to me that the path you want should have any relation to the structure in $form. In other words, if you have some reason for wanting it /caching/cache, then okay, but to me that's an independent decision from the UI for that data being within $form['caching']['cache']. In other words, storage needs depend on the data, and an HTML form needs to interact with that data, but there's no reason why storage location needs to be coupled to the HTML structure of the form.

So, actually, I think $form_state['config'], as you have it, is a fine approach for defining storage information for $form_state['values'], and that putting storage information directly into the $form element would actually be worse. This maintains what I think is the right decoupling: $form elements have a single #parents key that controls the name attribute for HTML, the result of which determines the structure of $form_state['values']. A separate variable, $form_state['config'] provides information on how to store $form_state['values'] that's completely independent of the structure of $form.

Hmmm, that does make sense. However it also seems like it is going to add some complexity to creating these forms. One of the advantages of system_settings_form() is its simplicity. Of course in the past we didn't have to specify where stuff got saved, we just repurposed the element name for this. I was just brainstorming this in IRC with msonnabaum and beejeebus and some things were brought up

- It's possible that system_settings_form() just doesn't make as much sense as it used to in the new storage paradigm. We could just dump it and write manual submit handlers for all these forms. However, this would increase code weight quite a bit. There are 26 instances of system_settings_form() in core alone.

- It was pointed out more than once that $form is already used for a lot of things beyond just how the elements are represented in HTML. For instance we add submit functions there. This break is not as clear as it seems at first glance.

- We tossed around varying ways of repurposing form_id and/or form element names for this purpose. For instance $form['system.performance:caching.cache'] would save to the caching/cache element in the file system.performance.xml. Or maybe the form_id becomes the name of the file that the form will save to (although forms can save to multiple files, which is another problem.)

Ultimately, I think, this is something we need to revisit in CMI - how are we going to approach forms saving to files in general, and is the current approach the best one we can take. This merits a separate discussion I think, so I'm going to open that up elsewhere and mark this postponed for the moment.

I changed my mind and decided to refocus this issue instead. I think we have two questions at hand

- Do we just want to dump system_settings_form() and write manual submit handlers for everything?
PRO: Easily implemented without any changes to form api, removes a magic 'drupalism'.
CON: Increased code weight, somewhat decreased DX and increased complexity of implementing simple forms.

- Do we figure out a way to implement system_settings_form() using CMI
PRO: Retains a somewhat simple way to implement simple forms.
CON: It will by necessity be more complex than before because we have to specify storage, not sure the best way to approach it (although it is probably by using $form_state, which seems to be the least ugly of the solutions so far.)

Personally I am leaning towards just not using system_settings_form() in the absence of a good solution but I'd like to hear other thoughts and approaches.

Also tagging cmi which I forgot to do before

Discussed this a bit with heyrocker in IRC, and wanted to submit a potential solution here.

Someone else previously mentioned passing configuration objects to forms. That's potentially a really good idea (though it may need to be an array of configuration objects depending). In this way we can pass along configuration objects to the validate and submit handlers, and this ends up being very useful because in validate we have canonical "this is the current value that you care about" data that we can compare against (when relevant) and in submit, we can simply migrate values from the form state into the appropriate configuration object. The magic part from there could just be a submit handler that iterates over all the config objects in the array and saves them for you. To put this into heyrocker's previous pro/con format:

PRO: Should actually provide a better DX experience. Won't require forms to get ugly and represent their schema or anything crazy, and will be consistent across all forms that utilize configuration (which should fundamentally be everyone).
CON: Will require submit handlers for anyone currently using system_settings_form(), but these submit handlers would be super straight forward since they're just migrating values in the form state into the configuration objects that the form is controlling.

Eclipse

yeah, i argued for something like #20 in IRC. more code, but much simpler, discoverable code, and we can create a really dumb helper(s) that this code can use.

#23: Well, my thought was that we would add form elements from the config objects. Like, a widget for each setting, kinda like Field API (except better DX, and supporting the whole config object better than Field API supports the entity). Instead of writing mapping arrays or submit handlers for every single config form, we provide a Configuration Form API that does this magically.

Edit: The interface for the CF API could be as simple as a magic naming convention. (Though, note that I am not fond of this-that:foo_bar-baz:various-delimiter as a pattern.) Or it could be some #property. Or something totally different if we want, like OO, sky's the limit. The point is that we can provide an API that automatically handles the necessary mapping/submission handling.

Edit 2: One of the down-the-road steps for the Entity API improvements is to provide $entity->form() or suchlike. You get a base form automatically, and then you alter it how you like. The burden is on the API to provide the data, not on the user. That's what I'm thinking.

Moving this to configuration system so I can track it more easily.

One wonders here -- the problem as stated start from form_values. But, we can grab the $form itself , use #value where given and just use that together with #parents . form_state['values'] is just convenience, a denormalization if you want.

I'm going to do some thinking and talking about this at the Denver code sprint and see if I can come up with a patch to argue over.

A first attempt (actually two if you take git history into account) lives in the config-settings-form-1324618-sun branch.

The problem space turned out to be much more complex than I originally imagined, so I'd love to collaborate in the sandbox and on IRC.

Slept over this. Let me introduce to you:

Configuration settings forms NG

1. Stop attempting to handle both variables and new configuration in a single API.
2. Kill the dated and ugly anti-pattern of calling system_settings_form() at the end of form constructors.
3. Leverage state of the art Form API features instead: Shared base form IDs and wrapper callbacks.

   This gives proper hook_form_alter() and theme hooks for free. And, also paves the way for the discussed vision of leveraging a generic property API (à la fields with meta info, data types, validation, and formatters for individual config settings).

4. Simpler, cleaner, better.

Since this is a total revamp, the new code lives in the config-settings-art-1324618-sun branch.

Also bumping to critical, since we should resolve this ASAP, before converting more and more module variables to the config system.

> Kill the dated and ugly anti-pattern of calling system_settings_form() at the end of form constructors.

How is that an anti-pattern? It's nice and simple.

That said, if you can replace it with even simpler, I'm all for it.

Reading the patch quickly, I am guessing that you simply do this in any form:

and the result is that you magically get:

- the config value shown as the form element's default
- the user-entered value saved to config when the form is submitted.

If that is the case, then it's new magic I am all in favour of :)

EDIT: However, there is one negative to this change I can see: harder to learn and harder to find the documentation. system_settings_form(), once you found it, was a function with its own documentation which explained how to use it. FormAPI is not a terribly well documented part of Drupal. This change would need an accompanying new section in the Big Monster FormAPI Document.

Hah I see some of this was already started in the code. Great minds etc.

And now, approach no. 3) KISS.

Based on #35, thus sharing some implementation concepts, but much simplified.

I've added extensive docs to config_get_form() this time to clarify the design goals and also simple vs. advanced usage.

Code for this approach/direction lives in the config-settings-kiss-1324618-sun branch.

One detail that was definitely wrong with #35 was the attempt to automate the assignment of #default_value element properties. While that sounds like a nice convenience behavior, it's prone to break, badly, because HTTP POST data !== Form API values !== Configuration object data values. That does not completely invalidate the other code of #35 though, so I'm going to remove that #default_value handling (and addition of Form API #preprocess) from that branch.

I've forked the Form API fix for this special usage into #1591726: Missing form_id, form_build_id, and form_token when using custom #parents and #tree = TRUE on $form itself

One potential issue with the approach in #41 is that all the config data is replaced when saved. Which means (if I'm not mistaken) that the form has to expose every config setting for the config objects you are administering. This limitation was not present in system_settings_form().

On one hand this is probably a good thing as this should mean that there are less orphaned keys. On the other, it means that the flexibility through the ability to inject config into another config's form might not be as useful as intended.

Pro:

I like the way the function wraps the standard syntax of setting things into the config. It simplifies effort for the developer as it reduces the amount of syntactical push-ups that we'll have to do in order to use the new Config API. Every module that sets settings will need to do this.

While at the same time developers using the Config API outside of the context of using a form to set and get values can still use the config api directly.

Also, let's change a real form, like system_performance_settings(), to use this approach, as part of this issue.

@effulgentsia: Note that there is a second/alternative approach in #35 - let's make sure to also review that. :)

+ $form_state['build_info']['base_form_id'] = 'config_settings_form';

This is an unfortunate consequence of drupal_retrieve_form() doing something with 'base_form_id' before 'wrapper_callback' is called. We should fix that, and move this line into config_settings_form_wrapper().

Mmm, actually, this particular code is not a work around of any unfortunate consequence, but instead, a clever (ab)use of base form ID concept we have in Form API already. However, this concept and its related functionality is normally only triggered for actual/true base form constructors; i.e., when you try to build a $form_id, for which no equivalent form constructor function name exists; in which case hook_forms() is called, which supplies the actual callback to execute. Only in that case, a base_form_id is assigned and the corresponding form alter as well as theme hooks are taken into account.

Here, the idea is to not actually use or require the usage of hook_forms() (or any certain function naming pattern), but instead, only leverage the base form functionality, which is baked into Form API already. In other words, I don't really want to change drupal_retrieve_form(), because it does exactly what I want ;)

I agree with the issue comments above that we should do something more like mergeData() rather than setData().

Diving into #1496542: Convert site information to config system made me realize that this is indeed not really possible, so I'm going to change that into individual ->set() calls in a follow-up patch.

However, we will have to find a solution for the actual problem space of retaining orphan/obsolete keys and sub-keys within a configuration object forever. That's not really limited to settings forms, so I've created #1601168: Orphan/obsolete keys and sub-keys *within* configuration objects are retained *forever*

We should decouple those and introduce a #default_value_callback

Nope, as mentioned in #41 already: The idea of automatically assigning #default_value properties is utterly bogus. While that sounds like a nice convenience behavior, it's prone to break, badly, because HTTP POST data !== Form API values !== Configuration object data values.

The last submitted patch, config.settings.52.patch, failed testing.