Files: 
CommentFileSizeAuthor
#85 menus-1814916-85.patch48.86 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,782 pass(es). View
#85 interdiff.txt707 bytestim.plunkett
#82 menu.config.82.patch49.56 KBsun
FAILED: [[SimpleTest]]: [MySQL] 50,643 pass(es), 6 fail(s), and 173 exception(s). View
#82 interdiff.txt686 bytessun
#80 1814916-interdiff-79.txt799 bytesandypost
#80 1814916-menu-cmi-79.patch48.89 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 50,669 pass(es), 3 fail(s), and 173 exception(s). View
#79 menus-1814916-79.patch48.89 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 50,669 pass(es), 3 fail(s), and 173 exception(s). View
#78 menu.entity.txt586 bytessun
#76 1814916-interdiff-76.txt730 bytesandypost
#76 1814916-menu-cmi-76.patch48.89 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 50,674 pass(es). View
#74 1814916-menu-cmi-73.patch48.56 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 50,703 pass(es), 1 fail(s), and 1 exception(s). View
#72 1814916-interdiff-72.txt2.86 KBandypost
#72 1814916-menu-cmi-72.patch48.64 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1814916-menu-cmi-72.patch. Unable to apply patch. See the log in the details link for more information. View
#68 menu-1814916-68.patch48.47 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,667 pass(es). View
#68 interdiff.txt1.58 KBtim.plunkett
#67 menu-list-67.jpg27.57 KBandypost
#67 menu-list-67-small.jpg15.89 KBandypost
#67 1814916-interdiff-67.txt1.99 KBandypost
#67 1814916-menu-cmi-67.patch48.32 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 50,668 pass(es). View
#64 before.png65.57 KBtim.plunkett
#64 after.png62.14 KBtim.plunkett
#63 1814916-interdiff-63.txt661 bytesandypost
#63 1814916-menu-cmi-63.patch48.02 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 50,542 pass(es). View
#63 menu-list.jpg31.82 KBandypost
#61 1814916-interdiff-61.txt18.58 KBandypost
#61 1814916-menu-cmi-61.patch48.02 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 50,516 pass(es). View
#60 1814916-interdiff-60.txt5.04 KBandypost
#60 1814916-menu-cmi-60.patch38.97 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 50,479 pass(es). View
#58 menus-1814916-58.patch38.38 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,477 pass(es). View
#56 menus-1814916-56.patch38.07 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 50,472 pass(es), 1 fail(s), and 1 exception(s). View
#53 menus-1814916-53.patch37.95 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,191 pass(es). View
#53 interdiff.txt409 bytestim.plunkett
#50 menus-1814916-50.patch37.88 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,376 pass(es). View
#48 menus-1814916-48.patch39.5 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 50,386 pass(es), 0 fail(s), and 52 exception(s). View
#48 interdiff.txt580 bytestim.plunkett
#42 interdiff.txt3.23 KBtim.plunkett
#42 menus-1814916-42.patch38.93 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 50,070 pass(es), 2 fail(s), and 1,564 exception(s). View
#40 interdiff.txt1.34 KBtim.plunkett
#40 menus-1814916-40.patch38.9 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 50,052 pass(es), 2 fail(s), and 1,473 exception(s). View
#39 interdiff.txt4.5 KBtim.plunkett
#39 menus-1814916-39.patch38.9 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 50,063 pass(es), 3 fail(s), and 1,468 exception(s). View
#37 1814916-interdiff-37.txt1.09 KBandypost
#37 1814916-menu-cmi-37.patch38.38 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 50,005 pass(es). View
#36 menus-1814916-36.patch38.48 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,056 pass(es). View
#36 interdiff.txt5.29 KBtim.plunkett
#33 1814916-interdiff-33.txt790 bytesandypost
#33 1814916-menu-cmi-33.patch37.34 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 50,032 pass(es). View
#31 1814916-menu-30.patch37.36 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/menu/menu.install. View
#28 1814916-menu-28.patch37.34 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1814916-menu-28.patch. Unable to apply patch. See the log in the details link for more information. View
#24 drupal-1826602-24.patch37.34 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 47,565 pass(es). View
#15 1814916-interdiff-15.txt1.53 KBandypost
#15 1814916-menu-15.patch37.4 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1814916-menu-15.patch. Unable to apply patch. See the log in the details link for more information. View
#14 menu-1814916-14.patch37.34 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 46,380 pass(es). View
#12 menu-1814916-12.patch37.02 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#12 interdiff.txt1.63 KBtim.plunkett
#9 1814916-interdiff-9.txt970 bytesandypost
#9 1814916-menu-9.patch37.08 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View
#7 1814916-interdiff-7.txt1.07 KBandypost
#7 1814916-menu-7.patch37.11 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 46,357 pass(es), 1 fail(s), and 0 exception(s). View
#5 1814916-menu-interdiff-fui.txt6.48 KBandypost
#5 1814916-menu-inderdiff-list.txt4.84 KBandypost
#5 1814916-menu-4.patch37.98 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 46,137 pass(es), 170 fail(s), and 100 exception(s). View
#1 1814916-menu-1.patch33.15 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 46,306 pass(es). View

