Tutorial/guidelines for how to convert variables into configuration

This is not critical, nor is it even essential. I won't disagree that the DX behind these names is less than ideal, but given the fact that currently core is not even 25% converted and we have at least three critical issues without which the system won't work at all (not to mention performance improvements to address), I find it very difficult to prioritize this. It is a 'nice to have if we have the time to get to it'. It also makes the conversion patches harder for new contributors. One of the reasons I have been telling everyone to simply use the existing variable names is because it means one less thing to think about and removes something that is very bikeshedabble, and this makes the patches a lot easier for new/novice contributors to attack. Finally it throws all the existing conversion patches back into reroll hell yet again.

I realize it will suck to go back and redo the names after we're done, but we absolutely have to prioritize the the things that are truly critical over the things that are simply DX niceties. We have lived with these names for years, we will live with them for a couple more.

As someone who's not familiar with the deep inner workings of the configuration system but has done a conversion patch (well, re-rolled, but it was tricksy), I completely agree with heyrocker on this. It's more than enough as an uninvolved party to wrap your head around how to call the new config system objects/functions properly and to check that things are working properly without also trying to grapple with updating naming conventions. $context_switching--;

I also don't agree that this should hold up other work in D8. It's great if we get to it, but definitely not essential. Downgrading to normal. Please keep it there.

Also, this is not a novice issue. We do not toss novice users into bikesheds. :) The proposal itself seems sane at initial glance, and if agreed upon we could spin off some true novice issues that say "Do this in X module." However, we should take time to discuss this proposal first, and then probably postpone this effort until the CMI conversions are more like 100%; it'll be much easier to crowd-source then.

There is plenty of interest in it, the only dis-interest is in when it gets done. Also, without a consensus on whether this plan is the right way forward, there are no fixes to be had. So lets keep this issue open and talk about the plan, then talk about implementing it after we're all on the same page.

Updated issue summary.

sun: for the record, this issue initially frustrated but now informs me of a proper way to go about making CMI patches. It frustrates because another than the naming convention I feel I was already doing this. It informs because I can adopt this naming strategy and feel that I'm writing code that provides a good developer experience for others.

Thank you for this.

@sun: I think you're absolutely right that we need some guidelines and I totally support your efforts to improve DX. However, after having read the backscroll in IRC last night, and your comments here, I need to say a few things:

• The "debate" as such is really just about the timing and scope. Instead of every CMI patch turning into "convert to CMI and bring sanity to all our variable names" the scope of the initial conversion can be "convert to CMI" and then follow-up issues can be "bring sanity". Doing both simultaneously makes the patches a) harder to write and b) harder to review. So let's do it in phases.
• There's an entire window of the release cycle for these sorts of cleanups. Your time is too valuable now during the period where "anything goes" to be doing these sorts of cleanups. No one can tell you what to do with your time, but we're inviting you to currently spend it on things that can only be done now, unlike "bring sanity" which can happen once the feature freeze hits.
• You're not listening to your comrades, here. ;) We're on your side, and you're taking a hostile, almost belligerent, attitude to anyone who says "yes, we want sanity, but let's not try to do everything in a single patch". You have contributed something useful, and we're grateful for that. We're just disagreeing in the best tactics to implement the strategy. This isn't a disagreement over principles, just tactics. So please let's not turn this into a holy war of DX vs. CMI or whatever. It's not.

In terms of the actual proposal: looks great to me. My only concern is that point #4 about subkeys is a bit subjective and it's not necessarily "Novice" material to sort some of that out. In many cases it'll be obvious. Overall, I think the guidelines are clear, well-written, and should be pretty easy to follow. I'm just pointing out that in some cases it's not necessarily going to be 100% clear-cut, and no matter how thorough these guidelines are, we'll still need to just use some sense and discuss those on a case-by-case basis.

Sounds like the compromise reached in IRC last night was that after each initial CMI conversion patch is RTBC, we open a followup issue that points to these guidelines as the "bring sanity" phase of that conversion. The only downside is that creates a lot of issues. However, the good news is that creates a lot of issues, which are small, self-contained, usually Novice, and therefore much easier to farm out in parallel at code sprints, meetups, whatever.

Are we all cool here now? ;)

Thanks,
-Derek

