Updated: Comment #0

Problem/Motivation

After #2084197: Uninstalling menu module removes all custom menus current menu module goes more menu_ui module because implements only List and Form controllers for menu and menu link entities.

Proposed resolution

rename menu module to menu_ui module according view_ui module

Remaining tasks

wait for #2084197: Uninstalling menu module removes all custom menus

User interface changes

some strings

API changes

no

#2084197: Uninstalling menu module removes all custom menus
#1882552: Deprecate menu_list_system_menus() and menu_ui_get_menus()
#2030645: Expand Menu with methods

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

jibran’s picture

So what about menu link module. Will it be renamed to menu module?

tim.plunkett’s picture

No, because it doesn't provide the menu entity, system does. Unless we moved both menu and menu_link to menu...

amateescu’s picture

Unless we moved both menu and menu_link to menu...

That's what I proposed some months ago (in a IRC conversation, I think), so yes, I'm definitely in favor of doing that :)

dawehner’s picture

Status: Active » Needs review
FileSize
31.09 KB

What a horrible idea to try that conversion instead of getting to bed. Don't expect a lot but at least core installs.

Status: Needs review » Needs work
Issue tags: -VDC

The last submitted patch, menu_menu_ui-5.patch, failed testing.

tim.plunkett’s picture

Issue summary: View changes
Issue tags: +beta target, +rename module

Tagging.

tim.plunkett’s picture

Issue tags: +Needs reroll
webchick’s picture

I can't see a way that these and other module rename patches get us to beta or release faster. IMO this was an API freeze deadline type of patch that we missed, and therefore should be moved to D9. Not doing so yet though so we could discuss it if I'm wildly off-base.

webchick’s picture

Version: 8.x-dev » 9.x-dev

Talked about this with Alexpott. He says since the name was already like this in D6 and D7, we should not attempt to do this in D8.

sun’s picture

Version: 9.x-dev » 8.x-dev

Coming from #2207893: Convert menu tree building to a service.

It's a little weird to have

  1. a menu module (UI for administering menus)
  2. a menu_link module that provides no UI at all (right?)
  3. some left-over legacy functionality in between of both in system module.

In addition, #2199633: Menu Link module is marked as required, but should not be required

The only reason for why menu_link is a module appears to be that Drupal\Core components are not able to define/expose config/content entities at this point — or much rather, it's trivial to define an entity in a core component, but the component is not able to participate in the hook system right now, which means that e.g. configuration import/export callbacks/hooks are not triggered.

OTOH, @dawehner raised a good point in that menu + menu link (tree) functionality is not actually required for a Drupal site. For example, a lightweight Drupal site could simply output a static list of links in a theme template — no need for a heavy-weight menu link management. In light of that, keeping this functionality in a module might actually make sense.

However, from an end-user/UX, developer/DX, as well as themer/TX perspective, the current constellation of modules + code fragments definitely makes no sense at all.

The following would be much more sane:

  1. Rename menu.module into menu_ui.module.
  2. Rename menu_link.module into menu.module.
  3. Move all remaining menu related code fragments from system.module into menu.module.

That way, we have

  1. A clear API module for code-driven menus, menu trees, and menu links. → menu.module
  2. A clear UI module that allows to administer menus, menu trees, and menu links in the UI. → menu_ui.module
  3. No more super-weird code in system.module that tries to establish menus + menu links where no such things are.

Tentatively moving back to D8, as I hope that people agree that we can't really leave things in the current very weird state.

andypost’s picture

Issue tags: +Needs committer feedback

Proper tag.
There's nothing except menu link entity is bound to module names

adevms’s picture

Assigned: Unassigned » adevms
adevms’s picture

Assigned: adevms » Unassigned
dawehner’s picture

Status: Needs work » Needs review
FileSize
45.34 KB

Let's try it out.

Status: Needs review » Needs work

The last submitted patch, 15: menu_ui-2085243-15.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
53.99 KB
9.96 KB

Status: Needs review » Needs work

The last submitted patch, 17: menu_ui-2085243-17.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
58.54 KB
7.92 KB

This could be green.

Status: Needs review » Needs work

The last submitted patch, 19: menu_ui-2085243-19.patch, failed testing.

sun’s picture

Issue tags: -Needs reroll

Thanks for kick-starting this, @dawehner!

