Suggested commit message

Issue #2301319 by pwolanin, dawehner, Wim Leers, effulgentsia: MenuLinkNG part5: Remove dead code; and party!

Problem/Motivation

After #2301317: MenuLinkNG part4: Conversion goes in there will be some dead code left in core - this patch just removes it.

Proposed resolution

Remaining tasks

User interface changes

API changes

Original report by @effulgentsia

Final chunk after #2301317: MenuLinkNG part4: Conversion. This removes the old menu_link module and dead procedural functions from menu.inc.

CommentFileSizeAuthor
#31 increment.txt2.51 KBpwolanin
#31 2301319-31.patch131.24 KBpwolanin
#26 2301319-26.patch128.73 KBpwolanin
#25 2301319-25.patch398.34 KBpwolanin
#23 2301319-23.patch393.4 KBpwolanin
#21 2301319-21-part5-only-do-not-test.patch128.73 KBpwolanin
#21 increment.txt583 bytespwolanin
#21 2301319-21.patch392.82 KBpwolanin
#20 2301319-20.patch383.93 KBpwolanin
#19 2301319-19.patch409.67 KBpwolanin
#17 2301319-17.patch408.53 KBpwolanin
#16 2301319-16.patch408.25 KBpwolanin
#14 2301319-14-part5-only-do-not-test.patch128.8 KBpwolanin
#14 2301319-14.patch409.3 KBpwolanin
#13 2301319-13.patch403.98 KBpwolanin
#12 2301319-12.patch474.26 KBpwolanin
#10 2301319-10.patch403.97 KBpwolanin
#9 2301319-the-full-kahuna-9.patch628.81 KBpwolanin
#6 xhprof.zip288.89 KBcilefen
#5 2301319-the-full-kahuna-5.patch617.38 KBpwolanin
#3 2301319-part5-3-do-not-test.patch128.8 KBpwolanin
#3 2301319-3.patch615.32 KBpwolanin
#2 2301319-part5-2-do-not-test.patch128.8 KBpwolanin
#2 2301319-2.patch612.91 KBpwolanin
menu-2256521-remove-dead-code.patch133.26 KBeffulgentsia
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Parent issue: » #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module
pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Postponed » Needs review
FileSize
2301319-2.patch612.91 KB
2301319-part5-2-do-not-test.patch128.8 KB

Testing the full patch with removal after fixing 8.x conflicts

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
2301319-3.patch615.32 KB
2301319-part5-3-do-not-test.patch128.8 KB

Here's a new one including fixups at step 1

Status: Needs review » Needs work

The last submitted patch, 3: 2301319-3.patch, failed testing.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Needs review
FileSize
2301319-the-full-kahuna-5.patch617.38 KB

Here's the full patch

cilefen’s picture

cilefen CreditAttribution: cilefen commented
FileSize
xhprof.zip288.89 KB

The xhprof runs:

53c29adf192f7.localhost@_drupal8x_menu-bench.xhprof is a load of /menu-bench from https://github.com/pwolanin/menu_bench.git on HEAD.
53c2d8e0670a6.localhost@_drupal8x_menu-bench.xhprof is the same with last patch.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue summary: View changes
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Priority: Normal » Critical
pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
2301319-the-full-kahuna-9.patch628.81 KB

Here's the current full patch

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
2301319-10.patch403.97 KB

Current full patch after part1 is committed.

Status: Needs review » Needs work

The last submitted patch, 10: 2301319-10.patch, failed testing.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Needs review
FileSize
2301319-12.patch474.26 KB

Rebases cleanly, not sure why it doesn't apply

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
2301319-13.patch403.98 KB

This should be the full remaining patch since parts 1 and 2 are committed.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
2301319-14.patch409.3 KB
2301319-14-part5-only-do-not-test.patch128.8 KB

updated combined patch and part5 only

Status: Needs review » Needs work

The last submitted patch, 14: 2301319-14.patch, failed testing.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Needs review
Issue tags: +MenuSystemRevamp
FileSize
2301319-16.patch408.25 KB

with fixes from part4

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Issue summary: View changes
FileSize
2301319-17.patch408.53 KB

Updated full remaining patch

Status: Needs review » Needs work

The last submitted patch, 17: 2301319-17.patch, failed testing.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Needs review
FileSize
2301319-19.patch409.67 KB

re-rolled full patch

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
2301319-20.patch383.93 KB

new snapshot of the full patch

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
2301319-21.patch392.82 KB
increment.txt583 bytes
2301319-21-part5-only-do-not-test.patch128.73 KB

Updated patch with all changes from part4, and some small added cleanup

Status: Needs review » Needs work

The last submitted patch, 21: 2301319-21.patch, failed testing.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Needs review
FileSize
2301319-23.patch393.4 KB

With part4 system controller fixes

Status: Needs review » Needs work

The last submitted patch, 23: 2301319-23.patch, failed testing.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Needs review
FileSize
2301319-25.patch398.34 KB

adding in fixes from part4

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
2301319-26.patch128.73 KB

last part!

Final patch after #2301317: MenuLinkNG part4: Conversion

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs change record updates

Looks good! Just deletions of functions in menu.inc, and the old menu_link module, whose .info.yml was already deleted in part 4.

After this lands, we should go back to the part 4 change records, and ensure these menu.inc deletions are referenced in there:

-const MENU_MAX_DEPTH = 9;
-function menu_set_active_menu_names($menu_names = NULL) {
-function menu_get_active_menu_names() {
-function menu_link_get_preferred($path = NULL, $selected_menu = NULL) {
-function menu_link_rebuild_defaults() {
-function menu_load_links($menu_name) {
-function menu_delete_links($menu_name) {
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

Thanks @effulgentsia! I checked and these are covered in https://www.drupal.org/node/2302069 with updates I wrote yesterday:
const MENU_MAX_DEPTH = 9;
function menu_load_links($menu_name) {
function menu_delete_links($menu_name) {

Covered in previous CR https://www.drupal.org/node/2240003
function menu_link_get_preferred($path = NULL, $selected_menu = NULL)

No references I found:
function menu_set_active_menu_names($menu_names = NULL) {
function menu_get_active_menu_names() {
function menu_link_rebuild_defaults() {

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +beta blocker

Tagging the child issues retroactively.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work

MenuHierarchy still uses MENU_MAX_DEPTH.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Needs review
FileSize
2301319-31.patch131.24 KB
increment.txt2.51 KB

I think we can just remove that class - the menu tree storage should not be access by Views.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +VDC

We discussed this a bit in IRC. As nothing is using this handler in core and there is no test coverage for it, @damiankloip and I are okay with simply removing the handler, at least here. I've left a message asking @dawehner to make the call as to whether we should consider this a regression that needs a followup.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Status: Needs review » Reviewed & tested by the community

I also left a comment in #1805996-71: [META] Views in Drupal Core. Even if we need that plugin back, I think that should be a followup rather than hold up this patch, as this plugin is completely broken in HEAD as-is (there's no {menu_links} table to join to). Therefore, back to RTBC if green.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Repeat after me: yay!!! Thanks everyone.

  • Dries committed c77851c on 8.0.x 
    Issue #2301319 by pwolanin, effulgentsia: MenuLinkNG part5: Remove dead...

Status: Fixed » Closed (fixed)

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