Comments

andypost’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

Status: Active » Needs review
FileSize
33.15 KB
PASSED: [[SimpleTest]]: [MySQL] 46,306 pass(es). View

Initial patch:
- added entity
- fixed most of tests

needs clean-up
- hook execution
- edit form
- remove procedural wrappers
- list controller
- form controller

swentel’s picture

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

aspilicious’s picture

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

Fabianx’s picture

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

andypost’s picture

FileSize
37.98 KB
FAILED: [[SimpleTest]]: [MySQL] 46,137 pass(es), 170 fail(s), and 100 exception(s). View
4.84 KB
6.48 KB

@aspilicious the reason is because having menu as entity field_ui_menu_load() assumed as hook_ENTITY_TYPE_load()

New patch introduces List controller and changed suggested field_ui_instance_load()

Status: Needs review » Needs work

The last submitted patch, 1814916-menu-4.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
37.11 KB
FAILED: [[SimpleTest]]: [MySQL] 46,357 pass(es), 1 fail(s), and 0 exception(s). View
1.07 KB

Wrongly touched changes reverted

Status: Needs review » Needs work

The last submitted patch, 1814916-menu-7.patch, failed testing.

andypost’s picture

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

My bad, suppose now tests should pass

xjm’s picture

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

Status: Needs review » Needs work
Issue tags: +Configuration system, +Config novice, +Configurables

The last submitted patch, 1814916-menu-9.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.63 KB
37.02 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Rerolled.

Status: Needs review » Needs work

The last submitted patch, menu-1814916-12.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
37.34 KB
PASSED: [[SimpleTest]]: [MySQL] 46,380 pass(es). View

Fixed the use statements.

andypost’s picture

FileSize
37.4 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1814916-menu-15.patch. Unable to apply patch. See the log in the details link for more information. View
1.53 KB

@tim.plunkett thanx a lot for re-roll

patch removes menu_load_all() used only once in core

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

The last submitted patch, 1814916-menu-15.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review

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

EDIT passed locally

Status: Needs review » Needs work

The last submitted patch, 1814916-menu-15.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +Config novice, +Configurables

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

aspilicious’s picture

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.

andypost’s picture

@aspilicious fails was fixed with follow-up for EntityTypePlugins, suppose I hit re-test when head was broken because all failures was different and unrelated to menu

andypost’s picture

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

Status: Needs review » Needs work
Issue tags: +Configuration system, +Config novice, +Configurables

The last submitted patch, 1814916-menu-15.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
37.34 KB
PASSED: [[SimpleTest]]: [MySQL] 47,565 pass(es). View

Rerolled, no changes.

Fabianx’s picture

Status: Needs review » Needs work

This needs a re-roll.

Fabianx’s picture

Status: Needs work » Needs review

xpost :-D

andypost’s picture

+++ b/core/modules/menu/config/menu.menu.account.ymlundefined
@@ -0,0 +1,3 @@
+id: account
+label: User account menu

+++ b/core/modules/menu/config/menu.menu.admin.ymlundefined
@@ -0,0 +1,3 @@
+id: admin
+label: Administration

+++ b/core/modules/menu/config/menu.menu.main.ymlundefined
@@ -0,0 +1,3 @@
+id: main
+label: Main navigation

+++ b/core/modules/menu/config/menu.menu.tools.ymlundefined
@@ -0,0 +1,3 @@
+id: tools
+label: Tools

Suppose we have to move this files to system module. Menu module could be uninstalled but this system menus are needed.

