Problem

  • Variables are converted to the new configuration system without any actual conversion besides the API/storage.

Goal

  • Setup proper standards/guidelines for how to convert variables into config objects.
  • Prevent us from having to redo everything (or alternatively, having totally bogus/wonky config names and keys in D8).

Guidelines

  1. Within your conversion issue work, convert one variable at a time.
    • Determine the variable name to convert.
    • Grep the entire Drupal code base for the variable name and identify all instances that need to be updated.
    • Speed up greps by checking /core first. But do not forget to grep /profiles.
  2. Keep configuration object names short and concise.
    • Given a module settings form system_site_information_settings_form() that manages the variables:
      site_name
      site_mail
      site_frontpage
      site_403
      ...
    • Understand that the primary namespace/owner of the configuration object is System module, so the configuration object name has to start with system..
    • Do not convert the form constructor function's name as-is for the configuration object name (system.site-information). No Drupal developer wants to have to remember or deal with needlessly long names on a daily basis.
    • Instead, find the common denominator and shortest base name of all variables (site) and choose an appropriate configuration object name:
      system.site
  3. Strip the configuration object name (or parts of it) from key names.
    • Given a new configuration object name:
      system.site

      and a variable name:

      site_name
    • Understand that converting site_name into system.site:site_name is bad-ass DX. ;)
    • Instead, remove the duplicate site_ from the key name; i.e.:
      system.site:name
  4. Leverage sub-keys.
    • Determine and understand the meaning of the entire set of variables to be converted.
    • Given a subset of variables:
      site_frontpage
      site_403
      site_404
    • Research a bit to understand that all of them refer to internal router paths that denote pages to be shown in certain cases.
    • Do not convert them to meaningless frontpage, 403, 404.
    • Instead, leverage sub-keys and introduce DX sanity for D8 developers:
      page.front
      page.403
      page.404
  5. Direct access, or provide context.
    • config('system.site')->get('name') is fine when being invoked only once within a function.
    • It's also fine, if the respective code is only reached through certain code conditions.
    • However, when config('system.site') is unconditionally used multiple times within a function, then only invoke it once.
    • Unless the context is crystal clear, try to avoid the ambiguous variable name $config.
    • Instead, use a self-descriptive variable name; e.g.:
      $site_config = config('system.site');
  6. Use system_config_form().
    • For a simple settings form somemodule_form(), write a form submission handler to save the configuration; e.g., somemodule_form_submit().
    • End the form with:
        return system_config_form($form, $form_state);

      This retains a consistent form actions container and submit button on settings forms. It also adds a handler for outputting a consistent (positive) confirmation message after form submission. However, it does NOT save any configuration like system_settings_form() did before. somemodule_form_submit() is responsible for that.

    • This takes effect until #1324618: Implement automated config saving for simple settings forms is resolved.
Files: 
CommentFileSizeAuthor
#52 1599554-52.patch552 bytesheyrocker
PASSED: [[SimpleTest]]: [MySQL] 50,482 pass(es).
[ View ]
#36 1599554-36.system-config-form.patch3.81 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 36,999 pass(es).
[ View ]
#36 interdiff.txt2.49 KBeffulgentsia
#32 1599554-32.system-config-form.patch3.31 KBdww
PASSED: [[SimpleTest]]: [MySQL] 36,978 pass(es).
[ View ]
#25 drupal8.system-config-form.25.patch3.14 KBsun
PASSED: [[SimpleTest]]: [MySQL] 36,969 pass(es).
[ View ]

Comments

heyrocker’s picture

Priority:Critical» Normal
Status:Active» Postponed

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.

sun’s picture

Category:bug» task
Priority:Normal» Major
Status:Postponed» Active
sun’s picture

Issue summary:View changes

Updated issue summary.

webchick’s picture

Priority:Major» Normal
Issue tags:-Novice

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.

sun’s picture

Status:Active» Closed (works as designed)

There doesn't seem to be common/shared interest in this. I will cater for the necessary fixes myself.

heyrocker’s picture

Status:Closed (works as designed)» Active

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.

heyrocker’s picture

Issue summary:View changes

Updated issue summary.

sun’s picture

Updated.

FWIW, all of this only rewords my original review of one of the novice patches in #1496542-66: Convert site information to config system either way...

And no, this issue is not about a "plan", but a mere "tutorial" for how to approach conversions. Sanity requires human evaluation.

Anyway. No need to waste any more time on discussing this. I thought I'd contribute something useful, not only for others, but also for myself to be able to focus on other things. But apparently, I did not. Doesn't matter.

cosmicdreams’s picture

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.

dww’s picture