#8 sounds like a good plan, I'd like to see this sorted out before we commit #1496542: Convert site information to config system since that's based on the guidelines here.

Issue summary generally looks fine for me. One question I'd have would be configuration that's used by stuff in includes, but where the configuration forms are provided by system module - does that belong to the core subsystem or system module in that case in terms of naming?

Because I understood the plan was to do direct conversions, then follow-up with renaming keys later.

I'm fine with that approach in general - it means twice as may patches in the end, but means the individual patches are reviewable, and there's nothing really to review in terms of naming with the current ones being worked on - the naming might be crap but that's because it's a dumb conversion of stuff that was there already.

If we're introducing a new standard, then there needs to be some agreement to that standard before applying it to every issue, otherwise it could mean thrice as much work rather than twice.

I think I agree with the system module analysis - if it's owned by system module, then it's owned by system module. It'll be a bit harder to later on decouple that stuff from system module if we ever make it optional though, but like you say that's a long way off.

Re: #10:

The actual conclusion that we've reached is that I'm going to review and potentially revise all config conversion patches myself.

No. ;) That's the conclusion you came to because you weren't listening to your comrades. I continue to believe the only disagreement is on scope and timing. You seem to believe the disagreement is about "we never value sanity" vs. "just get the basic conversion done, that's all we can do in this release." No one is saying that, but you keep reacting like that's the debate. We're actually saying:

1. We all want sanity
2. There's a period of the release specifically set aside for cleanups exactly like this
3. Doing the conversion and bringing sanity in one patch makes it harder to write and harder to review
4. Our time is too valuable now in the period where "anything goes" to be focused so heavily on the sanity cleanups that can/should happen later

This last point is why passions are so inflamed here. Every hour we spend doing the full sanity conversions now is an hour "wasted" that probably means some feature's just not going to make it into 8.0.

Alas, we spent more time with debating how long it would take to do it properly, than it actually takes to correct all conversion patches in the queue.

Alas, that means you're still not actually listening to and/or hearing the other side of this discussion. :/ I agree the more meta time we spend in this issue, the value of our valuable time is going down. So this is my last try. No one doubts that you're capable of "quickly" bringing sanity to all our variable names. We're just saying "please don't spend that time this week -- let's talk in October" (or whenever the cleanup phase is happening).

Re: #12. Totally agreed. Once we converge on a standard I see no reason to hold off polish/sanity patches if someone writes them. But, I continue to strongly advocate not putting energy into writing those while we're still in open freeze (except for perhaps CMI "maintainers" doing some spot checking to make sure the API is really going to work -- although surely they're going to learn that from the initial conversions much more than from the sanity patches).

However, something that might help in terms of config-related developer context switching: if you're working on the initial conversion and you have a set of configuration variables swapped into your brain's RAM and you're processing it -- if a sane set of variable/config object names comes to you, put it into the summary of the follow-up issue right away as the initial proposal. You don't actually have to rename everything in the code, but to the extent there's any art to the "sanity" phase, it's about coming up with the right names. So folks (like sun) that are determined to think about the whole problem all at once can share the output of their thinking right now, even though the first patch is just the conversion part, while the follow-up issue summary contains the sanity proposal. That means that the novice coming along latter to do the sanity conversion already has the proposal written out, increasing the chances of success. Plus, if there is disagreement about the sanity proposal, folks will have a chance to think about it and discuss it ahead of time (although again, I think that's a waste of precious "full thaw" time and generally those debates can/should happen only once the feature freeze hits).

Thanks,
-Derek

p.s. I meant to qualify "some feature". The quote should really be:

Every hour we spend doing the full sanity conversions now is an hour "wasted" that probably means some feature we all desperately care about is just not going to make it into 8.0.

One of the reasons I was against implementing a new naming convention for the initial conversions is that it could throw novices trying to roll these patches into a potential horrible bikeshed. However, as sun has pointed out, none of these patches have really turned out to be novice level at all. They have all had various quirks and intricacies that make them problematic. I would still like to see some discussion of this new naming standard though (and that is in fact what it is, despite sun's protestations to the contrary.)

It should be noted that 2) and 3) affect not only variable naming, but also file naming and the granularity of said files. This means that this issue is overlapping #1479492: [policy] Define standards for naming and granularity of config files to some extent. I've posted a comment there to allow participants in that issue to comment here if need be.

