Hi, are there any plans for Menu position porting to D8?

CommentFileSizeAuthor
#34 menu_position_d8_port-2215515-33.patch138.85 KBBarisW
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JohnAlbin’s picture

I'd love some help, but http://drupal.org/project/menu_block needs to be ported to D8 first.

netsensei’s picture

Hi John,

I've been looking at this over the past days. It's a challenge to wrap my head around this, but I like that. :-) I've been digging a bit in D8 code.

In a nutshell: I think the basic logic of re-setting the active trail somewhere in the request lifetime based on sets of conditional rules, as MP does in D7, also applies to D8.

The hurdle I'm looking at right now: hook_page_delivery_callback_alter() is where the D7 magic happens, but does not exist in D8. So we need to take a look at a different strategy to somehow alter the active trail. With D8 being symfony based, my guess was that this should have become a lot easier to do.

At this point, I've been toying a bit with swapping D8 core services to get around the problem:

menu.active_trail seems looks like a good spot to start: extend the class and override the getActiveLink() method. The comment in the body even hints at doing this if you want more control over what happens there.

The D7 logic 'fakes' a different request path by altering menu preferred link and such to control the active menu item that sets the active trail. In D8, this has become a bit harder to control. The $this->routeMatch object in the class, as part of the new Routing API, is a bit unclear to me. It contains an implementation of RouteMatchInterface (CurrentRouteMatch) but it could also contain a custom implementation of that interface. That object wraps around the current active request and uses that to fetch the active route used to set the active menu item.

So, I'm now left pondering:
1/ Should I create a 'fake' request of sorts and pass that to $this->routematch object. Knowing that this might break other stuff further down the line?
2/ Should I just do away way the existing $this->routematch object in the getActiveLink() method and go with my own implementation? This would contain the altering to my implementation of MenuActiveTrailInterface instead of opening a can of worms.

The other part I don't fully understand after a day and a few evenings of tinkering: how does this affect the Menu Block D8 port?

rootwork’s picture

Curious if there's been any progress on this. (I'm also not clear how menu_block would play into this, since it seems like its functionality is mostly already integrated into D8.)

@netsensei if you've been working on anything can you post it here or link to a repo somewhere? It'd be great to start testing something.

Lukas von Blarer’s picture

I think that I'll be working on this in Barcelona. If anyone is interested to join me... Just drop me a line.

morenstrat’s picture

Maybe this blog post can help to get a starting point?

sachbearbeiter’s picture

Title: D8 port » Menu position: D8 port
star-szr’s picture

Not sure if anyone else is working on this at the moment but this will likely be what we work on at this sprint: http://www.meetup.com/londug/events/228399619/

Will post updates here.

star-szr’s picture

I've ported two other D8 modules but both were smaller and one I had commit access to.

Some ideas on handling this, or maybe a combination of some of these ideas (and other ideas are welcome of course):

  • 1 giant patch on this issue
  • Sandbox/GitHub repo/similar
  • Separate issues with patches for porting different parts
  • Giving me commit access to start working on an 8.x branch ;)
akalata’s picture

I think separate issues works well, if we have somebody around with commit privileges. :)

If we don't hear back from a maintainer before the Global Sprint Weekend (I'll be happy to join you @cottser!), then Github will probably be the way to go.

BarisW’s picture

I'm happy to commit patches if they are RTBC'ed. If you can get them to RTBC as well at the sprint weekend, I'll commit them ASAP.

BarisW’s picture

Just added a 8.x-1.x branch for you to work on.

star-szr’s picture

@BarisW perfect thanks! That sounds good :)

star-szr’s picture

We are just getting started but a repo has been set up here: https://github.com/LonDUG/menu_position

We will make patches and issues as we go.

star-szr’s picture

So that was fun, and exhausting. I've sprinted probably a few dozen or so times on core but sprinting on contrib (particularly a module port) is a very different beast.

We did create a few issues but most of the work is half-done on branches on the GitHub repo. We didn't get super far in one day but I can at least sketch out the architecture we came up with. Can't guarantee if/when this group will be able to work on the porting again.