@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

catch’s picture

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

sun’s picture

The actual conclusion that we've reached is that I'm going to review and potentially revise all config conversion patches myself. 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.

#1496542: Convert site information to config system thus is the first one that has been revised and is ready.

That one is special on its own, since it revealed that the upgrade path doesn't work properly -- it clarified that the configuration system architects/developers should have performed a few of the first/initial conversions on their (our) own in the first place.

re: #9: config in include files: Not so simple. A few considerations:

  1. First and foremost, let's make this clear: No novice issue/patch should deal with them. None of those variables are novice.
  2. Configuration objects are namespaced by module/extension. If something in includes is configured by System module, then it's most probably OK to put the values into a system.[include] config object.
  3. Practically, that's it, I guess. But anyway, to squeeze this out a little more:
    • Namespacing the config being used by includes into System module's namespace will work as long as System module cannot be uninstalled. Uninstalling a module will also delete all config objects that start with the namespace. So if System module can ever be uninstalled, doing so will/would delete all the config for includes, too. System module is not optional and (unfortunately) won't likely be anytime soon though.
    • As long as the config is managed by System module and shipped with System module, I wouldn't be happy with a different namespace à la core.[include] being contained in core/modules/system/config. And, needless to say, core. is the namespace of Core module (if any).
    • The config system does not provide a facility to allow subsystems (mainly thinking of core/lib/Drupal/Core stuff) to provide default config. That's not on the roadmap (yet) and I don't think this has ever been discussed, and I'm neither sure whether it would make sense, nor whether there's any desire to discuss it. But anyway, the logical namespace for that would likely be drupal.core.[component].
sun’s picture

Also. Please excuse my ignorance, but what's the logic behind holding off with a commit that performs a polished conversion, after committing direct conversions (and possibly even continuing to do so)...?

sun’s picture

Issue summary:View changes

Updated issue summary.

catch’s picture

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.

dww’s picture

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

dww’s picture

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.

sun’s picture

Well. Please also understand my perspective:

  1. 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').
  2. Our track record of introducing fancy new features but leaving the world with a horrible DX is anything but small. Contrary to others, I do not believe that we will have 1) sufficient time and 2) sufficient contributors to iterate over all config objects and their keys once again in the D8 cycle.
  3. Adjusting the conversions that are happening right now takes only a few minutes each. For properly converted variables, we can at least forget about the structure of the configuration data already, because that is done.
  4. Finding sane and proper names, keys, and structures is trivial for the most part. There are only a few exceptions that might trigger some issue comments to figure out, but that's perfectly fine and acceptable.
  5. The conversion from variables into config requires a upgrade path, which has to be edited/replaced or advanced on, depending on the release cycle phase.
  6. In light of 1), I have to decide between A) spending a few minutes now or B) rewinding and revising all configuration object data from scratch, once more, and adjusting the upgrade path (either in-place or on top). The answer to that is crystal clear.
heyrocker’s picture

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.

heyrocker’s picture

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.

sun’s picture

Status:Active» Closed (works as designed)

We're wasting our time here. Let's move on with the direct conversions. I will adjust them, either before commit or after.

heyrocker’s picture

Status:Closed (works as designed)» Active

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

webchick’s picture

Status:Active» Needs review

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

cosmicdreams’s picture

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.

sun’s picture

It looks like the only disagreement that actually exists is 6) usage of system_settings_form().

I just wanted to say "fine", but then double-checked once more what system_settings_form() is actually doing, and it does a little bit more. Retaining it and just removing/replacing the #submit handler gives us the following benefits:

  1. Consistent 'actions' container on converted configuration forms.
  2. Consistent 'Save configuration' submit button on converted configuration forms.
  3. Consistent #theme function for all configuration forms.
  4. But most importantly: It retains a way to grep for all configuration forms throughout Drupal core, which will have to be rewritten, once a solution has been found for #1324618: Implement automated config saving for simple settings forms.
  5. NEW: Consistent confirmation message after form submission.

Therefore, (as always) on the grounds of the presumption that it is not guaranteed that there will be solution for #1324618: Implement automated config saving for simple settings forms before D8 gets released (e.g., nuclear bombs erasing all of America and Europe), our normal transition path is to retain at least basically working functionality without regressions.

When not using system_settings_form(), then the actual code for all configuration forms would have to be this:

* [...]
*
* @ingroup config_form_upgrade
*/
function mymodule_settings_form($form, &$form_state) {
  [...]

  $form['actions']['#type'] = 'actions';
  $form['actions']['submit'] = array(
    '#type' => 'submit',
    '#value' => t('Save configuration'),
  );
  // By default, render the form using theme_system_settings_form().
  if (!isset($form['#theme'])) {
    $form['#theme'] = 'system_settings_form';
  }
  return $form;
}

