Convert menu_configure() to new configuration system

This likely still needs work, but should get us closer:

• Added default configuration (yml) for menu module (did I name the file right? does anything else need to be done?)
• Fixed instances of variable_get() in core/includes/menu.inc that needed changing.
• I also cleaned up a couple whitespace errors.

+++ b/core/includes/menu.inc
@@ -2255,7 +2256,7 @@ function menu_set_active_menu_names($menu_names = NULL) {
-    $active = variable_get('menu_default_active_menus', array_keys(menu_list_system_menus()));
+    $active = config('menu')->get('default_active_menus', array_keys(menu_list_system_menus()));

The new configuration system does not support default values anymore.

When a default value behavior is desired, you need to get the value of the key, and if that returns no value, then override it with the desired default value.

E.g., in this case:

$active = config(...)->get(...);
if (empty($active)) {
  $active = array_keys(...);
}

+++ b/core/includes/menu.inc
--- /dev/null
+++ b/core/modules/menu/config/menu.yml

The configuration object name should be 'menu.settings'

+++ b/core/modules/menu/config/menu.yml
@@ -0,0 +1,3 @@
+main_links_source: main-menu
+secondary_links_source: user-menu
+override_parent_selector: false

Let's clean up these keys to make the config object more self-explanatory and leverage sub-keys for doing so. For example:

source:
    main_links: main-menu
    secondary_links: user-menu
active_menus: []
override_parent_selector: '0'

1) active_menus was default_active_menus. The actual YAML syntax for an empty array needs to be double-checked.

2) On override_parent_selector: Please note that, as of now, all configuration values are always strings; so a Boolean false would not be supported.