Without further ado (feedback/suggestions/ideas definitely welcome, a lot of these APIs I'm not as familiar with):

  • Menu position rules are configuration entities (config_entity GitHub branch has a WIP).
  • Plugins are D8 plugins, with perhaps three methods: condition, configurationForm, submitForm (or something like that, better names welcome). Basically the same as D7 but with annotations and classes and all that kind of stuff. We didn't get as far as figuring out how the JS stuff with plugins would work for the vertical tab summaries but that was not high on our list of priorities.
  • The actual mechanism of modifying the active trail we landed on was swapping out the menu.active_trail service (we called this the guts). See serviceAlter GitHub branch.

    Note: Unfortunately this would also mean only one such active trail manipulation module (see also https://www.drupal.org/project/menu_trail_by_path and others) could be active at one time - I think there's a need in core or contrib for a "chained" menu active trail service that would look for tagged, weighted active trail services so you could handle more complex use cases and/or add your own active trail classes for special cases.

    There's also the matter of breadcrumbs which we didn't really get to but theoretically it would work similar but with a breadcrumb builder (of which there can be multiple)

    • Inside the "guts", gather all the rules (config entities) by weight. From there call into each plugins' condition method, passing in any/all relevant information from the config entities. If the condition method on the plugin returns TRUE then the rule matches, same idea as the D7 plugins. More or less the same logic as menu_position_evaluate_rules().

Some other things we somewhat figured out:

  • The theme function can likely be removed in favour of using #type table inside the form directly (there is a WIP of this with dummy data on the GitHub repo - forms branch).
  • We also had someone working on updating the form_alter() code, that branch hasn't been pushed to GitHub yet.
  • The panels integration code can likely be removed for now.
lbainbridge’s picture

Alright so @porchlight and I had some time while we were at DrupalCon to sprint on this and I think we made some really good progress. Everything we did get done is pushed to the 8.x-1.x branch on the repo @Cottser mentioned above (https://github.com/LonDUG/menu_position). The main thing we focused on was getting the config entity in place and finishing the forms for managing menu position rules.

I am going to create some issues for remaining work on the github repository, but much of what we have done is almost a total re-write, as the new concepts in D8 kind of changed the way we had to think about some of the items, things we finished/made progress on were:

  • Configuration entity (complete)
  • Configuration forms (partially complete)

Outstanding work:

  • Service alter (the guts)
  • Plugins

We really tried to lay down the tracks for further work on this, take a look, create some issues if we've done something wrong (because we probably have) and let us know how we're doing.

MarcKwee’s picture

Hi @lbainbridge & @Cottser,

I am working on your port of Drupal 8 Menu position. We require it for our first big Drupal 8 Project so I intend to finish it in the coming 2 weeks. Currently creating the plugin system. Would it be possible to gain Git access so I can also push it to a feature Branch?.

Cheers, keep up the outstanding work!

star-szr’s picture

@MarcKwee same username on GitHub?

lbainbridge’s picture

@MarcKwee awesome! @porchlight and have been working on integrating the condition type plugins that the block module uses instead of writing a completely new plugin type for this module to use, not sure if that will pan out or not yet though. Feel free to fork the repo and send PRs our way as well if you'd like as well.

lbainbridge’s picture

Alright, the module is almost useable, we could use some help in the issue queue though, anyone with some time to install it and provide some bug reports in the issue queue on github that would be greatly appreciated, or better yet fork the repo and provide some PRs. The main things outstanding are some caching issues, admin ui improvements and additional plugin definitions.

We still have a ways to go for documentation, testing and code style as well, so feedback is definitely welcome, as this is pretty much a ground-up re-write of the module for D8.

Lukas von Blarer’s picture

Great, I will do so in the next days. Thank you!

Lukas von Blarer’s picture

ckaotik’s picture

I've also created an issue #2748341: MenuPositionLink is not compatible with MenuLinkContent. So far, other than these two issues, everything seems fine. Keep it up!

lbainbridge’s picture

Thanks for creating those issues everyone! It's awesome to really see this starting to come together now.

We are starting to think about how we can get this over to drupal.org now. I think there are two ways we could go about this:

1. We could create a massive patch and post it in this issue for a maintainer to apply to the 8.x-1.x branch.
2. I would be happy to help maintain the D8 branch of this module, so if someone would like to make me a maintainer I could push up all the commits and not lose the commit history.

Let me know how I can help get this over, we're really excited about the work here.

BarisW’s picture

Hi Luke,

I'd be happy to add you as co-maintainer, but I don't have permissions to do that.
Only John Albin can do that. So you could ask him, or I'll commit the huge patch.

j-vee’s picture

Hi there!

Thanks everyone for your great work porting this module to D8.

I've been using it while developing my new D8 website and it's working nicely for the most part.

I do get errors though that require me to uninstall the whole module and re-install it and then create all my menu position rules again.

This is what is breaking the module for me:
1. Create a menu item
2. Create a menu item position rule that is attached to the menu item from step 1
3. Manually create a new menu item and attach it under the same menu item, the one created in step 1

When saving the menu item from step 3 I get this error:

Notice: Undefined index: menu_name in Drupal\Core\Menu\MenuTreeStorage->findParent() (line 576 of /PATHTOROOT/core/lib/Drupal/Core/Menu/MenuTreeStorage.php).

And after that straight away I get this warning in my log messages:

Warning: asort() expects parameter 1 to be array, null given in Drupal\Core\Menu\MenuTreeStorage->preSave() (line 369 of /PATHTOROOT/core/lib/Drupal/Core/Menu/MenuTreeStorage.php).

And the menu position rule created in Step 2 disappears from the menu item list.

Now trying to access the Menu Position Rule (/admin/structure/menu-position) settings at this point gives me this error:

Drupal\Component\Plugin\Exception\PluginNotFoundException: Plugin ID '' was not found. in Drupal\Core\Menu\MenuLinkManager->getDefinition() (line 208 of /PATHTOROOT/core/lib/Drupal/Core/Menu/MenuLinkManager.php).

Cheers!

EDIT 1:

Now after uninstalling the module, I get the following error trying to access my main menu link items. It seems like the module doesn't get uninstalled cleanly:

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "menu_position_rule" entity type does not exist. in Drupal\Core\Entity\EntityTypeManager->getDefinition() (line 125 of core/lib/Drupal/Core/Entity/EntityTypeManager.php).
Drupal\Core\Entity\EntityTypeManager->getHandler('menu_position_rule', 'storage') (Line: 161)
Drupal\Core\Entity\EntityTypeManager->getStorage('menu_position_rule') (Line: 62)
Drupal\Core\Entity\EntityManager->getStorage('menu_position_rule') (Line: 113)
Drupal\menu_position\Plugin\Menu\MenuPositionLink->getEditRoute() (Line: 389)
Drupal\menu_ui\MenuForm->buildOverviewTreeForm(Array, 50) (Line: 426)
Drupal\menu_ui\MenuForm->buildOverviewTreeForm(Array, 50) (Line: 279)
Drupal\menu_ui\MenuForm->buildOverviewForm(Array, Object) (Line: 146)
Drupal\menu_ui\MenuForm->form(Array, Object) (Line: 115)
Drupal\Core\Entity\EntityForm->buildForm(Array, Object)
call_user_func_array(Array, Array) (Line: 512)
Drupal\Core\Form\FormBuilder->retrieveForm('menu_edit_form', Object) (Line: 271)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 74)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 574)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array) (Line: 139)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 62)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 98)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 77)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 628)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

