Port the module to Drupal 8.

CommentFileSizeAuthor
#12 2848327-10.patch4.79 KBstefan.r
#10 2848327-10.patch4.79 KBwebflo
#7 2848327-6.patch62.41 KBwebflo
#6 2848327-6.interdiff.txt957 byteswebflo
#2 2848327-menu-link-weight-d8.patch63.27 KBtstoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler created an issue. See original summary.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Status: Active » Needs review
FileSize
2848327-menu-link-weight-d8.patch63.27 KB

Since a lot of the related code in core - in particular the form-related code - that this module interacts with is (almost) unchanged I've pretty much performed a straight port so the patch should be at least somewhat reviewable.

I couldn't quite get the test to pass, but posting here already since the module does seem to be working quite well. I will try to finish the test soon.

rutiolma’s picture

rutiolma
Location 🇵🇹
CreditAttribution: rutiolma commented
Status: Needs review » Needs work

Great!
I just noticed that selecting a "Parent item" doesn't update the "Menu link weight items" (and throws an error), which means that link weight is working only for the menu root. It's also impossible to save the node when selecting the parent, throwing an error.
On both cases the error is "Drupal\Component\Plugin\Exception\PluginNotFoundException: Plugin ID 'menu_link_content' was not found. in Drupal\Core\Menu\MenuLinkManager->getDefinition() (line 208 of /core/lib/Drupal/Core/Menu/MenuLinkManager.php)."

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented

Thanks for trying this out. I did test this locally and it did work for me, maybe I managed to break something afterwards. Will try to investigate soon. Thanks!

tijsdeboeck’s picture

tijsdeboeck
Location Antwerp 🇧🇪 🇪🇺 🌎
CreditAttribution: tijsdeboeck at Open Up Media commented

Great! Love this module!
We also started on a D8 port, but will give your version a try, and if we find anything, we'll add it.

webflo’s picture

webflo CreditAttribution: webflo at UEBERBIT GmbH commented
Status: Needs work » Needs review
FileSize
2848327-6.interdiff.txt957 bytes

Look great, here is a fix for the issue @rmarques mentioned in comment #3.

webflo’s picture

webflo CreditAttribution: webflo at UEBERBIT GmbH commented
FileSize
2848327-6.patch62.41 KB

And the full patch.

webflo’s picture

webflo CreditAttribution: webflo at UEBERBIT GmbH commented 
+++ b/menu_link_weight.module
@@ -21,37 +30,62 @@ define('MENU_LINK_WEIGHT_MIN_DELTA', -50);
+  // Only allow users with the "administer menu" permission or that the Menu
+  // Admin Per Menu has granted access to some menus.
+  if (!$is_admin && !$is_admin_per_menu) {
+
   }

Sorry, i changed the condition accidentally. I will fix it with the next patch. Currently, working on the tests.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Version: 7.x-1.2 » 8.x-1.x-dev

#7 is now 8.x-1.x-dev (without the accidentally removed return statement), as soon as @webflo finishes this we can tag a beta.

Ideally I'd also like to port #2784555: Simplify access checks and allow modules to override to 8.x

webflo’s picture

webflo CreditAttribution: webflo at UEBERBIT GmbH commented
FileSize
2848327-10.patch4.79 KB

Lets fix the test first. All tests should pass with this patch.

@stefan.r Could you enable Automatic testing? Thanks

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented

Interdiff in #6 looks perfect, totally makes sense.

Re #10:

  1. +++ b/tests/src/FunctionalJavascript/MenuLinkWeightJavascriptTest.php
@@ -172,10 +172,10 @@ class MenuLinkWeightJavascriptTest extends JavascriptTestBase {
-      'menu[menu_link_weight][' . $link1_id . '][weight]' => -50,
...
+      'menu[menu_link_weight][' . $link1_id . '][weight]' => '-50',

    Wow, that's amazing.

    I tried to get the test to pass myself, but couldn't figure out why the values weren't being submitted correctly.

    Thanks! 

  2. +++ b/tests/src/FunctionalJavascript/MenuLinkWeightJavascriptTest.php
@@ -286,6 +286,7 @@ class MenuLinkWeightJavascriptTest extends JavascriptTestBase {
+    $this->menuLinkManager->resetDefinitions();

    Interesting, I guess that is needed due to the static cache in the test process itself?

So looks great to me, will post an updated patch. I also added a Browser Test to test the no-JS fallback button thingy.

Thanks a lot @webflo!

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
FileSize
2848327-10.patch4.79 KB

reuploading to trigger tests

Status: Needs review » Needs work

The last submitted patch, 12: 2848327-10.patch, failed testing.

webflo’s picture

webflo CreditAttribution: webflo at UEBERBIT GmbH commented

Hmm, maybe the old info file causes a issue with d.o composer thingy?

@stefan.r You could delete menu_link_weight.info, menu_link_weight.install.orig, menu_link_weight.install.rej, menu_link_weight.module.orig from the repo?

  • stefan.r committed 2f5fdaa on 8.x-1.x 
    Issue #2848327 by webflo, tstoeckler, stefan.r, tijsdeboeck, rmarques:...
stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Status: Needs work » Fixed

Tagged a beta2 release:

https://www.drupal.org/project/menu_link_weight/releases/8.x-1.0-beta2

Status: Fixed » Closed (fixed)

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