Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Currently menu links are basically point to a certain path, but in a world, in which drupal could alter its own internal paths,
we should rather store the route names and the parameters which together can build a link/url using the url generator.
API Changes
- Add a new database column to {menu_links} store route parameters
- Add handing for loading this new data
- Adds handling for parsing and saving the parameters derived from the path when saving a menu link
Comment | File | Size | Author |
---|---|---|---|
#34 | menu_link_parameters-2051889-34.patch | 10.26 KB | tstoeckler |
#34 | interdiff-32-34.txt | 650 bytes | tstoeckler |
#32 | menu_link_parameters-2051889-32.patch | 9.77 KB | pwolanin |
#32 | 2051889-25-32.increment.txt | 941 bytes | pwolanin |
#28 | 2051889-28.patch | 9.83 KB | pwolanin |
Comments
Comment #1
dawehnerAdd tag.
Comment #2
pwolanin CreditAttribution: pwolanin commentedYes, we should use e.g. href only for an external link and maybe store a route name + params also.
Comment #3
amateescu CreditAttribution: amateescu commentedWe 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.
Comment #4
dawehnerIsn't the menu hierarchy (at least in theory) based upon menu link IDs instead of paths?
Comment #5
amateescu CreditAttribution: amateescu commentedIt is, in theory and practice :) But when you don't specify a parent, it will try to find it based on the path.
Comment #6
pwolanin CreditAttribution: pwolanin commented@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.
Comment #7
amateescu CreditAttribution: amateescu commentedThat 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.
Comment #8
pwolanin CreditAttribution: pwolanin commentedOk, interesting - yes that sounds like something we should consider - but the problem with this format it that you can interchange it with actual URLs.
Comment #9
amateescu CreditAttribution: amateescu commentedCrossposting the issue about adding internal paths support to core's link.module: #2054011: Implement built-in support for internal URLs
Comment #10
dawehnerI am wondering whether using a different implementation on the ground would force it to be an API change?
Comment #11
dawehnerSo here is a patch which automatically adds the route parameters to the menu_links table.
This information is not used to provide menu links due to multiple reasons:
Comment #12
pwolanin CreditAttribution: pwolanin commentedI 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?
Comment #14
dawehnerThat is for sure right, but we simply can't do that in one issue, especially we kind of have to wait until the EntityNG conversion is done. What about getting this patch in, then have one issue which introduces some functionality to get menu links rendered via the url generator (the link generator patch would really help in that regard).
We can't remove functionality yet, as there are still classical non converted hook_menu entries.
Comment #15
dawehnerThis should fix most of the errors. Based upon my last comment I think this patch is big enough to make a small step forward.
Comment #16
dawehnerAdded some testcoverage.
Comment #17
amateescu CreditAttribution: amateescu commentedWhere is this unserialized? Either I'm not seeing it, or we're missing tests that we're not actually using this new property.
Comment #18
dawehnerGood catch! Ensured that by adding a test.
Comment #19
pwolanin CreditAttribution: pwolanin commentedYes, 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
Comment #20
catchI realise this is mostly just moved code, but shouldn't it be static::findRouteNameParameters() instead of $this::?
No upgrade path for existing menu links?
Comment #21
pwolanin CreditAttribution: pwolanin commented@catch - what's wrong with the update function?
Comment #22
amateescu CreditAttribution: amateescu commented@catch, afaik, we don't support HEAD2HEAD updates yet.
Comment #23
dawehnerRerolled and fixed point one of catch.
Opened a follow up for number 2: #2070191: Convert the url to route_name and route_parameters for existing menu links
Comment #25
dawehnerThe menu link test have to be adapted as well.
Comment #26
pwolanin CreditAttribution: pwolanin commentedugh - 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.
Comment #27
pwolanin CreditAttribution: pwolanin commentedWe should also add a field for a machine name - I'm not sure why we even have the uuid?
Comment #28
pwolanin CreditAttribution: pwolanin commentedsomething more like this as a basis for moving forward?
Comment #29
catch@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.
Comment #30
pwolanin CreditAttribution: pwolanin commentedhmm, 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?
Comment #31
pwolanin CreditAttribution: pwolanin commentedok, 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
Comment #32
pwolanin CreditAttribution: pwolanin commentedok, 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.
Comment #33
dawehnerThese small doxygen fixes are fine.
Comment #34
tstoecklerWhat's an attay? :-)
Comment #35
pwolanin CreditAttribution: pwolanin commentedattay == symptom of my bad typing skills...
We really need to get this in a basis for rendering route-based links
Comment #36
pwolanin CreditAttribution: pwolanin commented#34: menu_link_parameters-2051889-34.patch queued for re-testing.
Comment #38
pwolanin CreditAttribution: pwolanin commented#34: menu_link_parameters-2051889-34.patch queued for re-testing.
Comment #39
dawehnerBack to RTBC
Comment #40
pwolanin CreditAttribution: pwolanin commented#34: menu_link_parameters-2051889-34.patch queued for re-testing.
Comment #41
alexpottCommitted 28f1ad6 and pushed to 8.x. Thanks!
We need to ensure https://drupal.org/node/1914008 is up-to-date.
Comment #42
dawehnerChanged it: https://drupal.org/node/1914008/revisions/view/2704168/2825295
Comment #43
pwolanin CreditAttribution: pwolanin commentedLooks fine - will have some more changes that need doc before release in any case
Comment #44
BerdirComment #45.0
(not verified) CreditAttribution: commentedUpdated issue summary.