EDIT 2:
Fixed the error in EDIT 1 by deleting all menu_position related database entries from the table menu_tree.
Here's an example entry that was left there even after uninstalling the menu_position_link module:

# menu_name, mlid, id, parent, route_name, route_param_key, route_parameters, url, title, description, class, options, provider, enabled, discovered, expanded, weight, metadata, has_children, depth, p1, p2, p3, p4, p5, p6, p7, p8, p9, form_class
'', '355', 'menu_position_link:notice_board_post', '', NULL, '', ?, '', ?, ?, 'Drupal\\menu_position\\Plugin\\Menu\\MenuPositionLink', ?, 'system', '1', '0', '0', '-48', ?, '0', '1', '355', '0', '0', '0', '0', '0', '0', '0', '0', 'Drupal\\Core\\Menu\\Form\\MenuLinkDefaultForm'
akalata’s picture

Hi j-vee: You should check the issue queue for this, and if your issue isn't listed, add it there (or if it is listed, add this detailed info!). This is more of a meta/status issue.

akalata’s picture

#27 being said, what's the status of github vs. d.o? I'd like to work on this, but I'm not sure where to start since there are issues in both places.

star-szr’s picture

Hey @akalata I think the most up to date work is still on GitHub since I don't think anyone who has worked on the port has commit access on d.o quite yet.

