Add smart defaults for system_settings_form

So if (array_key_exists('#default_value', $item)), then you would replace the #default_value? You would still need to add a #default_value to the form item, so it doesn't save any lines of code, just removes some variable_get() calls. Doesn't seem worth it.

Seems like a great idea to me, but why do we need to check for $sensible_defaults if each form element was supposed to load it's own #default_value from the variable_get at form creation time?

Hmm... seems to me this would break on nested form arrays. Am I misreading something here? You'd probably have to do an array_walk_recursive() or something more complicated, right? For example... what about using system_settings_form() on a form with settings in fieldsets? We do this in several Ubercart modules to try and help categorize settings on some highly configurable modules.

If my assumptions are correct, I think it would negate the pay off of the proposed change.

Also, it's helpful to me to be able to copy/paste the variable_get() in my settings forms so I make sure the string literal and default setting are the same when I use that variable elsewhere in the code. This and making sure I use the new function right w/ nested form arrays would make me think more.

so people would just write '#default_value' = NULL or something in their form? A bit awkward, but not too too bad.

Yeah, a if (!isset(... looks a more natural one to me. If you really do not want a default value, you can set it to NULL but really most of all forms wont need it and the automatic default will do.

Good idea.

Iteration through the form elements should use element_children().

You have no idea how often I've forgotten to make a field load its current value, so +1.

I think if there is no way to save the thing other than as a variable, there is no point in loading it from elsewhere either. If the current setting must be overridden, I'd rather you explicitly delete it than implicitly not load it - the default behavior should be to use the current setting.

No patch here.

No patch is "active".

Code looks good. Defaults to FALSE (non-automatic)? Thats a minor buzzkill. Could we change that and still keep the patch at a reasonable size?

Code looks good. I did get 1 hunk failure when trying to apply (oddly, the .rej file was empty) - the patch bot will nt be happy with that when he wakes up.

If you go into the patch again, you can remove the TRUE from system_settings_form($form, TRUE). happens several times.

I'm not really convinced that this is an improvement. It makes the API more obscure and less easy. It doesn't hurt to write a little bit more code, if that keeps things more clear and explicit. -1 from me, personally.

This is very handy. I always found having to put in a variable_get() for each element every time I used system_settings_form() was counter-productive and subject to errors.

@David, you're right. I changed my position, and committed this patch to CVS HEAD. Thanks.

I'm marking this 'code needs work' until it has been documented.

How did this got committed? It was CNR and I did set it aside to review for later but then again boom! it gets in. I truly hate when the community review process gets skipped like this. If we defaulted to TRUE, then why we have calls that specify TRUE as a second argument? That's poor code.

Also, this is very unFAPIlike. You recurse through the children like that instead letting the builder do the hard lifting. I thought this what we had #value_callback for. Please fix.

Let's just continue cleaning up code using the current API.

I also suggested adding a simple flag to the form and when that's there then call variable_get. As the form itself is stoerd in $complete_form during form_building that's kinda trivial to do. Like, three lines of code. And I consider this utterly broken and updated the issue status so.

@CHX,

I reviewed this issue with David via IRC when it was first posted. I tested the patch and it worked, didn't throw errors, seemed like a good idea, and I only had one question for Strauss which he promptly answered. Then, the patch got some basic tests written, and it passes those tests.

I broke the patch review process by not posting my followup and setting to RTBC. Sorry man, I forgot! :-)

Cruft removed.

@chx - stop shouting. you made your point. I reviewed this patch, and others did as well. now dries is aware that you think it went in too quickly.

David, whatever approach (like a generic callback added to the inheritence section where currently the variable_get call is and moving the variable_get into a separate function) I take is going to change form.inc, that's quite natural when it already does the recursion. Having such a callback seems useful for the prepopulate contrib module as well. This patch adds that and still removes seven lines of code :)

With less tpyo.

If I'm not mistaken, $form['#inheritance_callbacks'] = array('_system_settings_set_defaults'); in system_settings_form() will overwrite any current values for #inheritance_callbacks, and not add to the array as expected.

If the new argument to system_settings_form is retained, it needs to be documented in the module upgrade guide. Someone already filed an issue on this, which I'm closing as a duplicate.
#625220: changes to system_settings_form()

Now can someone please commit this as immediately and without any review and consideration as the original broken one was?

So, given the prehistory of the issue and the lack of time I set this to be ready.

This needs to be reviewed properly. For example, I think the name '#inheritence_callback' is odd -- or at least not self-explanatory and I'd love to hear what others think of it.

Congratulations for having another WTF , bloat and fuckup in Drupal 7. I can't be bothered to rewrite the absolutely broken test that got committed here without any review.

It's not because you can't be bothered with it, that this should be marked "won't fix". There are other people in the community that can help.

Because it's broken read #28. The functionality is not critical. If it's rolled back, then it's normal.

I'm with chx that this has been committed too soon, that it is a very unflexible way of doing that. But I disagree on the critical qualification.

What an activity. Can someone please instead FIX this? And I wont relent that fixing the current state is critical.

@chx: Can we discuss this overall issue/patch in IRC/Skype?

I neither really like the current implementation, nor do I like the latest patch in there. In effect, this means we need to clean up this mess in D8.

This needs to stay as a D7 bug at least until the documentation is fixed. It's not currently documented in the module upgrade guide.

Similar to chx and Damien, I suggest to roll back this new convenience feature, since it is implemented in a way that is incompatible with certain forms and existing Form API features.

Form elements that expand into multiple and similar form processing code is incompatible with automatic defaults.

@see #820816: #type text_format not compatible with system_settings_form($automatic_defaults = TRUE)

This leaves a big WTF to developers facing the issue, so by introducing something for convenience, we also introduced weirdness that cannot be resolved cleanly for D7. We therefore should roll back this feature and try again in D8, for which we will need to improve the per-element features of Form API anyway.

Oh I am glad that as Dries said "there are other people in the community that can help" and we have seen tremendous progress since I won't fixed it! So many patches and discussions, whee! Edit: it's been five months.

OK. So we have a patch in #43 that made some changes to the committed patch in #23, which was rejected by Dries a few comments afterwards for having obscure variable names. That can certainly be fixed, but does that patch actually solve the problem(s) with #23's patch?

In other words, do we want to completely roll back #23, or is there some way to salvage the new API?

I took a look at #820186 (mentioned above in #56), and understand from that that not everything works with automatic variable loading, but I'm not sure I understand why, or whether we should completely scrap it or fix it up or just document better what works and what doesn't. I guess that's because I don't really understand what works and what doesn't... Can someone sum it up in a few words? And can't the forms that don't work with automatic loading just pass FALSE in to disable it and go back to the old way?

I'm in favor of a complete roll back of #23 for D7, as this indeed makes the API more obscure:
when the developer sets a default value, but a variable with the same name exists, the variable is used instead.
Using the old API, variable_get is used when the form is defined, so the developer can explicitly choose that the variable is used.

The proposed approach only makes sense when providing a default value can be made optional in the form definition, getting the default value in an automatic way (perhaps using #145164: DX: Use hook_variable_info to declare variables and defaults?).

1. The test is very confusing as it used the variable name "automated" and called system_settings_form with FALSE which is not automated.
2. The test is wrong because it did not use identical. NULL passes assertFalse for example.
3. The test is wrong because it does not use form API.
4. The code is wrong because it recursively iterates the form itself instead of hooking into form_builder. Aka the code also bypassed form API.

The child_callback name has been coined by mikey_p and ksenzee and I like it.

with less tyops.

I think the other thing that needs to be done to avoid #56 problems is to update the doc for the function. Here's the patch in #61 with updated doc.

Also, I have reviewed the code and test portion of the patch in #61/#62, and I am in favor of RTBC. As I wrote the doc section, I will not change the status however. Plus we should wait until the test bot springs into action and verifies it is working (which I guess is waiting for http://qa.drupal.org/head-status to get to green).

Then go. When the bot comes back it cna CNW if necessary. Criticals die!

#820816: #type text_format not compatible with system_settings_form($automatic_defaults = TRUE) has been marked as duplicate, but have a patch that fixes the text format issue and docs for upgrade page.

+++ includes/form.inc	18 Jun 2010 19:29:57 -0000
@@ -1443,6 +1443,12 @@
+    if (!empty($form_state['complete form']['#child_callback'])) {
+      foreach ($form_state['complete form']['#child_callback'] as $callback) {
+        $callback($element, $key);
+      }
+    }

This patch introduces a top-level form #property that has an impact on all elements in the form.

I'm quite sure that we do not have such a property and behavior in Form API yet. And I'm not comfortable with the idea of having to start to check properties on the top-level of a $form to find out what exactly happens or happened to all elements in a form.

Consequently, this patch takes the opposite of the direction we need to go: instead of improving the per-element behavior, we introduce "global top-down wildcard behaviors". Effectively, this patch does not solve the incompatibility with #type 'text_format' and other expanding form elements.

If this were 9 months ago, I'd be unhesitatingly in favor of rolling back automatic defaults until a proper solution were figured out. But now, I think we need to look for a solution that would cause as little screaming as possible from contrib authors. We asked developers to take the #D7CX pledge, and we should be as considerate to the ones who made good on it as we can be; otherwise, it'll be hard to ask them to take a #D8CX pledge.

• Rolling back would mean breaking every D7 contrib module that calls system_settings_form() expecting automatic defaults to work. But, in IRC, jhodgdon said this might be less of a problem as what I initially thought it would be, as we never documented automatic defaults in the module upgrade guide, so there might not be that many contrib modules taking advantage of it. Does anyone have a full CVS local copy of contrib in order to find out how big an impact a roll back would have?
• Moving the application of automatic defaults into the form_builder() part of FAPI, as #62 attempts, would cause a change in the relationship between automatic defaults and hook_form_alter(). In HEAD, a change to #default_value in hook_form_alter() happens after automatic defaults have been applied, and therefore, sets the real #default_value. With #62, automatic defaults would be applied after hook_form_alter(). While I think chx is ultimately right, and if automatic defaults exist at all, they should happen during the form_builder() stage, I think this change in when they get applied relative to hook_form_alter() is a worse BC break than a full reversion. So between rolling back, and fixing #62 to address #66, my vote is to roll back. Note, one way to make #62 address #66 is to have _system_settings_set_defaults() be added as a #process function that adds defaults to its children and adds itself as a #process for those children, but again, my preference is to roll back and do automatic defaults right in D8.
• A final option is to leave automatic defaults with their half-baked implementation: fix issues with #type=text_format and maybe similar elements, but otherwise, simply document that automatic defaults only apply to elements that exist at the time system_settings_form() is called, and not to elements that get added by hook_form_alter() or in a #process function. This is best from a BC perspective, but is an annoying WTF, so what would cause more screaming from contrib authors? If it turns out there are a lot of contrib modules already relying on automatic defaults, then perhaps this option is best. But otherwise, I think rolling back is preferable to the confusion that the current implementation would cause.

Reading the later comments on this issue, I think there's a growing consensus to roll back. But I'm not sure it's a sufficient enough consensus yet. Any disagreements? At some point, we may need to have Dries give his opinion, so as not to waste time writing a roll back patch that he won't approve.

Adding the WTF tag, because of the way in which the current implementation interacts with elements added by hook_form_alter() and #process.

By the way, I just want to also say that I mean no offense to the people who worked on this issue prior to #23. I think the idea is a good one. It's not at all uncommon for problems to turn up with an initial implementation of any idea, let alone one involved with FAPI. It's a shame we're this far into D7 with an implementation that's causing these problems, and now we just need to find the best path forward as a result. Regardless how we proceed for D7, I really want to see this functionality in D8.

I changed my mind on my current preference for how to proceed. While I can go along with a decision to roll back automatic defaults completely if that's what the community decides, I think #67.3 is acceptable if documented. Here's a patch that does that. It makes it clear that hook_form_alter() and #process happens AFTER automatic defaults are applied and that therefore those functions are responsible for implementing their own variable_get() if needed. It also adds a #system_settings_automatic_default_value property to allow individual elements to opt-out of having their #default_value changed with a variable_get() call, so this can be used when adding a 'text_format' element to a system settings form, as was the issue in #820816: #type text_format not compatible with system_settings_form($automatic_defaults = TRUE). Note, this new property is not a new FAPI property: it's a property that only has meaning when the system_settings_form() helper function is called, and the property is prefixed with the module name that defines its meaning. This is a normal pattern, similar to #node_edit_form.

To me, something called
#system_settings_automatic_default_value
sounds like I am giving the value. Maybe a different name like
#system_settings_use_automatic_default
would make it more self-documenting that it should be set to TRUE/FALSE?

Other than that, I think the documentation and code here is clear, and I think the approach is reasonable, given that this was originally committed a while back.

Thanks, I agree that #system_settings_use_automatic_default is a better name.

Sure. Please add a comment saying "This is a horrible hack to be removed in Drupal 8". We do not want people to get an idea it's ok to recurse their forms.

+++ modules/system/system.module	23 Jun 2010 20:24:23 -0000
@@ -2568,32 +2568,63 @@ function system_default_region($theme) {
+  // @todo For Drupal 8, instead of recursively applying automatic defaults
+  //   here, integrate automatic defaults into form_builder() processing.

@chx: here's what the comment is in the latest patch. Are you wanting something stronger? Perhaps within form.inc somewhere explaining how to properly recurse?

Powered by Dreditor.

Discussed with chx in IRC, and yes, the comment needs to be much stronger, and I think belongs in form.inc, linked to from _system_settings_form_automatic_defaults(). I'll work on that, so updating the issue attributes, but more reviews on approach are still welcome.

I have not tested the patch, but the code in #72 seems not to fix the automatic value loading bug of #type text_format - that the patch in http://drupal.org/node/820816#comment-3106520 fixes. Nevertheless the autoloading may be an unloved hack or not, the autoloading should also work for 'text_format' if this autoloading feature is kept. After I understood how the loading works I think this is really something that is a cool idea, but it was only sooo confusing that it had this buggy behavior. But on the end of the day if the autoloading and upgrade docs would have been existed - it wouldn't have caused so much confusion to me. Why are you not integrating my patch?

@hass: I disagree about having _system_settings_form_automatic_defaults() contain special-case logic for 'text_format'. 'text_format' is one example of an element type that's incompatible with auto-default. There may be others (even if not in core, then in contrib). #72 adds a property #system_settings_use_automatic_default that can be used to deal with the text_format problem. A settings form that adds a 'text_format' element needs to set #default_value and #format using variable_get() calls and set #system_settings_use_automatic_default to FALSE. We need to make auto defaults better in D8, but for D7, I don't think that's too bad of a compromise.

Re #73: @chx: How's this?

Also, I have more form_builder() documentation in #736298: Refactor file.module to use proper button click detection, enabling FAPI to improve button click detection security.

Made the @todo comment in _system_settings_form_automatic_defaults() even more strongly worded.

I very much like the documentation in the patch above. +1 for RTBC, but will let someone else review it and set the status.

These tests seem to cover it, and documentation was reviewed in #80, so rtbc.

If this is perceived as a 'critical' problem (and according to the status field it is), then it could be rolled back IMO. To some extend, this is why we're still in alpha and not in a release candidate.

I'm fine with rolling this back. It sounds like it was a bit contentious to begin with, and introduces problems for complex form elements. For once, our annoying habit of not documenting changes immediately things works out in our favour. ;P~

Everyone hold: we have BOTH committers on board for a rollback. Quick, someone do a rollback patch before they change their minds!

Some sloppy follow-up patch must have changed the default value of $automatic_defaults, without updating settings forms throughout core accordingly.

Additionally, many forms in core are manually using variable_get() AND automatic defaults.

Many clear signs of something going wrong.

We still want to commit the Form API documentation improvements by effulgentsia. Let's create a separate issue for them and link to it here.

yay for my first patch since months... that needs a proper review.

+++ modules/system/system.admin.inc	25 Jun 2010 14:10:45 -0000
@@ -1797,7 +1796,7 @@ function system_image_toolkit_settings()
-  return system_settings_form($form, TRUE);

In this function the default value isn't restored:
'#default_value' => variable_get('image_toolkit', image_get_toolkit()), so this would become
variable_get('image_toolkit', $current_toolkit) I suppose.

$current_toolkit is the toolkit that is effectively used, instead of the one currently configured.

Also, the call in system_performance_settings isn't included in this patch.

Powered by Dreditor.

Wow, that was quite a list to go through :)

Re #85 about still wanting the form_builder() documentation, but not in this issue any more: I merged it in with #736298: Refactor file.module to use proper button click detection, enabling FAPI to improve button click detection security, which also has a lot of new form_builder() documentation, so all of it can be reviewed together.

Reroll fixing image_kit issue and also removes a call to system_settings_form which still had 2 parameters. system_performance_settings doesn't need amending as it doesn't use default values.

@marcingy: The call to system_settings_form you mention in #88 is the one in system_performance_settings :)

The changes to modules/locale/locale.admin.inc got lost in the latest patch. Is there a reason you left this out?
(though I'm not entirely sure whether or not that change will cause problems or not)

locale changes back in.

This is good to go for me.

This is a clean rollback. Committed to CVS HEAD. Thanks sun et al.

This is a clean rollback. Committed to CVS HEAD. Thanks sun et al.

We need to double-check this is not referenced in the D6 to D7 module upgrading. This also needs to be broadcast on the devel list as this broke any modules ported to D7 that used this.

Confirmed that original feature was not documented or referenced in http://drupal.org/node/224333, so roll back requires no documentation. rfay sent an email to the dev list yesterday with the notification of the roll back.

Awesome. Thanks for double-checking.