Follow-up to #2285413-13: [Meta] Standardize entity route names

Problem/Motivation

\Drupal\menu_link_content\Form\MenuLinkContentDeleteForm::getCancelUrl() point to menu ui route but dependency is wrong
Menu entity defined in system module but uses admin permission defined in menu_ui module

Proposed resolution

move permission definition to system module
add check to MenuLinkContentDeleteForm::getCancelUrl() to make sure menu_ui module enabled

Remaining tasks

commit

User interface changes

no

API changes

No, except permission administer menu moved to system module so BC is fine

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because route used without checking that mdoule menu_ui enabled
Issue priority Normal because most of sites will have menu_ui module enabled
Disruption Non-disruptive because system module is always enabled
CommentFileSizeAuthor
#11 2328883-menu_link_content-11.patch4.23 KBandypost
#11 interdiff.txt3.46 KBandypost
#11 2328883-menu_link_content-fail.patch2.22 KBandypost
#7 2328883-7.patch756 bytesandypost
#3 2328883-3.patch748 bytesandypost
#2 2328883-2.patch769 bytesandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue tags: +MenuSystemRevamp

This isn't really WSCCI on an abstract level.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Active » Needs review
Issue tags: -WSCCI +Needs tests
FileSize
2328883-2.patch769 bytes

No idea where to put a test, probably it makes sense to add a unittest

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
FileSize
2328883-3.patch748 bytes

else is not needed

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/modules/menu_link_content/src/Form/MenuLinkContentDeleteForm.php
@@ -60,7 +60,10 @@ public function getQuestion() {
+    }
+    return $this->entity->urlInfo();

Seems okay for now

andypost queued 3: 2328883-3.patch for re-testing.

Status: Needs review » Needs work

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

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
FileSize
2328883-7.patch756 bytes

do we really need tests for that?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

I kinda thing we should have an integration tests. Note: this will give you probably some exception at the end of the day.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost at Skilld commented

yep, trying to write a test discovered that the module also use "administer menu" permission that defined in menu_ui module
So it's not clear how to resolve this dependency

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Tag1 Consulting for Drupal Association commented

Well, the test could just do: menu_ui disabled, menu_link_content enabled, and go to '/admin/structure/menu/item/{menu_link_content}/delete' and try to delete it.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost at Skilld commented
Issue tags: -Needs tests
FileSize
2328883-menu_link_content-fail.patch2.22 KB
interdiff.txt3.46 KB
2328883-menu_link_content-11.patch4.23 KB

integration fail test + moved permission to system module as suggested at IRC

PS: let's see what broken

The last submitted patch, 11: 2328883-menu_link_content-fail.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Great, thank you!

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost at Skilld commented
Issue summary: View changes

Updated IS and added beta evaluation

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

@andypost nice find, nice fix and nice test.

Committed 897056b and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 897056b on 8.0.x 
    Issue #2328883 by andypost, dawehner: menu ui route used in menu link...

Status: Fixed » Closed (fixed)

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