Convert menus into entities

+++ b/core/modules/field_ui/field_ui.moduleundefined
@@ -117,7 +117,7 @@ function field_ui_menu() {
+          $items["$path/fields/%field_ui_bundle/edit"] = array(

I'm not sure whether field_ui_bundle_load is good as the function returns a field instance object, not a bundle.

So it would make more sense to do field_ui_instance_load in a way.

+++ b/core/modules/field_ui/field_ui.moduleundefined
@@ -211,7 +211,7 @@ function field_ui_menu() {
-function field_ui_menu_load($field_name, $entity_type, $bundle_name, $bundle_pos, $map) {
+function field_ui_bundle_load($field_name, $entity_type, $bundle_name, $bundle_pos, $map) {
   // Extract the actual bundle name from the translated argument map.

Whats the reasoning behind the rename?

+++ b/core/modules/menu/menu.moduleundefined
@@ -161,6 +164,89 @@ function menu_menu() {
+  // Iterate through each property of the new config, copying it to the test
+  // object.
+  foreach ($new_config->get() as $property => $value) {

What do you mean by test object?

+1 for custom menu being configuration:

* I normally like to deploy menu items via code.
* I like to version control them as while the content might change, the structure usually is fixed.

#9: 1814916-menu-9.patch queued for re-testing.

Rerolled.

Fixed the use statements.

Hmm what caused the testbot failures before?
Because this "looks" like a random failure. And you're using a lot of random names in your tests.

Rerolled, no changes.

This needs a re-roll.

xpost :-D

+++ b/core/modules/menu/menu.admin.incundefined
@@ -480,51 +452,55 @@ function menu_edit_item_submit($form, &$form_state) {
+  /*$menu_id = $menu->id();
   $menu += array(
-    'menu_name' => '',
-    'old_name' => !empty($menu['menu_name']) ? $menu['menu_name'] : '',
+    'id' => '',
+    'old_id' => !empty($menu_id) ? $menu_id : '',
     'title' => '',
     'description' => '',
-  );
+  );*/
   // Allow menu_edit_menu_submit() and other form submit handlers to determine

I'm not sure what the status of this patch is but this version contains commented code. Should that could be removed or what has to happen with it?

+++ b/core/modules/menu/menu.installundefined
@@ -7,7 +7,7 @@
 function menu_schema() {

These schemas should be removed?

+++ b/core/modules/menu/menu.installundefined
@@ -38,11 +38,11 @@ function menu_schema() {
 function menu_install() {

Do we need the install function?

And should we remove the menu_custom table? I saw other conversions where we postpone the removal to D9.

Quoting catch from IRC:

there is already some horrible stuff in system module for the system menus, so I could live with putting it there then a follow-up for a proper home.

If we wanted it somewhere else, we'd have to deal with #1847002: Move entity type classes from Drupal\$provider\Plugin\Core\Entity to Drupal\$provider\Entity and #1821846: Consider better naming conventions for plugin types owned by includes first.

This patch moves everything to system (I hope) and fixes menu.install.

We should use entity_info_alter in that case. But I still see potential problems with the current patch. If you don't enable menu module you still registered the uri callback.

So when you call $menu->uri() it goes kaboom?

The custom menu links that are now in YAML were in menu_install originally anyway, they should only be added if the menu.module is installed.

I moved the list controller, the config_import hooks, and the uri callback.

@aspilicious pointed out that the url of 'admin/structure/menu/manage/' won't exist without menu.module, so we need to move that to the alter too.

Annndddd we have another problem.

The config_import "hooks" are called based on the config_prefix. See #1806178: Remove code duplication for hook_config_import_*() implementations of config entities for other issues with that.

This means that menu_config_import_* will be called unless we rename them system.menu.$foo.yml...

Or, nothing will get imported properly unless menu is enabled.

:(

#42: menus-1814916-42.patch queued for re-testing.

Not sure why that was retested, it's a legitimate failure. It seems that config_install_default_config() is being called for system.module before any plugin classes are loaded. Is that even possible?

#1806178: Remove code duplication for hook_config_import_*() implementations of config entities will allow us to switch back to menu.menu. I'd postpone it, but those exceptions look like they are a separate issue, so we might as well fix that in the mean time.

@berdir helped me track this down, is_subclass_of() was changed in PHP 5.3.9

I'm starting to think that the "fix" above is a coincidence?

Berdir found https://bugs.php.net/bug.php?id=55475 and https://bugs.php.net/bug.php?id=53727, possibly related.

The bots are on 5.3.5, I can reproduce locally with 5.3.6, but Berdir had it pass with 5.3.10 and 5.4.6.

#1806178: Remove code duplication for hook_config_import_*() implementations of config entities went in, so rerolling to re-rename the config files back and removing the config_import_ hooks

+++ b/core/includes/install.incundefined
@@ -448,6 +448,10 @@ function drupal_install_system() {
+  // @todo PHP 5.3.9 changes the behavior of is_subclass_of() with respect to
+  //   autoloaders. To allow earlier versions to properly find system.module,
+  //   warm the module list cache.
+  module_list();

We should upgrade our php requirements...

Besides of that this feels rtbc

Forgot the manifest file update.

+++ b/core/includes/install.inc
@@ -448,6 +448,10 @@ function drupal_install_system() {
+  // @todo PHP 5.3.9 changes the behavior of is_subclass_of() with respect to
+  //   autoloaders. To allow earlier versions to properly find system.module,
+  //   warm the module list cache.
+  module_list();
   config_install_default_config('module', 'system');

Hm. I do not really understand why and how priming of module_list() would change the introspection of a class (and the code comment does not really clarify that either).

I almost suspect that this code is running into some larger environment issues within the installer, which #1798732: Convert install_task, install_time and install_current_batch to use the state system is supposed to fix — essentially: System module is not installed like any other module, which means that its services are not readily available upon installation in the installer. Fixing that requires some larger changes to the installer, as contained over there.

+++ b/core/modules/field_ui/field_ui.module
@@ -211,12 +211,12 @@ function field_ui_menu() {
-function field_ui_menu_load($field_name, $entity_type, $bundle_name, $bundle_pos, $map) {
+function field_ui_instance_load($field_name, $entity_type, $bundle_name, $bundle_pos, $map) {

Hm. That's an unfortunate consequence/problem. Field UI is certainly not the only module that uses such menu argument loading helper functions.

OTOH, the problem seems to be limited to %[module]_menu and thus MODULE_menu_load(), which is rather rare, as you'd normally use MODULE_ENTITY_menu_load(), so this change is probably fine to ignore.

+++ b/core/modules/menu/lib/Drupal/menu/MenuListController.php
@@ -0,0 +1,67 @@
+    $operations['edit'] = array(
+      'title' => t('edit menu'),
+      'href' => $path . '/edit',
+      'weight' => 5,
...
+    $operations['add'] = array(
...
+      'weight' => 10,

1) It would be good to provide more room between these additional operations; e.g., just use steps of 10 instead of 5.

2) It looks like we're missing $uri['options']. Entity URIs are pluggable, so just because the default implementation only uses a path does not mean that there cannot be custom options.

+++ b/core/modules/menu/menu.admin.inc
@@ -47,16 +20,15 @@ function menu_overview_page() {
 function theme_menu_admin_overview($variables) {
-  $output = check_plain($variables['title']);
-  $output .= '<div class="description">' . filter_xss_admin($variables['description']) . '</div>';
-
-  return $output;
+  return check_plain($variables['label']) .
+    '<div class="description">' . filter_xss_admin($variables['description']) . '</div>';

Can we keep the less obscure and more nicely formatted output generation code? ;)

+++ b/core/modules/menu/menu.admin.inc
@@ -480,51 +452,48 @@ function menu_edit_item_submit($form, &$form_state) {
-function menu_edit_menu($form, &$form_state, $type, $menu = array()) {
+function menu_edit_menu($form, &$form_state, $type, $menu = NULL) {

An entity form controller is left to a follow-up issue?

+++ b/core/modules/menu/menu.install
@@ -6,64 +6,6 @@
-    'footer' => $t('Use this for linking to site information.'),

We're missing a default config file for the footer menu.

+++ b/core/modules/menu/menu.install
@@ -126,3 +68,20 @@ function menu_update_8003() {
+    config('menu.menu.' . $menu->menu_name)
+      ->set('id', $menu->menu_name)

Missing UUID.

+++ b/core/modules/menu/menu.module
@@ -186,13 +207,13 @@ function menu_enable() {
   $base_link = db_query("SELECT mlid AS plid, menu_name FROM {menu_links} WHERE link_path = 'admin/structure/menu' AND module = 'system'")->fetchAssoc();
   $base_link['router_path'] = 'admin/structure/menu/manage/%';
   $base_link['module'] = 'menu';

We should create an issue to remove this code. It essentially expands all available config entities into (static) menu links.

+++ b/core/modules/system/lib/Drupal/system/Plugin/Core/Entity/Menu.php
@@ -0,0 +1,60 @@
+ * @Plugin(
+ *   id = "menu",
...
+ *   module = "system",
...
+class Menu extends ConfigEntityBase {

I think we can do this for now, since the overall patch seems to be a very positive improvement.

However, in thinking through this issue over the past days, I wondered whether we need the Menu entity at all, and whether we cannot simply move the entire code for menus into Menu module. As a result of doing so, you would no longer see any menu blocks when Menu module is disabled. (Why would anyone expect any menu to appear without it anyway?) — The router/menu link building should not be affected by that; it can still operate on the 'menu_name' property of menu links. Essentially, we'd turn the 'menu_name' property of menu links into a simple "container"/realm that has no further business logic and also no UI attached to it by default. Only by enabling Menu module, those containers/realms are actually turned into something that has business logic behind it.

But as mentioned, we should discuss that in a separate follow-up issue. The proposed changes here look worth to commit on their own and are a step forward.

+++ b/core/modules/system/lib/Drupal/system/Plugin/Core/Entity/Menu.php
@@ -0,0 +1,60 @@
+   * The file UUID.

Bogus property summary.

I'll update the patch and/or open follow-ups, probably tomorrow.

This is a reroll for the blocks patch, no changes made from #54 yet.

One more try

Wow, thanks :) I didn't really mean to ask for a conversion to EntityFormController right within this issue though. ;)

However, the converted code looks really great and I can only guess that it will come back green. Only have a few picks on it:

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.php
+      '#access' => $menu->isNew() && !isset($system_menus[$menu->id()]),

isNew() needs to be FALSE, not TRUE, in order for delete to make sense. :)

+++ b/core/modules/menu/lib/Drupal/menu/MenuListController.php
   public function buildRow(EntityInterface $entity) {
-    $row['title'] = array(
-      'data' => array(
-        '#theme' => 'menu_admin_overview',
-        '#label' => $entity->label(),
-        '#id' => $entity->id(),
-        '#description' => $entity->description,
-      ),
-    );
+    $row['title'] = check_plain($entity->label());
+    $row['description'] = filter_xss_admin($entity->description);

I actually wanted to propose to take over the theme_menu_admin_overview() style of outputting config entities in a listing as the default for all config entity listings. ;)

However, I'm also fine with discussing and doing that in a separate follow-up issue, so standardizing this admin listing for now looks fine to me. Usability folks will argue that this is a regression though, so let's be clear about that it's only meant to be temporary.

+++ b/core/modules/menu/menu.api.php
 function hook_menu_insert($menu) {

Removal of menu.api.php is more or less bound to #1824184: Remove and replace entity type specific API hook documentation with generic entity API hook documentation

I think we actually did remove API documentation in some of the conversion issues already, but let's not get into that can of worms in this issue. ;)

+function menu_menu_insert(Menu $menu) {
+function menu_menu_update(Menu $menu) {
+function menu_menu_predelete(Menu $menu) {
+function menu_menu_delete(Menu $menu) {

In a separate follow-up issue, we should consider to move these hook implementations into a MenuStorageController. Let's not do that here. Let's make progress instead. :)

To be clear, here's the before and after:

Before:

After:

The only thing I would suggest - I dont think this change is necessarily bad, is to make the left column titles bold.

Here's an adapted mockup of the views listing which already uses bold titles to demonstrate how that would look: http://dl.dropbox.com/u/538835/stuff/Views%20%7C%20d8.png

Lets do the bold thingie and rtbc after that?

Made two small tweaks, but that looks great!
Thanks!

Thanks!

+++ b/core/includes/install.inc
@@ -448,6 +448,10 @@ function drupal_install_system() {
+  // @todo PHP 5.3.9 changes the behavior of is_subclass_of() with respect to
+  //   autoloaders. To allow earlier versions to properly find system.module,
+  //   warm the module list cache.
+  module_list();
   config_install_default_config('module', 'system');

I still think that this comment + data priming is not clear enough - or in other words, I don't really have an idea which exact problem the @todo is referring to and what exactly needs to be architecturally refactored to eliminate the @todo.

I think we want and need to clean this up prior to commit.

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.php
@@ -0,0 +1,103 @@
+      // The title of a system menu cannot be altered.
+      '#access' => !isset($system_menus[$menu->id()]),
...
+      // A menu's machine name cannot be changed.
+      '#disabled' => !$menu->isNew() || isset($system_menus[$menu->id()]),

I don't know why we have these restrictions, but they do not make much sense to me. Ideally, we should create a follow-up issue to remove them.

Let's also not forget about the follow-up issue I mentioned in #54 about moving the Menu entity type into Menu module and thus, detaching the entire Menu UI business logic from the low-level {menu_links}.menu_name container concept.

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.php
@@ -0,0 +1,103 @@
+      '#machine_name' => array(
+        'exists' => 'menu_edit_menu_name_exists',

Isn't this helper function identical to menu_load() and thus obsolete now?

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.php
@@ -0,0 +1,103 @@
+      watchdog('contact', 'Menu %label has been updated.', array('%label' => $menu->label()), WATCHDOG_NOTICE, l(t('Edit'), $uri['path'] . '/edit'));
...
+      watchdog('contact', 'Menu %label has been added.', array('%label' => $menu->label()), WATCHDOG_NOTICE, l(t('Edit'), $uri['path'] . '/edit'));

Bogus watchdog category 'contact'.

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.php
@@ -0,0 +1,103 @@
+  public function delete(array $form, array &$form_state) {
+    $menu = $this->getEntity($form_state);
+    $form_state['redirect'] = 'admin/structure/menu/manage/' . $menu->id() . '/delete';
+  }

FYI: #736564: Add #redirect property for form #type 'submit' to eliminate various submit handlers might allow us to remove these entity form controller methods in the long run. Feedback is welcome over there. :)

+++ b/core/modules/menu/lib/Drupal/menu/MenuListController.php
@@ -0,0 +1,65 @@
+  public function getOperations(EntityInterface $menu) {

Why is there no Delete operation? In general, shouldn't we call into parent:: here and just add the additional operations?

+++ b/core/modules/menu/menu.module
@@ -186,13 +205,13 @@ function menu_enable() {
+    $link['link_path'] = 'admin/structure/menu/manage/' . $menu->id();

Created a follow-up issue to remove this code:
#1882218: Remove static menu link creation for menus in menu_enable() and elsewhere

+++ b/core/includes/install.incundefined
@@ -429,6 +429,10 @@ function drupal_install_system() {
+  // @todo PHP 5.3.9 changes the behavior of is_subclass_of() with respect to
+  //   autoloaders. To allow earlier versions to properly find system.module,
+  //   warm the module list cache.
+  module_list();

Yes, this @todo is completely bogus, but I'm not sure what's going on here, other than it fails on PHP 5.3.5 without this :(

Can you try to replace the added code in drupal_install_system() with the attached interdiff?

(Extracted from #1798732: Convert install_task, install_time and install_current_batch to use the state system)

Ooh, that looks promising.

I extensively debugged this some more. We need to do #1798732: Convert install_task, install_time and install_current_batch to use the state system first. This patch runs into the exact problem space as over there and which the existing patch fixes.

We don't want to get blocked on that issue, so attached patch applies a stop-gap fix. This patch is expected to come back green, and that's the only piece I've authored here, so I feel eligible to RTBC.

If we need to stopgap fix this why don't we go with 76 for the time being. We are removing one line of code with a couple of lines to get things back to green. Are there technical reasons why 76 is that bad for now? (except for the fact we don't know whats happening :p)

Here's #76 rerolled with a better comment.

OK, thanks, let's stop-gap-fix it that way then. It's totally broken either way. ;)

Awesome! :D :D Really happy to see this at RTBC! It certainly cleans up a lot of stuff.

Channeling catch, I'm pretty sure he'd want to see benchmarks on this, to know how much of a performance loss (or gain? ;)) this patch buys us. Since this only converts menus themselves, and not menu links, it's probably not too shabby.

I looked through the patch thoroughly, and could really only find minor concerns around naming of things (menu_menu_add/edit is weirdly named for a page call-back, I'm not sure "field_ui_instance" is more clear than "menu) and some coding standards stuff (there's at least once instance of "thing. thing" instead of "thing . thing") but they are not really worth holding up the patch, I don't think.

Incidentally, I also did a thorough UI review and apart from the light table styling in #64 and the fact that the edit page now has a delete button on it (yay!) the custom/system menu admin experience in D7 and D8 is identical.

I did some quick profiling and discovered no difference in function calls with the patch for anonymous users. Talking with Tim about it, we only saw a difference with admin or on cache clears.

I'd argue that this issue should not be held up on profiling results, although I'm happy to defer to catch if he feels otherwise.

Yesss! This also makes menu labels and descriptions translatable (once those relevant CMI issues land). Superb! Tagging for that.

Ok, great. Thanks for double-checking!

Committed and pushed to 8.x. Woohoo!

This'll needs a change notice.

Made a few minor changes but looks good to me. The part that I didn't quite get (haven't reviewed the code) is the second half of "All hook_menu_*() hooks now receive \Drupal\system\Plugin\Core\Entity\Menu $menu as parameter and just a hook_ENTITY_TYPE_*() hooks".

A lot of information in there applies to entities in general (use of id(), which is more or less also already mentioned in http://drupal.org/node/1400186, not sure how much should be duplicated. And a documentation page along the lines of "Working with Entities in 8.x" or Best Practices... wouldn't hurt I guess. We can do that once we're through with the conversions...

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -0,0 +1,103 @@
+    if ($menu->isNew()) {
+      // Add 'menu-' to the menu name to help avoid name-space conflicts.
+      $menu->set('id', 'menu-' . $menu->id());
+    }

this was causing me problems when adding language select to menus... so I changed it to #field_prefix instead.

#1945226-62: Add language selector on menus

follow-up-like #2019647: Use EntityListController for menus

Added follow-ups

Untagging. Please remove the "Needs change notification" tag when the change notice task is complete.

added to related