Part of #1696224: Remove system_settings_form()
menu_configure uses system_settings_form() and needs to use system_config_form().

Files: 
CommentFileSizeAuthor
#22 1697170-22-convert-menu_configure-to-new-configuration.patch5.21 KBdagmar
PASSED: [[SimpleTest]]: [MySQL] 39,773 pass(es). View
#19 1697170-19-convert-menu_configure-to-new-configuration.patch5.18 KBtobiasb
PASSED: [[SimpleTest]]: [MySQL] 39,771 pass(es). View
#17 1697170-17-convert-menu_configure-to-new-configuration.patch5.2 KBtobiasb
PASSED: [[SimpleTest]]: [MySQL] 39,753 pass(es). View
#15 1697170-15-convert-menu_configure-to-new-configuration.patch5.19 KBtobiasb
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View
#13 1697170-13-convert-menu_configure-to-new-configuration.patch5.49 KBtobiasb
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View
#10 1697170-10-convert-menu_configure-to-new-configuration.patch5.41 KBtobiasb
PASSED: [[SimpleTest]]: [MySQL] 39,750 pass(es). View
#7 1697170-7-convert-menu_configure-to-new-configuration.patch13.81 KBnick_schuch
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View
#3 1697170-convert-menu_configure-to-new-configuration-3.patch12.76 KBadamdicarlo
FAILED: [[SimpleTest]]: [MySQL] 35,631 pass(es), 1,157 fail(s), and 9,186 exception(s). View
#3 1697170-convert-menu_configure-to-new-configuration-3.interdiff.txt4.27 KBadamdicarlo
#1 1697170-convert-menu_configure-to-new-configuration.patch9.52 KBnick_schuch
FAILED: [[SimpleTest]]: [MySQL] 37,062 pass(es), 28 fail(s), and 422 exception(s). View

Comments

nick_schuch’s picture

Status: Active » Needs review
FileSize
9.52 KB
FAILED: [[SimpleTest]]: [MySQL] 37,062 pass(es), 28 fail(s), and 422 exception(s). View

First attempt at this patch.

Status: Needs review » Needs work

The last submitted patch, 1697170-convert-menu_configure-to-new-configuration.patch, failed testing.

adamdicarlo’s picture

FileSize
4.27 KB
12.76 KB
FAILED: [[SimpleTest]]: [MySQL] 35,631 pass(es), 1,157 fail(s), and 9,186 exception(s). View

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.
adamdicarlo’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1697170-convert-menu_configure-to-new-configuration-3.patch, failed testing.

sun’s picture

+++ 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. :-/

nick_schuch’s picture

FileSize
13.81 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View

Second attempt. I have tried to take on all the advice in comment #6. Let me know what you think.

nick_schuch’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1697170-7-convert-menu_configure-to-new-configuration.patch, failed testing.

tobiasb’s picture

Status: Needs work » Needs review
FileSize
5.41 KB
PASSED: [[SimpleTest]]: [MySQL] 39,750 pass(es). View

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.

sun’s picture

Tagging.

aspilicious’s picture

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

tobiasb’s picture

FileSize
5.49 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View

yes looks better so.

sun’s picture

Status: Needs review » Needs work
+++ 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.

tobiasb’s picture

Status: Needs work » Needs review
FileSize
5.19 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View

Fixed all mistakes ;-)

sun’s picture

Status: Needs review » Needs work
+++ 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.

tobiasb’s picture

Status: Needs work » Needs review
FileSize
5.2 KB
PASSED: [[SimpleTest]]: [MySQL] 39,753 pass(es). View

Ok fixed this also and a typo module_exist -> module_exists

aspilicious’s picture

Status: Needs review » Needs work
+++ 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.

tobiasb’s picture

Status: Needs work » Needs review
FileSize
5.18 KB
PASSED: [[SimpleTest]]: [MySQL] 39,771 pass(es). View

fixed also that ;-)

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! This looks ready to fly for me.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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.

dagmar’s picture

Status: Needs work » Needs review
FileSize
5.21 KB
PASSED: [[SimpleTest]]: [MySQL] 39,773 pass(es). View

Added suggestions from #21.

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

The last submitted patch, 1697170-22-convert-menu_configure-to-new-configuration.patch, failed testing.

dagmar’s picture

Status: Needs work » Needs review
dagmar’s picture

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

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

Dries’s picture

Status: Reviewed & tested by the community » Fixed

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

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