Add route parameters property to menu links

Yes, we should use e.g. href only for an external link and maybe store a route name + params also.

We already store the route names (yes, the one you want to remove in #2021779-10: Decouple shortcuts from menu links :) ). I'm wondering how will this impact the ability to create hierarchies.

It is, in theory and practice :) But when you don't specify a parent, it will try to find it based on the path.

@amateescu - I think if we create the now hook_menu_links, we should make you specify the parent using a machine name of some sort.

That would remove a lot of complexity from our current code, so I'm definitely +1 on having to specify the parent.

Wih that being said, I have another proposal. How about doing this "route name + parameters" work in the link.module (the link field doesn't support internal paths atm) and then just switch menu links to use it? This was also one my original goals in the menu link entity conversion, to be able to leverage the field system and drop a lot of custom code.

Edit: The thing is.. if we don't do this alternative approach, contrib would have to do it anyway, and it will probably just copy the code from menu links.

Ok, interesting - yes that sounds like something we should consider - but the problem with this format it that you can interchange it with actual URLs.

Crossposting the issue about adding internal paths support to core's link.module: #2054011: Implement built-in support for internal URLs

I think we need some more fundamental reworking of this to start getting rid of path-based data except for parsing user input? Maybe also move to hook_menu_link()

Also - I thought this was using the link field type?

+++ b/core/modules/menu_link/menu_link.install
@@ -202,6 +202,13 @@ function menu_link_schema() {
+        'serialize' => TRUE,

Where is this unserialized? Either I'm not seeing it, or we're missing tests that we're not actually using this new property.

Yes, this is essential to have - storing a route name doesn't make sense if you don't have parameters for a route with path slugs

1. +++ b/core/modules/menu_link/lib/Drupal/menu_link/Plugin/Core/Entity/MenuLink.php
   @@ -485,7 +492,13 @@ public function preSave(EntityStorageControllerInterface $storage_controller) {
   +      if ($result = $this::findRouteNameParameters($this->link_path)) {
   

   I realise this is mostly just moved code, but shouldn't it be static::findRouteNameParameters() instead of $this::?

2. +++ b/core/modules/system/system.install
   @@ -2242,6 +2242,21 @@ function system_update_8059() {
   +function system_update_8060() {
   

   No upgrade path for existing menu links?

@catch - what's wrong with the update function?

+  db_add_field('menu_links', 'route_parameters', $spec);

@catch, afaik, we don't support HEAD2HEAD updates yet.

ugh - what is that method even doing on the entity class?

At this point we should make it protected and not expose it at least - there is way too much dependence on the old router system in this entity.

We should also add a field for a machine name - I'm not sure why we even have the uuid?

something more like this as a basis for moving forward?

@amateescu I mean the Drupal 7 to Drupal 8 upgrade path. At the moment it looks like this will be empty until the menu link gets resaved, then it might get populated. Maybe that is the upgrade path but there's no mention of it either in the issue or the patch.

hmm, it's unclear to me that we need to save the machine name to the DB, so maybe that was premature. Still, I don't even see how it's currently possible to build a link based on a route name and parameters, which should be the most natural code path in the final version?

ok, maybe adding the machine name here is overkill - but I still think we should be hiding the lookup code - ideally we won't need it at all in the long-run except when processing user-entered paths

ok, well there is a ton of cleanup and follow-up work to do for menu links, but we can't even start without the parameters.

So, let's go back to the patch in #25. This is the same modulo some minor doxygen fixes.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkInterface.php
@@ -56,13 +56,14 @@ public function reset();
+   *   attay if no route was matched.

What's an attay? :-)

attay == symptom of my bad typing skills...

We really need to get this in a basis for rendering route-based links

#34: menu_link_parameters-2051889-34.patch queued for re-testing.

#34: menu_link_parameters-2051889-34.patch queued for re-testing.

#34: menu_link_parameters-2051889-34.patch queued for re-testing.

Committed 28f1ad6 and pushed to 8.x. Thanks!

We need to ensure https://drupal.org/node/1914008 is up-to-date.

Looks fine - will have some more changes that need doc before release in any case