Finish converting menu.module to CMI

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.

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.

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.

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');

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?

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'])

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();

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.

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.

This addresses #13.1 and #13.2

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

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.

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.

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.

move menu_default_active_menus to 2106097

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?

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

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.

Here's a patch with that change

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?

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

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

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.