+++ b/core/modules/menu/menu.moduleundefined
@@ -801,12 +830,12 @@ function menu_form_node_type_form_alter(&$form, $form_state) {
 function menu_get_menus($all = TRUE) {
-  if ($custom_menus = menu_load_all()) {
+  if ($custom_menus = entity_load_multiple('menu')) {
     if (!$all) {
       $custom_menus = array_diff_key($custom_menus, menu_list_system_menus());

System menus are hard-coded so probably we should not allow to change their machine names

andypost’s picture

FileSize
37.34 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1814916-menu-28.patch. Unable to apply patch. See the log in the details link for more information. View

Just a re-roll to follow head changes

andypost’s picture

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

Status: Needs review » Needs work
Issue tags: +Configuration system, +Config novice, +Configurables

The last submitted patch, 1814916-menu-28.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
37.36 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/menu/menu.install. View

Chasing head

Status: Needs review » Needs work

The last submitted patch, 1814916-menu-30.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

Status: Needs work » Needs review
FileSize
37.34 KB
PASSED: [[SimpleTest]]: [MySQL] 50,032 pass(es). View
790 bytes

Update functions numbers are changed

aspilicious’s picture

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

andypost’s picture

Suppose {menu_custom} table should be there according #1860986: Drop left-over tables from 8.x

schema definition should be removed.

The status of the issue - needs review #27

The Menu.module is optional but system menus and entity needs new place

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
5.29 KB
38.48 KB
PASSED: [[SimpleTest]]: [MySQL] 50,056 pass(es). View

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.

andypost’s picture

FileSize
38.38 KB
PASSED: [[SimpleTest]]: [MySQL] 50,005 pass(es). View
1.09 KB

It works! Patch with small fixes for comments

EDIT

  • Maybe better add list controller with hook_entity_info() when menu module enabled?
  • Used to setup minimal profile, menus is here because of system but no .yml files in config storage...
  • Suppose we should move menu.menu.*.yml fies to system/config too

Otherwise patch RTBC

aspilicious’s picture

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?

tim.plunkett’s picture

FileSize
38.9 KB
FAILED: [[SimpleTest]]: [MySQL] 50,063 pass(es), 3 fail(s), and 1,468 exception(s). View
4.5 KB

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.

tim.plunkett’s picture

FileSize
38.9 KB
FAILED: [[SimpleTest]]: [MySQL] 50,052 pass(es), 2 fail(s), and 1,473 exception(s). View
1.34 KB

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

tim.plunkett’s picture

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.

tim.plunkett’s picture

FileSize
38.93 KB
FAILED: [[SimpleTest]]: [MySQL] 50,070 pass(es), 2 fail(s), and 1,564 exception(s). View
3.23 KB

:(

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

The last submitted patch, menus-1814916-42.patch, failed testing.

parasite’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work
Issue tags: +Configuration system, +Config novice, +Configurables

The last submitted patch, menus-1814916-42.patch, failed testing.

tim.plunkett’s picture

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?

tim.plunkett’s picture

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
580 bytes
39.5 KB
FAILED: [[SimpleTest]]: [MySQL] 50,386 pass(es), 0 fail(s), and 52 exception(s). View

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

tim.plunkett’s picture

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.

tim.plunkett’s picture

FileSize
37.88 KB
PASSED: [[SimpleTest]]: [MySQL] 50,376 pass(es). View

#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

aspilicious’s picture

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

aspilicious’s picture

Besides of that this feels rtbc

tim.plunkett’s picture

FileSize
409 bytes
37.95 KB
PASSED: [[SimpleTest]]: [MySQL] 50,191 pass(es). View

Forgot the manifest file update.

sun’s picture

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

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Needs review » Needs work

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
38.07 KB
FAILED: [[SimpleTest]]: [MySQL] 50,472 pass(es), 1 fail(s), and 1 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, menus-1814916-56.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
38.38 KB
PASSED: [[SimpleTest]]: [MySQL] 50,477 pass(es). View

One more try

andypost’s picture

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

andypost’s picture

Assigned: tim.plunkett » Unassigned
FileSize
38.97 KB
PASSED: [[SimpleTest]]: [MySQL] 50,479 pass(es). View
5.04 KB

Fixed most of #54 and unused css

patch still needs upgrade tests, form controller and proper fix for module_list()

PS: Suppose we should just remove theme_menu_admin_overview()

EDIT: menu_install() was removed without fixes, seems this needs some testing.
Logic with menu_enable() needs some refactoring
menu_save and menu_delete() should be removed

andypost’s picture

FileSize
48.02 KB
PASSED: [[SimpleTest]]: [MySQL] 50,516 pass(es). View
18.58 KB

Patch introduces form controller.
menu_save() and menu_delete() are removed - suppose better to move 'em into storage controller.
Changes menu.api.php - probably we need to delete this file in follow-up.

sun’s picture

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. :)

andypost’s picture

Issue tags: +Needs usability review
FileSize
31.82 KB
48.02 KB
PASSED: [[SimpleTest]]: [MySQL] 50,542 pass(es). View
661 bytes

Fixed isNew() for delete button.

MenuStorageController is ok to separate issue, same for menu.api.php and probably for upgrade tests.

Except this I think this issue is RTBC.

Let's get usability review of removal theme_menu_admin_overview(), suppose in case of menu it looks more same to have description as separate column in overview table
menu-list.jpg

@tim.plunkett has assigned this issue, sorry I missed this...

tim.plunkett’s picture

FileSize
62.14 KB
65.57 KB

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

Before:
before.png

After:
after.png

Bojhan’s picture

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

yoroy’s picture

Status: Needs review » Needs work

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?

andypost’s picture

Status: Needs work » Needs review
FileSize
48.32 KB
PASSED: [[SimpleTest]]: [MySQL] 50,668 pass(es). View
1.99 KB
15.89 KB
27.57 KB

Added class menu-label with bold style

menu-list-67.jpg

... and marked description column hidden for mobile screens

menu-list-67-small.jpg

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs usability review
FileSize
1.58 KB
48.47 KB
PASSED: [[SimpleTest]]: [MySQL] 50,667 pass(es). View

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

sun’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs usability review

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

andypost’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs usability review

Yes, this kind of attach looks better :)

sun’s picture

Status: Reviewed & tested by the community » Needs work
andypost’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

Status: Needs work » Needs review
FileSize
48.64 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1814916-menu-cmi-72.patch. Unable to apply patch. See the log in the details link for more information. View
2.86 KB

Fixed watchdog category and getOperations() - added delete operation with restriction for system menus

module_list() - needs Tim's attention

menu_edit_menu_name_exists() - is not obsolete because id() is generated with prefix of menu owner "menu-", "shortcut-" etc

Filed follow-up for #54 #1882552: Get rid of menu_list_system_menus()

EDIT issue summary updated with follow-ups

Status: Needs review » Needs work

The last submitted patch, 1814916-menu-cmi-72.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
48.56 KB
FAILED: [[SimpleTest]]: [MySQL] 50,703 pass(es), 1 fail(s), and 1 exception(s). View

Merged HEAD

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuTest.phpundefined
@@ -168,10 +168,10 @@ function addCustomMenu() {
-    // Unlike most other modules, there is no confirmation message displayed.
-
+    // Verify that confirmation message displayed.
+    $this->assertRaw(t('Menu %label has been added.', array('%label' => $label)));

Added this assert for new message

+++ b/core/modules/menu/menu.moduleundefined
@@ -221,129 +233,69 @@ function menu_save($menu) {
-      // Make sure the menu is present in the active menus variable so that its
-      // items may appear in the menu active trail.
-      // See menu_set_active_menu_names().
-      $config = config('system.menu');
-
-      $active_menus = $config->get('active_menus_default') ?: array_keys(menu_get_menus());
-      if (!in_array($menu['menu_name'], $active_menus)) {
-        $active_menus[] = $menu['menu_name'];
-        $config->set('active_menus_default', $active_menus);
-      }

Patch also fixes the bug in menu_save() when config is not saved...

Status: Needs review » Needs work

The last submitted patch, 1814916-menu-cmi-73.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
48.89 KB
PASSED: [[SimpleTest]]: [MySQL] 50,674 pass(es). View
730 bytes

Another re-roll

tim.plunkett’s picture

+++ 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 :(

sun’s picture

FileSize
586 bytes

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)

tim.plunkett’s picture

FileSize
48.89 KB
FAILED: [[SimpleTest]]: [MySQL] 50,669 pass(es), 3 fail(s), and 173 exception(s). View

Ooh, that looks promising.

andypost’s picture

FileSize
48.89 KB
FAILED: [[SimpleTest]]: [MySQL] 50,669 pass(es), 3 fail(s), and 173 exception(s). View
799 bytes

Patch with #78 applied

Status: Needs review » Needs work

The last submitted patch, 1814916-menu-cmi-79.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
686 bytes
49.56 KB
FAILED: [[SimpleTest]]: [MySQL] 50,643 pass(es), 6 fail(s), and 173 exception(s). View

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, menu.config.82.patch, failed testing.

aspilicious’s picture

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)

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
707 bytes
48.86 KB
PASSED: [[SimpleTest]]: [MySQL] 50,782 pass(es). View

Here's #76 rerolled with a better comment.

sun’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

Status: Reviewed & tested by the community » Needs review

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.

webchick’s picture

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.

msonnabaum’s picture

Status: Needs review » Reviewed & tested by the community

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.

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-config

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

webchick’s picture

Title: Convert menus into entities » Change notice: Convert menus into entities
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Ok, great. Thanks for double-checking!

Committed and pushed to 8.x. Woohoo!

This'll needs a change notice.

andypost’s picture

Status: Active » Needs review

Filed a change notice http://drupal.org/node/1888504
Needs help to check grammar

Berdir’s picture

Title: Change notice: Convert menus into entities » Convert menus into entities
Priority: Critical » Normal
Status: Needs review » Fixed

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

beejeebus’s picture

Issue tags: +Needs profiling

Status: Fixed » Closed (fixed)

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

YesCT’s picture

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

YesCT’s picture

YesCT’s picture

Issue summary: View changes

Added follow-ups

xjm’s picture

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

xjm’s picture

Issue summary: View changes

added to related