Problem/Motivation
MENU_LOCAL_TASK
and MENU_DEFAULT_LOCAL_TASK
were removed by #2004334: Separate Tabs (MENU_LOCAL_TASK) from hook_menu(). This has left contextual links somewhat stranded on hook_menu
items.
One of the goals of converting hook_menu
items to routes is to eventually remove the menu_router
table. The context property of a menu item is stored in the menu_router
table. Therefore, we must move the definition of the context of a menu item before we can remove this table. This should be the last impediment to removing the menu_router
table.
The composition of contextual links in D7 is cumbersome and tied tightly to the rendering layer. The declaration of standard links available for an entity was introduced in #1970360: Entities should define URI templates and standard links. We can extend this pattern to reduce the cumbersomeness declaring contextual links as well.
Proposed resolution
We already define links for an entity as a keyed hash. We could define the contextual links in annotation as a sub-set of these links.
/**
* Defines the node entity class.
*
* @EntityType(
* links = {
* "canonical" = "/node/{node} ",
* "edit-form" = "/node/{node}/edit",
* "version-history" = "/node/{node}/revisions"
* },
* contextual_links = [
* "edit-form",
* "delete-form"
* ]
* )
*/
Each entity (e.g. nodes, blocks, views, menus, etc.) would define their own set of contextual links. These sets would then be aggregated (much as they are today) into the set of inks associated with a unit during rendering (e.g. a menu block that provides contextual links to configure the block and edit the menu). The current end-user experience would not change.
Much complexity in the aggregation of contextual links can be removed in this issue because we will not longer need to process contextual links in the rendering layer. We can do this well before rendering.
We may be able to extend this work to automatically generate routes from patterns defined for links on an entity in the same way that the REST module generates routes for HTTP methods against entities. The two systems are highly analogous. Such work need not be undertaken in this issue.
Remaining tasks
Propose an initial patch.
User interface changes
There shouldn't be any.
API changes
Contextual links will no longer be identified in hook_menu
as the value of the context
property on a task item.
Contextual links will be declared as annotations on entities.
Comments
Comment #1
jessebeach CreditAttribution: jessebeach commentedBumping this to critical. We can't remove the
menu_router
table unless we resolve this issue.Comment #2
pwolanin CreditAttribution: pwolanin commentedThis proposal is based on discussion among jessebeach , myself, Wim Leers, and effulgentsia in person after we stumbled on this while working on router conversion.
I hadn't remembered that this system was also using
{menu_router}
and in any case it's implemented in a really confusing way in D7.Comment #3
catchI knew this was using router access checks via loading the router item, but I didn't realise the actual contextual links definition depended on hook_menu() in the first place, that's not good.
Switching to making these based on entities sounds good - in practice they're only used for entities of various kinds anyway in 8.x.
Comment #4
Wim LeersAlright, sounds like we have consensus. Let's get that patch created! Any takers?
Comment #5
moshe weitzman CreditAttribution: moshe weitzman commentedAdd tag
Comment #6
Crell CreditAttribution: Crell commentedMy knee-jerk responses here is "ooh, shiny!" I don't fully grok all the implications, though. Some questions:
1) Does this mean only content entities can have contextual links? That may already be the case, if so, no problem, but I'm pretty sure Views have them now, for instance.
2) What do we do with contextual links that do not map to one of the IANA defined relationships? We still don't technically have support for custom relationships. We may need to add that for #2019123: Use the same canonical URI paths as for HTML routes anyway, but it's still a requirement we don't currently support.
3) I'm unclear on how getting contextual links from the annotation rather than menu_router addresses where they get rendered. Can you elaborate?
4) What's the implication for cacheability? IIRC contextual links are one of the things that makes non-per-user caching painful/impossible. How does this help with that issue? (This probably overlaps with point 3.)
Comment #7
Wim Leers#6:
2)
Mosheeffulgentsia said we can define custom relationships. For e.g. "Delete" (which would probably map todelete-form
) that's necessary.4) This does not help. It's the same problem as before: permissions are unpredictable (solely role-based permissions are predictable, but with the various permission altering and entity access mechanism, they become unpredictable) and hence they cannot be cached. Hence we need to do an AJAX request to retrieve the contextual links on a page. It's already working that way ever since #914382: Contextual links incompatible with render cache.
Comment #8
Crell CreditAttribution: Crell commentedIf we want to be good REST citizens, we can't just throw new link relationships around without namespacing them. They need to be namespaced, and it would be good DX to provide a curie for them.
Being able to define Drupal-specific link relationships would be very good, but we do need to do it properly.
Comment #9
Wim Leers#8: I'm not arguing against that at all. I was just explaining that we'd considered this. Is there an issue for that already?
Comment #10
pwolanin CreditAttribution: pwolanin commented@Crell - when you are rendering the entity it would seem a fine time to include contextual links?
Also, this isn't about REST is it? REST uses HTTP verbs and a single resource URL. As best as I could tell, the relationships are more for UI building?
The "Edit view" contextual link is apparently already added in a non-standard way (not using the men.inc code) due to the multitude of ways in which a view can be rendered.
Comment #11
Crell CreditAttribution: Crell commentedI guess I should put "REST citizens" in quotes. Defining links alongside IANA links requires namespacing them. That's proper good-citizen behavior for using hyperlink relationships. That's my point. We currently have no way to define and mark such relationships. If we want to go this route, we probably will have to. I'm totally cool with doing so; I'm just noting it as a thing.
Comment #12
pwolanin CreditAttribution: pwolanin commentedThere is nothing related to deletion or removal at http://www.iana.org/assignments/link-relations/link-relations.xml#link-r... so we need to propose or use a non-standard one in any case?
Comment #13
Crell CreditAttribution: Crell commentedpwolanin: That's exactly what I'm saying. :-) We need to use a non-standard link, which means we need a way to properly define non-standard links. Just making up new strings doesn't count as "proper" in this case; they need to be namespaced.
Comment #14
dawehnerAdding tag.
Comment #15
Crell CreditAttribution: Crell commentedThe REST team discussed this issue on our Thursday call. Our conclusion was that the links annotation should NOT be where contextual links defines its operations. Entity operations are a separate thing from link relationships. Some link relationships may also map to an entity operation for contextual links (edit-form, for instance), but not all of them, and many contextual links operations would have no relationship link.
Therefore, while it's fine to use annotations for entity operations (which is, really, what we're talking about here), it should be a separate key from links. Additional code to turn selective links into operations where it makes sense would be fine as long as the two are distinct lists in the annotation.
Comment #16
Wim LeersI'd like to challenge this.
create-form
edit-form
delete-form
(doesn't exist yet, but I doubt this would be considered unreasonable by them)version-history
alternate
[1]replies
canonical
This is a superset of the number of contextual link operations used in Drupal 8 core.
So, what I'm specifically challenging is the claim that "many contextual links operations would have no relationship link".
[1]: Designates substitute versions for the document in which the link occurs. When used together with the lang attribute, it implies a translated version of the document. When used together with the media attribute, it implies a version designed for a different medium (or media). See http://microformats.org/wiki/existing-rel-values.
Comment #17
Crell CreditAttribution: Crell commentedWell, let me rephrase a bit. Can we *guarantee* that every contextual link would make sense as a general link relationship, which would then be a link also included in various other autogenerated places (meta tags, HAL links in REST, etc.)? And can we *guarantee* that every link relationship appropriate for HAL make sense as a UI-exposed link in a browser drop down menu?
Unless we're certain it's a 1:1 list, then we shouldn't build them as a 1:1 list. I am not at all certain of that, even if there is potentially quite a bit of overlap. Eg, we don't define alternate/translation links in the links array, but that could totally make sense in a header tag or a REST link, but probably not in a contextual link.
(Incidentally, that site has way more link relationships in it than IANA. Right now we're only using IANA. Expanding to other such standard lists is something we should discuss separately, but I'm open to discussing it.)
Comment #18
tim.plunkettSeems partially inline with #1889790: [Meta] Allow modules to register links for menus, breadcrumbs and tabs (if not with hook_menu)
Comment #19
catchTagging so this is easy to find, although it's already critical.
Comment #20
Wim Leers#17: That's a good point! :) You've convinced me.
Before continuing in that direction, it seems we should look into #1889790: [Meta] Allow modules to register links for menus, breadcrumbs and tabs (if not with hook_menu) that tim.plunkett pointed to? (Thanks, Tim!)
Comment #21
Crell CreditAttribution: Crell commentedThat meta is mostly for tracking issues like this that are taking care of the remainder of hook_menu. :-)
So how can we move forward here?
Comment #22
Wim Leerspwolanin was very passionate about this issue, so maybe he can run with it? I don't have the bandwidth. Tentatively assigning to him to hopefully get feedback from him :)
Comment #23
pwolanin CreditAttribution: pwolanin commentedOk, let me take a stab at it - it's too bad we aren't close to making the route declarations live on the entities too.
Comment #24
pwolanin CreditAttribution: pwolanin commentedI dug into this a bit - if we start this conversion, it would make much more sense to use routes rather than paths, so I think this is blocked on these issues:
#2046737: Add a method to the AccessManager that only needs a route name and parameters
#2057155: Add a generateFromRoute() method on the url generator to accept options like url()
Comment #25
catchCross-posting #1810350: Remove menu_* entity keys and replace them with entity links.
Comment #26
plachComing from #1810350: Remove menu_* entity keys and replace them with entity links and looking for feedback (this may appear OT initially but I need to provide some context :)
Over there we are trying to get rid of the
menu_*
entity keys that the Content Translation module introduced almost one year ago to be able to generate a translation UI for any entity type in a generic fashion. Basically CT needs to:and so on.
In #1810350: Remove menu_* entity keys and replace them with entity links we are defining a new
'translation-overview'
entity link, which defaults to<canonical-path>/translations
, and then we are using it to derive the other paths and register the related routes:This way we can use
EntityInterface::uri()
to generate the actual paths.However, given the following statements from @Crell
it seems this solution is not ideal, AAMOF we are providing a non-namespaced relationship which probably does not make much sense from the HAL perspective. It would be much more suited as a plain entity operation (had we a way to define them).
That said, I am wondering whether it would make sense to change the initial proposal as follows:
This way we would decouple operations and relationship links, which would be occasionally tied by what they actually share, i.e. the route. Since we are moving towards using route names instead of paths/patterns, I think this approach would be consistent with #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit.
Additionally keying the operations that way would allow us to automate access checks, as we would just do
$node->access($operation_key)
.Comment #27
pwolanin CreditAttribution: pwolanin commented@plach - I like the direction this is heading, but I think there is a possible problem listing the routes for the links in that those are the HTML routes.
Possibly we can simply resolve this by making the assumption (and documenting it) that the link relation URIs will be generated from the HTML route path patterns, and hence any other format that wants to use these needs to match the path pattern of the HTML route?
In any case, the content-type based routing may not even be working yet, so it's currently no difference.
Comment #28
pwolanin CreditAttribution: pwolanin commentedI would also like to see us consider making contextual links be plugins like local tasks and we could create plugin derivatives automatically from these entity annotations to cover the entity use cases.
Comment #29
plach@pwolanin:
Sounds reasonable but foreseeing the implications of going that way is totally above my head :)
Maybe @Crell can weigh on this?
Comment #30
Xano#1839516: Introduce entity operation providers tries to unify the way operations are exposed to other APIs. However, it does not limit where the operations are initially declared. The default uses controller methods and hooks, but it can be extended to check annotations as well.
Comment #31
pwolanin CreditAttribution: pwolanin commenteddawehner has started on a plugin implementation at: #2084463: Convert contextual links to a plugin system similar to local tasks/actions
At this point it might just be a lot easier to define the links we need, rather than coupling them to the annotation?
Comment #32
Crell CreditAttribution: Crell commentedSo with #2084463: Convert contextual links to a plugin system similar to local tasks/actions RTBC, should this issue be closed as a duplicate?
Comment #33
Wim LeersWell, RTBC != committed, but it seems very unlikely that this approach will happen, so I'd say "yes".
Comment #34
Crell CreditAttribution: Crell commentedGood enough for me. If that falls off for some reason we can come back here to regroup.