+++ b/core/modules/menu/menu_ui.routing.yml
@@ -1,4 +1,4 @@
     _form: 'Drupal\menu\MenuSettingsForm'

@@ -61,15 +61,15 @@ menu.menu_add:
+    _title_callback: '\Drupal_ui\menu\Controller\MenuController::menuTitle'

Most likely, the remaining test failures are caused by the oversights + typos in the routing file...? :)

dawehner’s picture

Status: Needs work » Needs review
FileSize
58.54 KB
545 bytes

Let's hope

Status: Needs review » Needs work

The last submitted patch, 22: menu_ui-2085243-22.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
62.46 KB
4.93 KB

Maybe now.

dawehner’s picture

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: menu_ui-2085243-24.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
63.35 KB

Reroll

Status: Needs review » Needs work

The last submitted patch, 28: menu_ui-2085243-28.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
63.63 KB
588 bytes

This should be it.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

back to RTBC.

tstoeckler’s picture

Title: Rename menu to menu_ui module » Split Menu module into Menu and Menu UI module

Looks great. Re-titling for the new approach.

andypost’s picture

Leaving rtbc for approval

dawehner’s picture

I don't see what need to change an the issue summary

rename menu module to menu_ui module according view_ui module

That is the solution. simple.

Change notice: have a look at previous comments; https://drupal.org/node/2216585

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 30: menu_ui-2085243-30.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
65 KB

RErole.

sun’s picture

Status: Needs review » Reviewed & tested by the community
dawehner’s picture

Title: Split Menu module into Menu and Menu UI module » Rename Menu module into Menu UI module

.

Landish’s picture

30: menu_ui-2085243-30.patch queued for re-testing.

The last submitted patch, 30: menu_ui-2085243-30.patch, failed testing.

dcrocks’s picture

Status: Reviewed & tested by the community » Needs work

Cloned current D8 and did 'git apply' of patch in #36. Apply was clean except for whitespace warnings and install went successfully. Added/modified/deleted menus and menu links with no problems. However when I went to admin/structure/menus/setting I got


Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "drupal\menu\menusettingsform". in Symfony\Component\DependencyInjection\Container->get() (line 303 of core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Container.php). 
dcrocks’s picture

Error located in core/modules/menu_ui/menu_ui.routing.yml

@@ -1,4 +1,4 @@
-menu.settings:
+menu_ui.settings:
   path: '/admin/structure/menu/settings'
   defaults:
     _form: 'Drupal\menu\MenuSettingsForm'

'menu' should be 'menu_ui'. Tested change and worked successfully.

dawehner’s picture

Status: Needs work » Needs review
FileSize
65.04 KB
474 bytes

@dcrocks
Thank you for pointing that out!

jibran’s picture

Status: Needs review » Reviewed & tested by the community

back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 43: menu_ui-2085243-43.patch, failed testing.

dcrocks’s picture

#2120871: Rename EntityListController to EntityListBuilder renamed MenuListController.php to MenuListBuilder.php

dcrocks’s picture

#2226903: Step 1: Move static menu links to yml files also adds a file to the menu module. There a lot of active issues in this area that will obviously be seriously impacted by the patch. Should this fix get in asap to avoid playing constant catch-up and allow all the other issues to get this sorted out, or should it wait a bit until the dust settles?

webchick’s picture

I still don't understand why we are doing this at all at this point. Did alexpott sign off?

sun’s picture

Does the reasoning in #11 help?

dawehner’s picture

I still don't understand why we are doing this at all at this point. Did alexpott sign off?

Everyone should be able to work on whatever they want, fine if it does not get in. But seriously I don't want permissions
on what I can/should work on.

dawehner’s picture

Status: Needs work » Needs review
FileSize
64.55 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 51: menu_ui-2085243-51.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
64.66 KB
579 bytes

There we go.

Status: Needs review » Needs work

The last submitted patch, 53: menu_ui-2085243-53.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
67.75 KB
3.23 KB

Fixed some failures.

Status: Needs review » Needs work

The last submitted patch, 55: menu_ui-2085243-55.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
68.47 KB

Reroll

Status: Needs review » Needs work

The last submitted patch, 57: menu_ui-2085243-57.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
67.2 KB

There we go.

vijaycs85’s picture

nice change. +1.

