Problem/Motivation

The major parts of #1781372: Add an API for listing (configuration) entities that met resistance was providing a base menu item for each List controller, and providing a hookMenu() method for the addition of even more.
Pros:

  • Less duplication: Currently if you change the path for that listing, you have to change it in at least two places, plus once more for each operation
  • Ease of adding a list controller: Adding the controller itself and it's definition in hook_entity_info() is it, no need to touch hook_menu()
  • The controller knows where it exists: Often times an operation might need to redirect or reload, and it shouldn't have to hardcode the destination

Cons:

  • Discoverability of menu items: With the exception of field_ui_menu(), menu items are all in your own .module, in hook_menu() or hook_menu_alter()
  • localize.drupal.org: It currently parses the hook_menu() definitions directly

Proposed resolution

  • Allow entities, or their controllers, to specify their path.
  • Allow controllers to provide menu items

Remaining tasks

@todo

User interface changes

N/A

API changes

N/A

Related issues

Comments

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Postponed » Active

Actually, because this is about more than just list controllers, unpostponing and unassigning. Discuss!

sun’s picture

Component: base system » entity system
Issue tags: -Configuration system +Field system

I'm tempted to rewrite/revamp the issue summary, because the actual problem is different from my perspective:

For me, it's primarily about derived router paths of entities.

  1. Every List controller needs at minimum

    • the URI of each entity
    • the edit + delete (+ ...) operation URIs for each entity (usually derived from the entity's URI)
    • the URI for adding a new entity of the listed type (typically used for #empty tables)
  2. Every Form controller needs at minimum

    • the URI of the entity
    • the URI on which the entity is listed (to $form_state['redirect'] after submission)
    • the delete (+ ...) operation URIs for each entity (usually derived from the entity's URI)
  3. Every View/Render controller needs at minimum

    • the URI of the entity
    • the edit + delete (+ ...) operation URIs for each entity (usually derived from the entity's URI)

To clarify what I mean with derived :

  public function getOperations(EntityInterface $entity) {
    $uri = $entity->uri();
    $operations['edit'] = array(
      'title' => t('edit'),
--->  'href' => $uri['path'] . '/edit',
      'options' => $uri['options'],
      'weight' => 10,
    );
    $operations['delete'] = array(
      'title' => t('delete'),
--->  'href' => $uri['path'] . '/delete',
      'options' => $uri['options'],
      'weight' => 100,
    );
    return $operations;
  }

Given what @EclipseGc did with Contextual Administration, I'm pretty sure he'll say that this information needs to be injected into each controller, so as to be able to view/edit/list entities on completely different URIs and not having them hard-coded within the controllers.

tim.plunkett’s picture

I definitely agree that the path should be injected.

tstoeckler’s picture

It was mentioned in some other issue, and I wanted to mention that here, because it wasn't clear to me at first:
We already have a precedent for this in core with Field UI. More specifically from the API doc on hook_entity_info:

  • admin: An array of information that allows Field UI pages to attach themselves to the existing administration pages for the bundle. Elements:
    • path: the path of the bundle's main administration page, as defined in hook_menu(). If the path includes a placeholder for the bundle, the 'bundle argument' and 'real path' keys below are required.
    • bundle argument: The position of the bundle placeholder in 'path', if any.
    • real path: The actual path (no placeholder) of the bundle's main administration page. This will be used to generate links.
    • access callback: As in hook_menu(). 'user_access' will be assumed if no value is provided.
    • access arguments: As in hook_menu().

That's obviously not the nicest way to do this, but...

catch’s picture

For a while I was thinking about trying to reduce the amount of data we store in the router. So just store information that has to be queried, plus a string pointing to a class name, and then move the title callback, description callback and similar in there. The idea would be to make building the router itself a lot cheaper since we'd not need to instantiate any of those classes until they were used, and you'd also be able to have router items that inherit from each other etc. I've forgotten how much of that is done by the new router patch (and deliberately not checking that now), but if we do that, then how much stays in hook_router_info() or hook_menu_item_info()? If we do this we also need to be careful about hook_menu_alter() - it'll be as easy to completely replace a router item but much harder to change just one thing (or for two modules to change two different things).

joachim’s picture

If I understand #4 correctly, we could have a 'base path' property in hook_entity_info, so for example in for nodes that would be just 'node/'.

That would be an enormous help to contrib modules, as currently there's no way to do something like add a tab to every entity.

xjm’s picture

xjm’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Added related issue

Anonymous’s picture

@joachim, I suggested something kind of similar in #1803818: Make it possible to dynamically provide additional routes for entities. We will need have that kind of information in order to provide serialization routes for arbitrary entities.

Crell’s picture

A couple of things:

1) We really need to rename entity controllers. "Controller" is "callable that the Kernel calls", basically formerly "page callback". Like "bundle" that's an inherited term from Symfony so we cannot really change it. For the remainder of this comment I will refer to Entity Storage Drivers (ESD) (what are currently called NodeController, UserController, etc.), to avoid confusion.

2) This is one of the reasons I've been calling for a Generator (#1705488: Implement a generator for Drupal paths) for a long time. Once we are able to refer to routes by name to generate URLs, a lot of the "injection problem" that EclipseGc is concerned about is taken care of. (Not all, but a lot.)

3) It sounds like what we're talking about, then, is each ESD exposing standardly-named routes that point to some standard Controller. $entity_type/{$id}/{?operation} seems like a reasonable default for that. And then we would need to piggyback on to that for each supported mime type. Then what an ESD would be specifying is: 1) Whether or not to generate routes. 2) A pattern (if not the default above). 3) A class to use for the controllers for those routes (if not the default above), possibly split by mime?

4) I haven't kept up on List "controllers", so I don't know what to offer there. Are there docs I can see first?