I disagree with 6) completely. There's no guarantee that #1324618: Implement automated config saving for simple settings forms will get resolved at all, and there's been some discussion in that issue that the whole concept of system_settings_form() should go away. Until that is resolved in one form or another, I'd like to see the conversions continue as they have been, with no mention of system_settings_form() at all. If #1324618: Implement automated config saving for simple settings forms gets fixed, we can just spawn off a bunch of novice issues to convert the forms to use it. I'm sure xjm and zendoodles would be happy to coordinate that during their office hours and sprint times.

All that said I overall agree with these and I really appreciate sun taking the time to think it through and put them together. I just want to make sure we have some consensus and thought about it before we go converting patches all over the place.

I crossposted with both sun and dww (when I was writing mine those weren't up yet) and don't have a lot of time but I have to address this:

There's no way I'm going to accept a Drupal 8 with stuff like config('system.rss-publishing') or config('system.site')->get('site_name').

It is not your choice to accept this or not. We are a community, we will decide this as a community.

This is not closed. We have a proposal but no consensus on it and discussion ongoing.

Sorry, but we still need to agree on what we're adjusting. I see some concerns being raised here with the original proposal. Let's get this issue RTB*C*.

I opened #1606980: Rename site information config to new naming convention so that we could apply these naming conventions to a set of config once these changes have been agreed upon.

In the spirit of DRY I strongly vote for these standards

My main worry with 6) was that we would never get around to implementing system_settings_form() at all, however it now appears there is little chance of that happening. So I actually just withdraw my original objection to #6. We will still have to go back and remove the unset() from the forms we implement later, but on the other hand we're going to have to go back and change everything if we use #25 too. So I'm just going to call this fixed as there hasn't been any other controversy around it and the people I've pinged have no problems with it either (other than everyone agreeing the unset() hack is awful). This also allows us to move on without worrying about getting another patch committed.

So this means that all existing conversion patches need to be rerolled for new names, and the ones that have been committed need to be reopened or have new issues for renaming created.

Before we call this "fixed", can we get a handbook page (or at least an updated issue summary or something) that clearly spells out the final agreed tutorial/guidelines? Is the current summary an accurate representation of the actual plan? I haven't been following closely enough to know if we've been going with "convert + sanity" in the initial patches, or if we're working with just "convert" for now and planning "sanity" later.

Also, sun proposes a patch here with a helper function -- don't we want that committed before this is "fixed"? If so, the patch will probably have to be re-rolled assuming #1659000: $form_id is not recorded/provided in $form_state['build_info'] is committed.

Thanks!
-Derek

In the original proposal, sun outlined the option of simply unset()ting submit functions added by system_settings_form(). He then proposed his patch as an alternate solution. Given that either way requires going back and changing all the affected forms, I'd just a soon use the unset() solution since it doesn't require a temporary function be added that would be much easier to forget to remove down the road. Also it allows this to move forward now rather than wait for the patch to be committed.

Given that this was the only point of contention, the current summary is accurate. For all conversion patches now, we will be moving forward with convert+sanity.

Setting this back as fixed.

Given that the UX regression is temporary, and we are already assuming that we will return to fix system_settings_form(), I don't really mind it for the time being. It seems kind of silly just to add a drupal_set_message() for something we know we're going to throw away anyways. I don't really care about leaving that broken temporarily, and in fact going out of our way to add it was kind of confusing to me when I first looked at the patch.

I'm in general against adding stuff to core that we don't know if we're ever going to use (this loops us back to the rearchitecture issue as well.) As you yourself pointed out in #15, we have a terrible record of cleaning up after ourselves, and it is confusing to people encountering it for the first time. So I'd prefer to just move forward with the original solution.

Or we can just punt the system_settings_form() question entirely and fix this issue for the naming. I'd be fine with that too.

#31 makes perfect sense to me and #25 looks nearly RTBC to my eyes, except system_config_form() is missing PHPDoc for the $form_state @param, and the summary line is over 80 chars wide and has a few cosmetic problems. Here's an updated copy that fixes those.

#32: 1599554-32.system-config-form.patch queued for re-testing.