JohnAlbin’s picture

So Luke and Baris are both maintainers now (with "add new maintainers" perms). Go! Go! Go! :-)

Thanks for all the hard work already done on this, everyone!

When it gets merged into the d.o. repository, make sure everyone's git commits match up with their drupal.org usernames, if you can, please.

lbainbridge’s picture

Just realized I was made a maintainer earlier this week, I will work on getting the work from github on d.o so that we can proceed with work in a single spot :).

Lukas von Blarer’s picture

It would be great to have the code on d.o. Could you push the status of the github repository here?

  • BarisW committed d57af99 on 8.x-1.x
    Issue #2215515 by Cottser, lbainbridge, Lukas von Blarer, akalata,...
BarisW’s picture

Sure, just did!

Lukas von Blarer’s picture

Great, thank you! Would be great if someone could go through the issues that need review, especially this one: #2745295: Drupal\Component\Plugin\Exception\PluginNotFoundException: The "entity_bundle" plugin does not exist.

ckaotik’s picture

That's awesome! How are your plans regarding the issues and pull requests on GitHub? Move them over to drupal.org? Should be doable considering it's only a few issues/PRs and the proposed patches are easily exported by appending .patch to the PR's url.

rutiolma’s picture

Is there any major task preventing an alpha release? What can be done to help?

BarisW’s picture

I just committed #2745295: Drupal\Component\Plugin\Exception\PluginNotFoundException: The "entity_bundle" plugin does not exist.. If someone can work on #2808679: Importing config not working and provide a patch, I think we are ready for a first release.

BarisW’s picture

A D8 release is blocked by issue #2808679: Importing config not working.
We are so close!

Lukas von Blarer’s picture

What is the status for a first release?

Lukas von Blarer’s picture

It would be great if we got #2760373: Provide an event to alter the rules in as well.

Olafski’s picture

The blocker Importing config not working mentioned in #39 was fixed a while ago.

edit: URL corrected

Bwolf’s picture

It would be great to know when there will be a stable D8 release I have been using the D7 version for years.

This is a great module and I appreciate the work the maintainers have put into the project!

Cheers!

iancawthorne’s picture

Does context_active_trail do everything that menu_position did as a Drupal 8 alternative? I've been using it on a few projects due to menu_position not having a release and it seems to cover all scenarios.
https://www.drupal.org/project/context_active_trail

ckaotik’s picture

Does context_active_trail do everything that menu_position did

Thanks for mentioning this module! From a first look, I'd say they have a lot in common. However, menu_position can actually insert the current page into the menu tree, so it shows up in menu blocks. Also, the way context_active_trail uses a different approach by using the context module, which not everyone might need.

  • Menu tree: menu_position
  • Menu trail: both
  • Breadcrumbs: context_active_trail - yes, menu_position - uses system default (compatible with other breadcrumb modules)
  • Dependencies: context_active_trail - context, menu_position - none

So yes, they have a lot in common, but they target different use cases.

Mohammed J. Razem’s picture

So the alpha version for D8 released on 3 January 2019.

I'll go ahead and mark this as fixed.

Mohammed J. Razem’s picture

Status: Active » Fixed

Status: Fixed » Closed (fixed)

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