Updated: Comment 0

Problem/Motivation

menu.module contains a number of calls to variable_get, which will not be supported in Drupal 8.

core/modules/menu/menu.module
278 $type_menus = variable_get('menu_options_' . $type, array('main' => 'main'));
411 $menu_name = strtok(variable_get('menu_parent_' . $node->getType(), 'main:0'), ':');
416 $type_menus = variable_get('menu_options_' . $node->getType(), array('main' => 'main'));
534 $default = ($link['mlid'] ? $link['menu_name'] . ':' . $link['plid'] : variable_get('menu_parent_' . $type, 'main:0'));
600 '#default_value' => variable_get('menu_options_' . $type->id(), array('main')),
613 '#default_value' => variable_get('menu_parent_' . $type->id(), 'main:0'),

Proposed resolution

Convert all calls to use CMI

Remaining tasks

User interface changes

API changes

This is a child of #1775842: [meta] Convert all variables to state and/or config systems
Some variables have already been converted in #1829308: Convert variables in menu.inc to config and state
menu_default_active_menus has been moved to #2106097: Finish converting menu_default_active_menus to CMI

Files: 
CommentFileSizeAuthor
#44 2102521-44-menu-module-variables.patch9.62 KBianthomas_uk
PASSED: [[SimpleTest]]: [MySQL] 59,018 pass(es). View
#35 2102521-31-35-interdiff.txt734 bytesianthomas_uk
#35 menu-2102521-35.patch9.62 KBianthomas_uk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu-2102521-35.patch. Unable to apply patch. See the log in the details link for more information. View
#31 2102521-23-31-interdiff.txt1.9 KBianthomas_uk
#31 menu-2102521-31.patch9.77 KBianthomas_uk
PASSED: [[SimpleTest]]: [MySQL] 60,130 pass(es). View
#23 menu-2102521-23.patch9.48 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 59,362 pass(es), 0 fail(s), and 1 exception(s). View
#23 interdiff.txt5.12 KBtim.plunkett
#12 2102521-convert-menu-12.patch7.63 KBianthomas_uk
PASSED: [[SimpleTest]]: [MySQL] 58,409 pass(es). View
#11 2102521-convert-menu-11.patch6.28 KBianthomas_uk
PASSED: [[SimpleTest]]: [MySQL] 58,455 pass(es). View
#9 2102521-convert-menu-9.patch5.15 KBianthomas_uk
FAILED: [[SimpleTest]]: [MySQL] 58,407 pass(es), 2 fail(s), and 0 exception(s). View
#7 2102521-convert-menu-7.patch5.15 KBianthomas_uk
FAILED: [[SimpleTest]]: [MySQL] 58,386 pass(es), 2 fail(s), and 3 exception(s). View
#6 2102521-convert-menu-6.patch5.15 KBianthomas_uk
FAILED: [[SimpleTest]]: [MySQL] 58,852 pass(es), 2 fail(s), and 3 exception(s). View
#4 2102521-convert-menu-4.patch5.14 KBianthomas_uk
FAILED: [[SimpleTest]]: [MySQL] 58,390 pass(es), 18 fail(s), and 260 exception(s). View

Comments

catch’s picture

Priority: Normal » Critical

These all look like node type-specific variables. Might be worth checking if there's another issue dealing with the node type variables that touched these.

ianthomas_uk’s picture

Component: content_translation.module » menu.module

Similar issues seem to be #1739928: Generalize language configuration on content types to apply to terms and other entities and #111715: Convert node/content types into configuration although both relate to other node type-specific variables and have large, committed patches.

node_type_language_default_.$node_type became language.settings.node.$node_type.language.default_configuration

node_submitted_$node_type became node.type.$node_type.settings.node.submitted

Should menu_options_$node_type become node.type.$node_type.settings.menu.options?

UI (at least in 7.x) is at /admin/structure/types/manage/$node_type, menu settings vertical tab.

ianthomas_uk’s picture

Catch was right, there is a related issue open: #2026165: Finish node type settings conversion
Which refers to: #1911080: Replace menu node form additions with entity reference field

To summarise, we can either store this information in the node type settings or in the menu settings. The main purpose of the option seems to be to have a manageable-sized menu to select from when editing a node, which would suggest the node settings is the appropriate place. This also matches the UI.