4) The discussion in #1801570: DX: Replace hook_route_info() with YAML files and RouteBuildEvent is highly relevant, as at the moment the leading contender is to use YAML, not a hook, to define routes. That would definitely impact dynamically generated routes of this sort.

effulgentsia’s picture

YesCT’s picture

#1810350: Remove menu_* entity keys and replace them with entity links is postponed on this issue. (updated issue summary)

YesCT’s picture

Issue summary: View changes

Added the exact comment

Crell’s picture

Title: Allow controllers to provide menu items » Allow entities to provide menu items

Retitling for clarity.

tim.plunkett’s picture

Title: Allow entities to provide menu items » Allow entity types to provide menu items
sun’s picture

Issue tags: +Release blocker

#1188388: Entity translation UI in core just landed, which introduced multiple entity plugin properties to define various paths of an entity.

That shouldn't have been introduced there, so this issue becomes even more major than it was.

Also, we have some very closely related issues:
#1332058: Need a way to distinguish "public/final" URIs for entities from admin and redirected ones
#1803586: Give all entities their own URI
#1823574-9: [Meta] Improve the Views Bulk Operations (VBOs) that are in core

I fear we need a meta issue to consolidate all of those discussions and change proposals. :-/

Lukas von Blarer’s picture

I am also coming from a similar issue: #1783964: Allow entity types to provide menu items

Lukas von Blarer’s picture

I created a patch implementing a small part of this to demo the usage: http://drupal.org/node/1832778#comment-6698138 What do you think of it?

andypost’s picture

There's a contrib project with a some UI that uses entity_uri() http://drupal.org/project/entity_path

sun’s picture

Sorry, this issue focuses too much on an aspect that I only consider an artifact of a larger problem space, and various other issues are running into that identical problem space again and again, so I created a dedicated issue for the actual, larger task that we need to address for D8:
#1839516: Introduce entity operation providers

andypost’s picture

Suppose first we need to make a widget for menu_link edit. Currently each module introduces own form for links.

Related issues:
#916388: Convert menu links into entities
#1882552: Deprecate menu_list_system_menus() and menu_ui_get_menus()
#1882218: Remove static menu link creation for menus in menu_enable() and elsewhere

tim.plunkett’s picture

I had no idea that #1188388: Entity translation UI in core snuck in part of this.

EntityManager documents the following keys:

  • menu_base_path
  • menu_view_path
  • menu_edit_path
  • menu_path_wildcard

Which is used by CustomBlock, Term, and various test entity types.

Um, what!?

plach’s picture

Yep, it was the only way to make that stuff work at the time. We need to get rid of those/refactor them in a sane way, as part of #1839516: Introduce entity operation providers I guess.

sun’s picture

I agree with @tim.plunkett in #20.

Either this, or #1839516: Introduce entity operation providers, or perhaps even both issues need to be bumped to critical.

There is no way we can release with these E.T. router path declarations in entity plugin definitions. They break encapsulation and separation of concerns on multiple fronts.

P.S.: It's about time to send translation.module to /dev/null, and ET to take over its namespace. Until then, all overly ambiguous "E.T." pun is intended :P

tim.plunkett’s picture

It's actually translation_entity.module, who knew?
See #1968970: Standardize module-provided entity info documentation and clean-up the @EntityType annotation for cleaning that mess up.

Crell’s picture

Pancho’s picture

Priority: Major » Critical
Issue tags: -Release blocker

Re #14:
If this is a release blocker, then it's critical per definition.
If not, then it might be just major plus "Revisit before release".

catch’s picture

Priority: Critical » Major
Issue tags: +revisit before beta
catch’s picture

Issue summary: View changes

Updated issue summary. adding that #1810350: Remove menu_* entity keys and replace them with entity links is postponed on this issue.

tim.plunkett’s picture

catch’s picture

Status: Active » Closed (duplicate)
Issue tags: -revisit before beta

Yep.