sun’s picture

  1. +++ b/core/modules/menu_ui/lib/Drupal/menu_ui/Tests/MenuTest.php
    --- /dev/null
    +++ b/core/modules/menu_ui/lib/Drupal/menu_ui/Tests/MenuTest.php.rej
    

    an .orig + .rej file slipped into this patch ;)

  2. +++ b/core/modules/menu_ui/menu_ui.module
    --- /dev/null
    +++ b/core/modules/menu_ui/menu_ui.module.rej
    

    one more :)

dawehner’s picture

FileSize
65.1 KB

No real idea what happened here.

dawehner’s picture

62: menu_ui-2085243-62.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 62: menu_ui-2085243-62.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +DX (Developer Experience)
FileSize
66.96 KB

Yet another reroll.

dawehner’s picture

FileSize
65.25 KB
0 bytes

This had the rej/orig files in there

dawehner’s picture

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

+204, -204

Everything looks good, and tests still pass.

Thanks @dawehner!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 07f63fe and pushed to 8.x. Thanks!

Since this is confusing people thinking that the menu module is necessary for menu's I think this is a good idea - it is conforming to the field/field_ui & views/views_ui pattern.

  • Commit 07f63fe on 8.x by alexpott:
    Issue #2085243 by dawehner: Rename Menu module into Menu UI module.
    

  • Commit 62ba47d on 8.x by alexpott:
    Revert "Issue #2085243 by dawehner: Rename Menu module into Menu UI...

  • Commit e562ca4 on 8.x by alexpott:
    Issue #2085243 by dawehner: Rename Menu module into Menu UI module.
    
alexpott’s picture

Had to revert because #1366020: Overhaul SearchQuery; make search redirects use GET query params for keywords sneaked in in the same commit... but did an immediate recommit.

jibran’s picture

Well @dawehner got double commit credit for such an awesome effort I am really happy with that.

dawehner’s picture

Thank you alex for letting this in, despite the resistance earlier!

Status: Fixed » Closed (fixed)

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

mradcliffe’s picture

Status: Closed (fixed) » Active
Issue tags: -Needs committer feedback +Needs change record
Related issues: +#2251879: Finish Rename Menu module into Menu UI module

I didn't find the change obvious when trying to search where functions disappeared/were changed to because I wasn't sure if menu_ui was the same or not. I thought perhaps that menu_get_menus was now in the Menu tree building or Menu entity in system module so I searched in the wrong direction.

Search for "menu_ui" in change records has no results: https://drupal.org/list-changes/published/drupal?keywords_description=me...

Seaching api.drupal.org for current menu_ui functions shows the following changes: https://api.drupal.org/api/drupal/8/search/menu_ui. So here's a draft of a change notice that reflects those API functions (non-hook/alter functions only).

Draft Change Record

Title: menu module renamed to menu_ui
Branch: 8.x
Version: 8.0-ALPHA11

The menu module has been renamed to menu_ui to avoid confusion for site builders. All menu module API functions have been renamed. Module developers should also consider using the <code>entity.manager service to find menu entities.

<h4>Similar topic</h4>
<ul>
  <li>Change record: <a href="https://drupal.org/node/2226481" title="Menu tree building is now a service">Menu tree building is now a service</a></li>
</ul>
<h4>Renamed non-hook functions</h4>
<table>
  <tr><th>Before</th><th>After</th></tr>
  <tr><td>menu_load</td><td>menu_ui_load</td></tr>
  <tr><td>menu_get_menus</td><td>menu_ui_get_menus</td></tr>
  <tr><td>menu_node_save</td><td>menu_ui_node_save</td></tr>
  <tr><td>_menu_ui_get_options</td><td>_menu_ui_get_options</td></tr>
  <tr><td>menu_parent_options</td><td>menu_ui_parent_options</td></tr>
  <tr><td>_menu_ui_parents_recurse</td><td>_menu_ui_parents_recurse</td></tr>
  <tr><td>_menu_ui_parent_depth_limit</td><td>_menu_ui_parent_depth_limit</td></tr>
</table>
mradcliffe’s picture

Status: Active » Closed (fixed)
Issue tags: -Needs change record

I missed the comment by dawehner above linking the change record :*(. It was not published and not searchable by default. dawehner published the change record, and I'll modify it from there.