Edit: stupid non-deterministic locale tests! :/

+++ b/core/modules/system/system.module
@@ -3026,6 +3026,51 @@ function system_settings_form_submit($form, &$form_state) {
+  $form['#submit'][] = 'system_settings_form_submit';

That's the wrong function. We want system_config_form_submit().

+++ b/core/modules/system/system.module
@@ -3026,6 +3026,51 @@ function system_settings_form_submit($form, &$form_state) {
+  $form['#submit'][] = $form_state['build_info']['form_id'] . '_submit';

Ugh. That's introducing a different logic than what drupal_prepare_form() does for $form['#submit'] for all other forms.

+++ b/core/modules/system/system.module
@@ -3026,6 +3026,51 @@ function system_settings_form_submit($form, &$form_state) {
+ * @todo D8: Replace this temporary helper with a more sophisticated solution.

Let's link to the issue where we're working on that.

Here's a fix for these 3. Otherwise, fwiw, I agree with sun and dww that this temporary helper is better than requiring each form being converted from a settings form to a config form having to do unset($form['#submit']) as explained in #6 of the current issue summary, but I'm not gonna fight about that one way or the other; either way is fine.

That said, I wanted to re-RTBC this, but I kinda disagree with the conditional addition of the system_config_form_submit() handler

+++ b/core/modules/system/system.module
@@ -3026,6 +3026,63 @@ function system_settings_form_submit($form, &$form_state) {
+  $form['#submit'][] = 'system_config_form_submit';

There's no if statement here :)

Agreed, #36 is the 100% patch. ;) Back to RTBC.

Thanks!
-Derek

My impression from #30 is that Greg doesn't seem to care one way or the other, and #38 seems to refute #37. So, since this small patch greases the wheels for CMI conversions, and in order to get some of the CMI patches we talked about in Barcelona moving, I'm going to go ahead and put this in.

Committed and pushed to 8.x. Thanks, all!

We still need to move the OP to something that's not a random issue in an issue queue. I suggest a sub-page under http://drupal.org/developing/api. Marking "critical" and "active" for that.

Thanks!

However: did you not actually push, am I blind and I just don't see this at https://drupal.org/node/3060/commits nor in a local Git clone after a fresh pull, or did you commit this with some obfuscated message so I can't find it? ;)

Cheers,
-Derek

Looks like it did get pushed eventually: http://drupalcode.org/project/drupal.git/commit/4f50bc1

Hm. Not sure what happened there, sorry!

One question that came up in #1496542: Convert site information to config system is how we should name form elements to match the configuration names when making settings forms. Right now in core, form elements on settings forms are named to match the variable name they are setting. This is a little different in the new system. For instance:

+++ b/core/modules/system/system.admin.incundefined
@@ -1522,7 +1528,7 @@ function system_site_information_settings() {
   $form['error_page']['site_404'] = array(
...
-    '#default_value' => variable_get('site_404', ''),
+    '#default_value' => $site_config->get('page.404'),

For instance here, would we change the form element to just be '404' or 'page.404'? The latter is more explicit but introduces a dot in element names which may or may not be safe. The former is simpler but could in some edge cases create name collision (not sure that is necessarily worth worrying about.) I would lean towards 'page.404' if we can myself since it matches our existing standard, but I don't really have a strong opinion either way.

Updated issue summary.

Some follow-up items here.

1) This never got a change notice, so marking as a critical task for that.
2) There's a @todo in the code for system_config_form() pointing to an issue that has since been marked won't fix.
3) Therefore, we should probably add a @deprecated marker (at least) to system_settings_form() so people don't continue to use it.

Here's a start of one, since I need a node ID for Pants module: http://drupal.org/node/1910694

This is more properly a change notice for #1324618: Implement automated config saving for simple settings forms. I have added that nid to webchick's change notice and added a more extensive description.

Attached is a patch to remove the @todo, I don't really think #1828354: Should singular config objects be instances of a class that extends Entity? is an appropriate replacement.

I agree with the remainder of sun's comments, not sure what else is desired here.

Nice cleanup as #1324618: Implement automated config saving for simple settings forms closed
2 and 3 in #49 addressed in follow-ups

Committed/pushed the follow-up, I think this can be marked fixed now?

Updated issue summary.