However, [#9111080] proposes that it should be possible to add any entity type to a menu, which would suggest either storing on the entity (where?) or the menu. I think this just raises a whole load of questions, so for now I'm going to work on a patch that stores them on the node type.

ianthomas_uk’s picture

Status: Active » Needs review
FileSize
5.14 KB
FAILED: [[SimpleTest]]: [MySQL] 58,390 pass(es), 18 fail(s), and 260 exception(s). View

OK, here goes.

I've decided to store under menu using both the entity type (currently hard coded to null) and content type/bundle in the config key. This brings the form handling code from node.module into menu.module*, which should hopefully make it easier to convert to an entity reference-based implementation at some point in the future, or at least extend to other entity types.

When reviewing, please consider if this data is being stored with the best IDs (menu.entity.$entity_type.$content_type, subkeys available_menus and parent).

* I've not removed the code from node.module yet, but I'd like to deal with that later to avoid distractions.

Status: Needs review » Needs work

The last submitted patch, 2102521-convert-menu-4.patch, failed testing.

ianthomas_uk’s picture

Status: Needs work » Needs review
FileSize
5.15 KB
FAILED: [[SimpleTest]]: [MySQL] 58,852 pass(es), 2 fail(s), and 3 exception(s). View

Add back the default that was missed in #4

-    $type_menus =\Drupal::config('menu.entity.node.'.$type)->get('available_menus');
+    $type_menus =\Drupal::config('menu.entity.node.'.$type)->get('available_menus') ?: array('main');
ianthomas_uk’s picture

FileSize
5.15 KB
FAILED: [[SimpleTest]]: [MySQL] 58,386 pass(es), 2 fail(s), and 3 exception(s). View

Oops, also spotted that I changed the submit handler name without changing what it was registered as in the form.

-  $form['actions']['submit']['#submit'][] = 'menu_form_node_type_form_alter_submit';
+  $form['actions']['submit']['#submit'][] = 'menu_entity_settings_form_submit';

Is there a way to cancel the tests on #6?

Status: Needs review » Needs work

The last submitted patch, 2102521-convert-menu-7.patch, failed testing.

ianthomas_uk’s picture

Status: Needs work » Needs review
FileSize
5.15 KB
FAILED: [[SimpleTest]]: [MySQL] 58,407 pass(es), 2 fail(s), and 0 exception(s). View

Hmm, parent form field id was wrong, not sure how I made that mistake.

-    ->set('parent', $form_state['values']['parent'])
+    ->set('parent', $form_state['values']['menu_parent'])

Status: Needs review » Needs work

The last submitted patch, 2102521-convert-menu-9.patch, failed testing.

ianthomas_uk’s picture

Status: Needs work » Needs review
FileSize
6.28 KB
PASSED: [[SimpleTest]]: [MySQL] 58,455 pass(es). View

There was another variable name that I'd incompletely tidied

-    ->set('available_menus', $value)
+    ->set('available_menus', $available_menus)

and a test that needed to be updated

-    variable_set("menu_options_$type", $menus);
-    variable_set("menu_parent_$type", 'tools:0');
+    \Drupal::config('menu.entity.node.'.$type)
+      ->set('available_menus', $menus)
+      ->set('parent', 'tools:0')
+      ->save();
ianthomas_uk’s picture

FileSize
7.63 KB
PASSED: [[SimpleTest]]: [MySQL] 58,409 pass(es). View

One final patch from me (subject to reviews and SimpleTest of course). I've changed the name of the submit handler, ensured it is only attached in one place and improved the commenting around the alter hook/submit handler to match the format used in the field_ui module.

I've also removed the code from NodeTypeFormController that was designed to save these variables (and those of the Comment form, which has already been removed).

I've tested the UI of both the Node Type form and the Node form and checked the yml output, and I think it's all correct, so just need a review now please.

andypost’s picture

Looks mostly good, just minor nitpicks

  1. +++ b/core/modules/menu/menu.module
    @@ -617,6 +627,27 @@ function menu_form_node_type_form_alter(&$form, $form_state) {
    +  foreach ($form_state['values']['menu_options'] as $value) {
    +    if (!empty($value)) {
    +      $available_menus[] = $value;
    

    maybe array_filter() is shorter here?

  2. +++ b/core/modules/menu/menu.module
    @@ -617,6 +627,27 @@ function menu_form_node_type_form_alter(&$form, $form_state) {
    +  \Drupal::config('menu.entity.node.'.$type->id())
    

    needs surrounding spaces around dot

  3. +++ b/core/modules/node/lib/Drupal/node/NodeTypeFormController.php
    @@ -197,32 +197,6 @@ public function save(array $form, array &$form_state) {
    -    // Do not save settings from vertical tabs.
    -    // @todo Fix vertical_tabs.
    -    unset($variables['additional_settings__active_tab']);
    

    seems need manual checking

ianthomas_uk’s picture

RE #13.3 The code in this function saves all values from $form_state as variables, except those that are already handled by the node type form, and this variable which should not persist. $variables is not used outside of the code that I removed.

ianthomas_uk’s picture

FileSize
1003 bytes
7.82 KB
PASSED: [[SimpleTest]]: [MySQL] 58,782 pass(es). View

This addresses #13.1 and #13.2

ianthomas_uk’s picture

FileSize
7.51 KB
PASSED: [[SimpleTest]]: [MySQL] 58,955 pass(es). View

I had some time to test that a bit more carefully and noticed that the array_filter code caused kept the array keys, so the .yml files look like:

available_menus:
  admin: admin
  main: main

I've added an array_values() around this line to fix that.

-    ->set('available_menus', array_filter($form_state['values']['menu_options']))
+    ->set('available_menus', array_values(array_filter($form_state['values']['menu_options'])))

Which outputs:

available_menus:
  - admin
  - main
cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me

ianthomas_uk’s picture

Status: Reviewed & tested by the community » Needs work

Speaking to tim.plunkett in IRC yesterday it sounds like this doesn't follow best practise.
- \Drupal::config() should not take a variable in it's parameter, these should be configuration entities instead
- Defaults should be provided by the configuration system instead of by using :?

Documentation for configuration entities is at https://drupal.org/node/1809494 but I don't understand how they would be used in this scenario. I'll ask tim if he can offer further guidance.

ianthomas_uk’s picture

Status: Needs work » Needs review
FileSize
3.92 KB
9.05 KB
FAILED: [[SimpleTest]]: [MySQL] 59,270 pass(es), 0 fail(s), and 19 exception(s). View

I spoke to tim.plunkett on IRC again and he said that I had misunderstood that \Drupal::config() could not accept variables in it's parameter, it's just that I should be confident that the config set I am trying to access exists and therefore has a value.

For this patch, that means inserting default config when a node type is added/removed, or when the menu module is installed (Drupal will clean up all of menu's config when the module is uninstalled). These changes are in the attached patch.

Status: Needs review » Needs work

The last submitted patch, 2102521-convert-menu-19.patch, failed testing.

aspilicious’s picture

I would put the ->delete on the same line

ianthomas_uk’s picture

I've had another look into this, most of the test failures are in *UpgradePathTest tests. From speaking to chx I understand these tests are superseded by content migration, so we should just remove the tests rather than spend time fixing them.

The remaining test failure is actually on the 'Add content type' and was spotted in passing by BreadcrumbTest. Should there not be a specific test that we can add/remove content types?

To fix the warning we need to provide a default value for available_menus in menu_form_node_type_form_alter() when creating new content types, but that would mean providing the same defaults in three places (others are menu_node_type_insert and menu_install), which isn't great. Is there a better way these could be provided? They can't be included in a .yml file because we don't know the config keys they will be accessed with.

ianthomas_uk’s picture

Issue summary: View changes

move menu_default_active_menus to 2106097

tim.plunkett’s picture

Issue summary: View changes
FileSize
5.12 KB
9.48 KB
FAILED: [[SimpleTest]]: [MySQL] 59,362 pass(es), 0 fail(s), and 1 exception(s). View

Yes, we don't need this in a migrate-based world, but that is not here yet and we need this stuff to pass.
Made a couple other fixes.

tim.plunkett’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 23: menu-2102521-23.patch, failed testing.

cosmicdreams’s picture

Looks like #2084463: Convert contextual links to a plugin system similar to local tasks/actions was committed about 3 hours before this patch and touched on menu system so I'm surprised that you didn't run into issues with this patch. Did you pull before you created the patch?

cosmicdreams’s picture

The problem seems to be this line of code from menu.module (line 629)

'#default_value' => $node_type_config->get('available_menus'),

By reading the menu.schema.yml file I'm a bit confused as to exactly where the problem is. It seems that CMI is smart enough to navigate to specific set of config even if several levels deep in a config structure.

For the sake of argument let's say that get command returns what the yml declares:

array(type => array, label = > 'Available menus')

This is still not the previous data that was passed along:

array('main');

Later on in form.inc's form_type_checkboxes_value, we expect an array to be passed in so can extend it. Then we do a foreach to loop through the values.

The error we get is when that foreach fails. It seems to fail because it doesn't recieve an array to iterate over, the array is deliver to the foreach ultimately because we are sending the right data to form_type_checkboxes_value. (I think)

ianthomas_uk’s picture

The error is on the create node type page, so I think that $node_type_config->get('available_menus') will return null, even though the schema says it should be an array (if you accept the empty string as a matching the * in menu.schema.yml).

I think it would work if we changed
$node_type_config = \Drupal::config('menu.entity.node.' . $type->id());
to

if (!empty($type->id())) {
  $node_type_config = \Drupal::config('menu.entity.node.' . $type->id());
  $menu_options_default = $node_type_config->get('available_menus');
}
else {
  $menu_options_default = array('main');
}

But that means we'd now be setting the default to array('main') in four places - there must be a better way?

xjm’s picture

Issue tags: +beta blocker
xjm’s picture

ianthomas_uk’s picture

Status: Needs work » Needs review
FileSize
9.77 KB
PASSED: [[SimpleTest]]: [MySQL] 60,130 pass(es). View
1.9 KB

No one has suggested a better fix, so here's a patch that implements #28.

tim.plunkett’s picture

+++ b/core/modules/menu/menu.module
@@ -608,7 +608,19 @@ function menu_node_submit(EntityInterface $node, $form, $form_state) {
+    $config_values = array(
+      'available_menus' => $node_type_config->get('available_menus'),
+      'parent' => $node_type_config->get('parent'),
+    );

Why not just $config_values = $node_type_config->get();? I thought that worked.

ianthomas_uk’s picture

Yes, I think we could just call ->get().

I quite like the way each half of the if/else is the same though, so if you change one half then it's a prompt for you to change the other half (e.g. if someone adds a third config value, they would notice it wasn't in $config_values whether or not they were editing an existing content type or creating a new content type).

I don't really mind either way though. I can roll a patch tomorrow with that change if you want.

aspilicious’s picture

Status: Needs review » Needs work

I agree with tim :). Go for the reroll!

ianthomas_uk’s picture

Status: Needs work » Needs review
FileSize
9.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu-2102521-35.patch. Unable to apply patch. See the log in the details link for more information. View
734 bytes

Here's a patch with that change

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Reading through the comments and the patch, I think this is finally done :)

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/menu/menu.install
@@ -40,6 +40,16 @@ function menu_install() {
+      \Drupal::config('menu.entity.node.' . $type_id)

Isn't this going to run into the default config issue with uuids?

ianthomas_uk’s picture

Isn't this going to run into the default config issue with uuids?

Do you mean #2127573: Stop creating node, comment and custom block fields automatically and provide defaults in CMI? If so, yes there is a conflict. That issue is still a bit up in the air though.

Should this be postponed on that, or is it better to get this in and add it to the list of things that need to be converted from hooks to yaml for that issue?

catch’s picture

Assigned: Unassigned » alexpott

Hmm that's a good question. I'd lean towards committing this as is and then adding it to that issue, but since Alex Pott has been working on the uuid stuff double checking.

chx’s picture

ianthomas_uk’s picture

35: menu-2102521-35.patch queued for re-testing.

The last submitted patch, 35: menu-2102521-35.patch, failed testing.

ianthomas_uk’s picture

Assigned: alexpott » ianthomas_uk

#2127573: Stop creating node, comment and custom block fields automatically and provide defaults in CMI has now been won't fixed, so #35 should be ready to commit. It needs a re-roll though, so assigning to myself to do that.

ianthomas_uk’s picture

FileSize
9.62 KB
PASSED: [[SimpleTest]]: [MySQL] 59,018 pass(es). View

The patch didn't apply because the case of a function call had been changed in #2113319: Rename getOriginalID() to getOriginalId() and setOriginalID() to setOriginalId(), so other than the case of one deleted character this is identical to #35, which was RTBC until this issue was postponed on an issue that has since been won't fixed.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

rtbc than :)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Since this was RTBC for a while already, committing it straight away. Thanks!

Status: Fixed » Closed (fixed)

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