+++ b/core/modules/menu/menu.admin.inc
@@ -664,6 +664,8 @@ function menu_reset_item_confirm_submit($form, &$form_state) {
 function menu_configure() {
+  $config = config('menu');

When assigning the config object to a variable way out of context like this (which is fine), then we need to use a self-descriptive variable name; e.g., $menu_config.

+++ b/core/modules/menu/menu.admin.inc
@@ -671,26 +673,42 @@ function menu_configure() {
+function menu_configure_submit($form, &$form_state) {
+  $config = config('menu');
+  $config_items = array('main_links_source', 'secondary_links_source');
+  foreach ($config_items as $config_item) {
+    if ($config->get($config_item) != $form_state['values'][$config_item]) {
+      $config-set($config_item, $form_state['values'][$config_item]);
+    }
+  }
+  $config->save();

There's no need to check for the existing values. Let's use the standard pattern here - just simply:

config('menu.settings')
  ->set('foo', $bar)
  ->set('baz', $beer)
  ->save();

+++ b/core/modules/menu/menu.install
@@ -60,6 +60,10 @@ function menu_install() {
+  // Set default config values.
+  config('menu')->set('default_active_menus', array_keys(menu_get_menus()));
+  config('menu')->save();

1) It looks like this code did not exist before -- are we sure that it is required, given that menu_set_active_menu_names() computes the default value automatically already?

2) In any case, the config object cannot be saved this way, since you're re-instantiating a completely new object in the second line.

+++ b/core/modules/menu/menu.module
@@ -375,7 +378,10 @@ function menu_parent_options($menus, $item, $type = '') {
     // If a node type is set, use all available menus for this type.
-    $type_menus = variable_get('menu_options_' . $type, array('main-menu' => 'main-menu'));
+    // @todo: Node module will have to handle the setting of this config value.
+    // $type_menus = variable_get('menu_options_' . $type, array('main-menu' => 'main-menu'));
+    $type_menus = config('menu')->get('options_' . $type);

ok, this is going to be tricky. :-/

The patch converts only the menu settings. other menu stuff should be converted, when we have a solution for #1175054: Add a storage (API) for persistent non-configuration state.

Tagging.

+++ b/core/includes/menu.incundefined
@@ -1756,21 +1756,36 @@ function menu_list_system_menus() {
+  $data = $config->get();

There are better ways to check if a module is enabled

Maybe this code is better?

$menu_enabled = module_exist('menu');
$main_links_source = $menu_enabled ? $config->get('source.main_links') : 'main-menu';
$secondary_links_source = $menu_enabled ? $config->get('source.main_links') : 'user-menu';

-29 days to next Drupal core point release.

yes looks better so.

+++ b/core/includes/menu.inc
@@ -1756,21 +1756,32 @@ function menu_list_system_menus() {
+  // Example in Drupal\system\Tests\Menu\RebuildTest.
...
+  // Example in Drupal\system\Tests\Menu\RebuildTest.

Let's remove this -- functional code should not refer to tests. :)

+++ b/core/includes/menu.inc
@@ -1756,21 +1756,32 @@ function menu_list_system_menus() {
+  $main_links_source = $menu_enabled ? $config->get('source.main_links') : 'main-menu';
+  $secondary_links_source = $menu_enabled ? $config->get('source.main_links') : 'user-menu';

I guess the second line intends to get the source.secondary_links key instead of the same for main links?

+++ b/core/modules/menu/config/menu.settings.yml
@@ -0,0 +1,3 @@
+source:
+    main_links: main-menu
+    secondary_links: user-menu

If these two are really the only settings of Menu module, then I wonder whether the additional 'source' parent key makes sense?

+++ b/core/modules/menu/menu.admin.inc
@@ -687,26 +688,38 @@ function menu_configure() {
-  $form['menu_main_links_source'] = array(
...
+  $form['main_links_source'] = array(
...
-  $form['menu_secondary_links_source'] = array(
+  $form['secondary_links_source'] = array(

Let's not change the $form array keys for now, please.

+++ b/core/modules/menu/menu.install
@@ -69,3 +69,14 @@ function menu_uninstall() {
+function menu_8000() {

Missing "_update_" in "menu_update_8000" :)

+++ b/core/modules/menu/menu.install
@@ -69,3 +69,14 @@ function menu_uninstall() {
+    'menu_secondary_links_source' => 'source.secondary_links'

Missing trailing comma for last array element.

Fixed all mistakes ;-)

+++ b/core/modules/menu/menu.admin.inc
@@ -679,7 +679,8 @@ function menu_reset_item_confirm_submit($form, &$form_state) {
+function menu_configure($form, $form_state) {

&$form_state is still not taken by reference though :)

+++ b/core/modules/menu/menu.admin.inc
@@ -701,12 +702,24 @@ function menu_configure() {
+function menu_configure_submit($form, &$form_state) {
+  config('menu.settings')
+  ->set('source.main_links', $form_state['values']['main_links_source'])
+  ->set('source.secondary_links', $form_state['values']['secondary_links_source'])
+  ->save();

1) Sets the old/previous config keys.

2) Looks up the old/previous keys in the submitted form values (they equal the keys in the $form array).

3) Chained method calls on an object should be indented with two spaces.

+++ b/core/modules/menu/menu.install
@@ -69,3 +69,14 @@ function menu_uninstall() {
+    'menu_main_links_source' => 'source.main_links',
+    'menu_secondary_links_source' => 'source.secondary_links',

Ditto on config keys.

Ok fixed this also and a typo module_exist -> module_exists

+++ b/core/modules/menu/menu.admin.incundefined
@@ -701,12 +702,24 @@ function menu_configure() {
+    ->set('source.main_links', $form_state['values']['menu_main_links_source'])

remove "source."

+++ b/core/modules/menu/menu.admin.incundefined
@@ -701,12 +702,24 @@ function menu_configure() {
+    ->set('source.secondary_links', $form_state['values']['menu_secondary_links_source'])

same

-30 days to next Drupal core point release.

fixed also that ;-)

Thanks! This looks ready to fly for me.

Couple of minor nitpicks...

+++ b/core/modules/menu/menu.admin.incundefined
@@ -679,7 +679,8 @@ function menu_reset_item_confirm_submit($form, &$form_state) {
 /**
  * Menu callback; Build the form presenting menu configuration options.
  */
-function menu_configure() {
+function menu_configure($form, &$form_state) {

PHPDoc block needs:
@ingroup forms
@see menu_configure_submit()

+++ b/core/modules/menu/menu.admin.incundefined
@@ -701,12 +702,24 @@ function menu_configure() {
+/**
+ * Form submission handler for menu_configure().
+ *
+ * @see menu_configure()
+ */
+function menu_configure_submit($form, &$form_state) {

Unnecessary @see in PHPDoc block... the function description covers this.

Added suggestions from #21.

#19: 1697170-19-convert-menu_configure-to-new-configuration.patch queued for re-testing.

#22: 1697170-22-convert-menu_configure-to-new-configuration.patch queued for re-testing.

This is good to go. Thanks everyone for the good work!

Committed to 8.x. Nicely done. Thanks all.