To spell out the differences:

  1. Consistent 'actions' container is a standard pattern throughout Drupal anyway already, but will have to be manually coded.
  2. Consistent submit button and its label is in no way guaranteed (other than patch reviews).
  3. Consistent default theme function needs to be coded in manually. (Not doing so would mean a regression compared to D7. Several contributed and custom themes are actively relying on this.)
  4. @ingroup or a comparable comment needs to be used to "tag" each form in order to find all instances of configuration forms that will have to be migrated to the new config form solution.
  5. NEW: Consistent confirmation message after form submission, which apparently was forgotten in the existing conversions.

In light of that, that's why I recommend to keep point 6) about re-using system_settings_form() as is.

cosmicdreams’s picture

In the spirit of DRY I strongly vote for these standards

sun’s picture

Title:Setup standards/guidelines for how to convert variables into configuration» Tutorial/guidelines for how to convert variables into configuration

Unfortunately, we didn't find time to discuss this in Barcelona, as we've been too busy with major architectural changes for the config system.

The only remaining disagreement is on 6) Usage of system_settings_form(). #16 argues against it, #22 provides reasons for it. My main concerns are really 1) discovery of config/settings forms 2) repetition of code 3) consistent #theme regression.

FWIW, I'd also be happy to introduce a very simple temporary helper function to be used instead of system_settings_form(); e.g., system_config_form().

sun’s picture

StatusFileSize
new3.14 KB
PASSED: [[SimpleTest]]: [MySQL] 36,969 pass(es).
[ View ]

Attached patch contains the temporary helper function.

Note that I've added a new point to #22; the existing conversions introduced UX regressions, because there is no confirmation message after submitting a configuration form anymore. I only realized that after creating this system_config_form() helper.

Also, the lack of $form_state['build_info']['form_id'] is a blatant oversight and the fix should also be backported to D7. Hence, I've created #1659000: $form_id is not recorded/provided in $form_state['build_info']

heyrocker’s picture

Status:Needs review» Fixed

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.

dww’s picture

Status:Fixed» Needs review

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

heyrocker’s picture

Status:Needs review» Fixed

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.

sun’s picture

Status:Fixed» Needs review

Hrm. That misses the additional point that got revealed with the patch in #25:

The converted settings forms introduced UX regressions, because they do not output a confirmation message after submitting the form.

If we can agree on that, I'd highly prefer to do a quick commit of the patch in #25. That simplifies the conversion of settings forms even further. It also does not conflict with any more sophisticated configuration form approach that might be introduced via #1324618: Implement automated config saving for simple settings forms or #1648930: Introduce configuration schema and use for translation — in fact, the more code and behavior is handled by (temporary) helper functions, the easier all conversions will be (regardless of whether to system_config_form() or from that to a new Form API/metadata approach).

heyrocker’s picture

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.

sun’s picture

  1. While the purpose is temporary, only The Oracle knows whether it will, in fact, be temporary and replaced later on.
  2. The new helper does everything the old helper did, with the only exception of handling the configuration values to save.
  3. It ensures a consistent form structure, button labeling, theme function, and also confirmation message after form submission, resolving all possible pitfalls and regressions.
  4. We know that we're using the helper, because we'll be using it for all converted settings forms. This very patch adds the first usage already.
  5. In the unlikely but possible case of not being able to introduce a more sophisticated way for configuration forms until feature freeze, this "temporary" system_config_form() would actually be viable and working code that could be released.
  6. What I pointed out in #15 was based on the problem of having to convert variables to config more than once. The track record of introducing new stuff in dirty and incomplete ways would rather map to releasing D8 with both system_settings_form() and system_config_form(). Unlike the issue of a "more fancy config form", this, I believe, "cannot" happen, as I think we've essentially locked ourselves into the absolute requirement that there won't be a {variables} table and system_settings_form() in freshly installed D8 sites.
  7. In case there will be a more sophisticated solution at any point in time, this new helper allows us to identify the already converted forms to convert them again more easily.

Since recent follow-ups leave the impression that people don't really care, I'd recommend to just simply call this patch RTBC, ask for a quick commit, and move on.

The form.inc change in this patch is identical to the one in #1659000: $form_id is not recorded/provided in $form_state['build_info'] (in fact, I only removed the other hunks from this patch). It has no actual impact on Form API functionality or anything else, so it is fine to commit it as part of this patch and just move the other issue back to D7. In the opposite case, this can be re-rolled within seconds.

