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

jessebeach’s picture

Priority: Normal » Critical

Bumping this to critical. We can't remove the menu_router table unless we resolve this issue.

pwolanin’s picture

This 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.

catch’s picture

I 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.

Wim Leers’s picture

Alright, sounds like we have consensus. Let's get that patch created! Any takers?

moshe weitzman’s picture

Add tag

Crell’s picture

Issue tags: +WSCCI

My 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.)

Wim Leers’s picture

#6:

2) Moshe effulgentsia said we can define custom relationships. For e.g. "Delete" (which would probably map to delete-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.

Crell’s picture

If 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.

Wim Leers’s picture

#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?

pwolanin’s picture

@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.

Crell’s picture

I 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.

pwolanin’s picture

There 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?

Crell’s picture

pwolanin: 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.

dawehner’s picture

Issue tags: +MenuSystemRevamp

Adding tag.

Crell’s picture

The 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.

Wim Leers’s picture

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.

I'd like to challenge this.

  • entity creation: create-form
  • entity editing: edit-form
  • entity deletion: delete-form (doesn't exist yet, but I doubt this would be considered unreasonable by them)
  • entity revisions: version-history
  • entity translations: alternate [1]
  • entity comments: replies
  • entity permalink: 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.

Crell’s picture

Well, 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.)

tim.plunkett’s picture

catch’s picture

Issue tags: +Approved API change

Tagging so this is easy to find, although it's already critical.

Wim Leers’s picture

#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!)

Crell’s picture

That 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?

Wim Leers’s picture

Assigned: Unassigned » pwolanin

pwolanin 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 :)

pwolanin’s picture

Ok, 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.

pwolanin’s picture

I 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()

catch’s picture

plach’s picture

Coming 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:

  • Let entity types define their route paths or derive them from the "canonical" one, if they are missing, e.g:
    /node/{node} -> /node/{node}/translations/add
    /taxonomy/term/{taxonomy_term} -> /taxonomy/term/{taxonomy_term}/translations/add
    /entity_foo/{entity_foo}/subentity_bar/{subentity_bar} -> /entity_foo/{entity_foo}/subentity_bar/{subentity_bar}/translations/add
    

    and so on.

  • Register those.
  • Generate the actual URLs when building the UI.

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:

<translation-overview-path>/overview                                  (e.g. /node/{node}/translations/overview)
<translation-overview-path>/add/{source-language}/{target-language}   (e.g. /node/{node}/translations/add/en/it)
<translation-overview-path>/edit/{language}                           (e.g. /node/{node}/translations/edit/it)
<translation-overview-path>/delete/{language}                         (e.g. /node/{node}/translations/delete/it)

This way we can use EntityInterface::uri() to generate the actual paths.

However, given the following statements from @Crell

Being able to define Drupal-specific link relationships would be very good, but we do need to do it properly.
[...]
And can we *guarantee* that every link relationship appropriate for HAL make sense as a UI-exposed link in a browser drop down menu?

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:

/**
 * Defines the node entity class.
 *
 * @EntityType(
 *   links = {
 *     "canonical" = "node_page_view",
 *     "edit-form" = "node_page_edit",
 *     "version-history" = "node_page_revisions"
 *   },
 *   operations = [
 *     "update" = {
 *       "label" = @Translation("Edit"),
 *       "route" = "node_page_edit",
 *       "contextual" = TRUE
 *     },
 *     "content_translation_overview" = {
 *       "label" = @Translation("Translate"),
 *       "route" = "node_page_content_translation_overview",
 *       "contextual" = TRUE
 *     },
 *     "content_translation_add" = {
 *       "label" = @Translation("Add translation"),
 *       "route" = "node_page_content_translation_add",
 *       "contextual" = FALSE
 *     }
 *   ]
 * )
 */

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).

pwolanin’s picture

@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.

pwolanin’s picture

I 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.

plach’s picture

@pwolanin:

Sounds reasonable but foreseeing the implications of going that way is totally above my head :)
Maybe @Crell can weigh on this?

Xano’s picture

#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.

pwolanin’s picture

dawehner 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?

Crell’s picture

So with #2084463: Convert contextual links to a plugin system similar to local tasks/actions RTBC, should this issue be closed as a duplicate?

Wim Leers’s picture

Well, RTBC != committed, but it seems very unlikely that this approach will happen, so I'd say "yes".

Crell’s picture

Status: Active » Closed (duplicate)

Good enough for me. If that falls off for some reason we can come back here to regroup.