dww’s picture

StatusFileSize
new3.31 KB
PASSED: [[SimpleTest]]: [MySQL] 36,978 pass(es).
[ View ]

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

Status:Needs review» Needs work
Issue tags:-Configuration system, -Config novice

The last submitted patch, 1599554-32.system-config-form.patch, failed testing.

dww’s picture

Status:Needs work» Needs review
Issue tags:+Configuration system, +Config novice

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

Edit: stupid non-deterministic locale tests! :/

sun’s picture

Status:Needs review» Reviewed & tested by the community

Thanks :) I'd suggest to let's simply do this and shift focus back on actual progress and important things.

Will adjust the tutorial in the summary post-commit.

effulgentsia’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new2.49 KB
new3.81 KB
PASSED: [[SimpleTest]]: [MySQL] 36,999 pass(es).
[ View ]
+++ 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.

sun’s picture

well, that's the 100% patch now ;) Thanks! I originally didn't intend to account for the special-case of a base form ID, because I don't know of any settings form that is as simple as a settings form, but at the same time, as complex as a form working with base form IDs ;)

That said, I wanted to re-RTBC this, but I kinda disagree with the conditional addition of the system_config_form_submit() handler, depending on whether #submit is set or not. That condition makes sense for generic Form API, because it has no idea of what kind of form it deals with. But in this case, the caller is invoking system_config_form() for a very clear purpose. If the caller does not want the submit handler that is added by default, thus, no confirmation message for any reason (humm...?), then the solution for that caller is array_pop().

In the end, I don't really care for this detail though... config/settings forms specifying custom/additional submit handlers are very rare.

effulgentsia’s picture

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 :)

dww’s picture

Status:Needs review» Reviewed & tested by the community

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

Thanks!
-Derek

webchick’s picture

Priority:Normal» Critical
Status:Reviewed & tested by the community» Active
Issue tags:+Needs Documentation

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.

dww’s picture

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

sun’s picture

Status:Active» Reviewed & tested by the community
David_Rothstein’s picture

Status:Reviewed & tested by the community» Active

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

webchick’s picture

Hm. Not sure what happened there, sorry!

heyrocker’s picture

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.

heyrocker’s picture

Issue summary:View changes

Updated issue summary.

sun’s picture

Issue summary:View changes

Updated issue summary.

sun’s picture

I've updated the issue summary to state system_config_form().

re: #45: Quoting myself from #1496542-168: Convert site information to config system:

I disagree with changing the array elements in the form structure. That's the entire topic of #1324618: Implement automated config saving for simple settings forms and/or #1648930: Introduce configuration schema and use for translation. Performing any kind of premature change in any conversion has a probability of 99% for being bogus and unnecessary.

One of the existing conversion patches already introduced a bogus premature change, which won't ever happen in that form, so it introduced code that's completely off, and I almost guess that if I hadn't reviewed that patch post-commit no one would have ever noticed.

sun’s picture

Priority:Critical» Normal
Status:Active» Fixed

Copied into brand new Configuration system handbook chapter: http://drupal.org/node/1667896

Status:Fixed» Closed (fixed)

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

webchick’s picture

Title:Tutorial/guidelines for how to convert variables into configuration» Change notice: Tutorial/guidelines for how to convert variables into configuration
Priority:Normal» Critical
Status:Closed (fixed)» Active
Issue tags:+Needs change record

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.

sun’s picture

Hm. The guidelines were added to the online documentation: http://drupal.org/node/1667896

So not sure what to do with regard to a possible change notice — we have the main change notice for the new configuration system already; perhaps it would be best to simply add to that?

Otherwise, the only content the change notice would contain would be a pointer to aforementioned handbook page, and in itself, it would partially duplicate the main config system change notice.

The @todo / @see could be replaced with #1828354: Should singular config objects be instances of a class that extends Entity?

The @deprecated markers are handled in #1848980: Mark variable functions as deprecated until they are removed, which should care for system_settings_form(), too.

webchick’s picture

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

heyrocker’s picture

Status:Active» Needs review
StatusFileSize
new552 bytes
PASSED: [[SimpleTest]]: [MySQL] 50,482 pass(es).
[ View ]

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.

andypost’s picture

Status:Needs review» Reviewed & tested by the community

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

catch’s picture

Title:Change notice: Tutorial/guidelines for how to convert variables into configuration» Tutorial/guidelines for how to convert variables into configuration
Status:Reviewed & tested by the community» Fixed

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

Status:Fixed» Closed (fixed)

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

xjm’s picture

xjm’s picture

Issue summary:View changes

Updated issue summary.