Problem

For every entity in entity listings we display several links that represent operations users can perform on those entities. At the time of writing these operation links are displayed in list controllers, and Field UI. Operations links are defined by those controllers and Field UI, and alter hooks are not called consistently, and there is no way to dynamically define operations for other modules' entities without abusing the alter hook.
Because there is no single place operations can be retrieved from, field operations are repeated all over core, for instance. List controllers can be used to retrieve operations, but that's not what they were designed to do.

Proposal

Introduce entity operation providers whose sole purpose is to retrieve entity operations from within the class itself and using hooks. Automatically run operations through entities' access controllers for increased and centralized security. We can then also make routes use _entity_access for arbitrary operations.

API additions & changes

  1. The addition of a new type of entity controller, which will not break anything.
  2. Introduce hook_entity_operations(), which will not break anything.
  3. Replace hook_entity_operation_alter() with hook_entity_operations_alter() to match the newly introduced hook.
  4. Operation access will be run through entities' access controllers in order to centralize and increase security. Operations that are exposed in operations providers can have their access checked in access controllers. Operations that are exposed through the hook can have their access checked through hook_entity_access(), which is invoked by access controllers.

Related issues (specific comments)

CommentFileSizeAuthor
#186 drupal_1839516_186.patch134.86 KBXano
#186 interdiff.txt3.72 KBXano
#183 drupal_1839516_183.patch132.75 KBXano
#183 interdiff.txt689 bytesXano
#180 drupal_1839516_180.patch132.73 KBXano
#173 drupal_1839516_173.patch133.5 KBXano
#165 drupal_1839516_165.patch133.51 KBXano
#165 interdiff.txt2.89 KBXano
#163 drupal_1839516_163.patch130.2 KBXano
#156 drupal_1839516_156.patch132.63 KBXano
#156 interdiff.txt1.65 KBXano
#151 drupal_1839516_151.patch130.37 KBXano
#151 interdiff.txt1.49 KBXano
#150 drupal_1839516_150.patch129.93 KBXano
#150 interdiff.txt22.5 KBXano
#142 drupal_1839516_142.patch114.11 KBXano
#141 drupal_1839516_141.patch0 bytesXano
#139 interdiff.txt1.18 KBXano
#139 drupal_1839516_139.patch111.14 KBXano
#134 drupal_1839516_134.patch111.24 KBXano
#134 interdiff.txt5.79 KBXano
#130 drupal_1839516_130.patch110.82 KBXano
#130 interdiff.txt16.8 KBXano
#128 drupal_1839516_128.patch99.69 KBXano
#128 interdiff.txt2.5 KBXano
#123 drupal_1839516_123.patch97.74 KBXano
#123 interdiff.txt406 bytesXano
#122 drupal_1839516_122.patch98.13 KBXano
#122 interdiff.txt21.1 KBXano
#120 drupal_1839516_120.patch87.09 KBXano
#120 interdiff.txt9.67 KBXano
#118 drupal_1839516_118.patch84.3 KBXano
#118 interdiff.txt20.5 KBXano
#116 interdiff.txt5.68 KBXano
#116 drupal_1839516_116.patch64.68 KBXano
#114 drupal_1839516_114.patch59.46 KBXano
#112 drupal_1839516_112.patch26.87 KBXano
#98 operations-1839516-98.patch25.42 KBtim.plunkett
#98 interdiff.txt6.69 KBtim.plunkett
#79 drupal_1839516_78.patch24.15 KBXano
#79 interdiff.txt368 bytesXano
#76 drupal_1839516_76.patch24.16 KBXano
#63 drupal_1839516_63.patch33.24 KBXano
#63 interdiff.txt1.04 KBXano
#62 interdiff.txt2.84 KBXano
#62 drupal_1839516_62.patch33.45 KBXano
#61 drupal_1839516_61.patch34.14 KBXano
#57 drupal-1839516-57.patch33.47 KBdawehner
#57 interdiff.txt15.47 KBdawehner
#54 drupal-1839516-54.patch32.24 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

Yes, we definitely need to address this from a more general point of view.

joachim’s picture

I made something that intersects with some of this for http://drupal.org/project/party -- each operation on a party (add, edit, attach, delete, etc) is represented by a class which provides the form, and form handlers.

Beyond that it's a bit simplistic in that permissions and paths were deduced from the declared machine name for the operation: so declaring an op 'edit' automatically mean you got party/x/edit and a permissions 'edit any party', 'edit own party', etc.

joachim’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Added

Bundle entities of fieldable entity types need to manually hack in Field UI operation URIs (i.e., /fields, /display).

to the problem space in the summary.

tim.plunkett’s picture

Category: task » feature

This is not a blocking task. Yes it'd be great to have, but I'm not sure it's something we can add in after feature freeze.

Don't get me wrong, half of the issues that this would simplify are pet issues of mine as well, but this is a feature.

plach’s picture

Category: feature » task

I'm afraid that without an operation API the only fixes available for the issues above will be hacky at best. If I understood the current guidelines to distinguish between pre and post freeze material correctly, if we are able to put this API together by mostly refactoring existing code we should be able to work on it after feature freeze. It's true it would be a new API, but actually it would mostly be glue code for stuff we already have in place.

I'd like to stress on the fact that we added the D7 Entity API just before the end of the code freeze...

catch’s picture

Priority: Critical » Major

We can definitely add this post-feature freeze since it'd be mainly refactoring existing code (and simplifying/cleaning up that code).

I don't think it's critical though, if we release without it we'll have some hacks in core, but not nearly as bad as some other hacks in core.

plach’s picture

Agreed, I didn't change the priority because we are in better shape with the critical queue right now :)

fubhy’s picture

Assigned: Unassigned » fubhy

I am totally on board with this. If you don't mind I would even be bold enough to assign this to me so I can work on this @ the weekend. Feel free to unassign / assign to yourself if you have any objections or are already working on this.

sun’s picture

Go ahead. ;) I didn't start with any code; writing this concise issue summary was enough of a large task on its own ;)

As mentioned in #1696660-92: Add an entity access API for single entity access, it might make sense to explore a plugin-based approach first and see how it goes.

sun’s picture

@fubhy: Any updates? I think we at least need to show some substantial progress for this issue before feature freeze (i.e., something beyond a 50%+ prototype) in order to have any chance to get this in anytime before or after. @catch already indicated that "we had this problem in the past already and we survived" and to some extent I agree with that (but then again not, but I'm also biased).

fago’s picture

I posted my thoughts about the difference to actions here: http://drupal.org/node/1846172#comment-6758258

fubhy’s picture

First of all, sorry for the late update. After starting to write the first lines of code I realized that this touches many topics at once. This lead to quite a few discussions that I had with @fago and today also with @EclipseGc about Entity Operations, Plugins, Actions, Access, Routing, Blocks&Layouts and how to bring all these concepts together (thanks you two). I now have a more sophisticated idea of what we should try to accomplish with this issue... So here is my conclusion / proposal:

EntityOperation

A type of plugin that is as an operation which a user can perform for a given (single) entity. Simply speaking: blocks (Blocks&Layouts) or actions (Actions API) that match certain criteria get aggregated into a list of operations for a given entity.

An example for a Block operation would be "view" and an example for an Action operation would be "publish".

We would achieve that through derivative discovery by iterating over the available Actions and Blocks and evaluating whether a given Block or Action applies as an operation for a given entity type. In that sense, operations are simply glue code that expose Blocks & Actions in a uniform way. Other, custom operation types (which may be completely custom and not use Blocks or Actions) can be added, of course.

Once we have that, we will be able to:

  • Retrieve lists of available (and, for the current user, accessible) operations. Note: Contextual Links would then, in case of entities, basically just load that list in the future.
  • Easily and consistently link to things like the Manage Fields / Manage Display pages without ever having to make assumptions about the URLs for these pages.
  • Build fully custom administrative pages for managing entities and move them to wherever we want.
  • ... And much more.

This is a very quick summary of what we've been discussing over the past couple of days. I will dive back into the code now. We will not be able to implement this full stack in the first iteration because it is blocked by the fact that the other systems are not quite finished yet but we can still build the framework for this already.

Again, sorry for the delay.

joachim’s picture

I was wondering whether these needed to be plugins, and I wasn't sure that they do -- aren't plugins for sets of things where only one (or some) may be active?

Whereas here, it seems to me it's more a case of 'here are the operations this entity can have' and that's it. These are more like controller classes where contrib can override one with hook_entity_info_alter() (unless those are plugins too on D8 -- I admit I've not been following that).

I'm not convinced about the relationship to actions described in the issue summary either. I can't see that most actions in node_action_info() duplicate operations.

But maybe I'm not managing to see the abstraction here :/

fago’s picture

I agree that #12 makes sense - have a separate implementation but make it simple to expose actions as entity operations also via the derivative plugin implementation.

tim.plunkett’s picture

I just got bit by the difference between user_user_operations() and user_action_info(), I'm now officially +100 on this issue.

mitchell’s picture

Would Diff be an operation?

Diffs viewed at something like '/node/{node}/revision/{vid}?[node:node:revision]' seems to match with:
* "Unlike actions, entity operations come with a menu callback..." (#1846172-6: Replace the actions API)
* "Operations may have to work on multiple router paths for the same entity type."

This is also a topic in #1826688: REST module: PATCH/update.

fubhy’s picture

Quick update: We discussed this again today with the VDC folks and concluded that we need to solve the dependencies first before we can come up with code that actually makes sense.

We will focus on #1846172: Replace the actions API for now and then build the prototype based on an implementation that already exposes actions as operations.

Wim Leers’s picture

I was told to follow this issue to be notified of progress on the "Node module implements the new D8 Entity Access API" front. There is now #1862750: Implement entity access API for nodes for that :) See #1696660-117: Add an entity access API for single entity access for implementations for other entity types.

joachim’s picture

I've had a stab at the sort of thing I had in mind: http://drupal.org/sandbox/joachim/1872460 Details on the project page, in the api.php file, and further comments in the code.

I confess I am very much out of touch with new developments in D8, and completely baffled by most of them. So in particular how this would be done with the plugin system is a total mystery to me.

I have the feeling my approach with this is going to seem rather naive in D8 terms -- but I'm working on a project where I'm creating custom entities all over the place, and having to create a UI for each type over and over seemed like tedious repetition. So this at least does the job on D7 :)

andypost’s picture

It would be good to normalize a case and length of operation title
Entity list operations also needs ability so change default operation.
Listings of config-entities partially would be a forms too, so probably listing is a kind of operation

Also related #1855402: Add generic weighting (tabledrag) support for config entities (DraggableListController)

fubhy’s picture

Assigned: fubhy » Unassigned

Okay, this has been sitting here for some time now... And enlightenment didn't come... However, I just had a chat with @timplunkett on IRC and we agreed to simply move forward with a rather straight-forward port of the existing system by simply refactoring what we have to OO / plugin system. That way we might even find the enlightenment that I've been longing for :).

More on monday.

fubhy’s picture

Issue summary: View changes

Updated issue summary.

Crell’s picture

Dave Reid’s picture

I've also come up with a simple API for entity operation links in D7 based on menu_contextual_links(). I'm using it to add a generic 'Operations' Views field handler since it enables more flexibility (like Entity Translation adding 'Translate' links). It might be of some help/inspiration here so I thought I'd share.

andypost’s picture

The other related task here is usage of some operations in "node-links" that's one of problems in #731724: Convert comment settings into a field to make them work with CMI and non-node entities

joachim’s picture

It would be really great to get another pair of eyes or two on http://drupal.org/project/entity_operations, in particular someone with a better understanding of D8 idioms than me -- the plugin system especially.

I'm using this on a project where I've several custom entity types, and it is working very nicely. For each entity type, I define a 'base path' for entities (analogous to the 'node' part of 'node/NID'), and then list the operations I want, such as 'view', 'edit', 'delete', 'devel' and so on. This lets me build a complete UI for each entity type very quickly out of reusable components.

It's proving to be very nicely extensible too: adding a 'delete' operation took very little code, and because one of my entity types is being used as an OG group type I've added an operation for the OG group membership management.

EDIT: Now also with operations that are defined as actions exposed to Services.

podarok’s picture

podarok’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

I'm working on http://drupal.org/project/config_translation for Drupal 8 and I need to inject translation operations on entity listings. Consulted with damiankloip in IRC, looks like this would be the way to do it, so a huge plus for making this happen :) My other options are *very* intrusive or *very* hackish and one-off.

[1:44pm] GaborHojtsy: hey!
[1:44pm] GaborHojtsy: looking for someone familiar with views ui list controller
[1:46pm] damiankloip_lapt: GaborHojtsy: What's up with it?
[1:46pm] GaborHojtsy: damiankloip_lapt: I'm looking at adding an operation on each view in the views listing from a contrib module and not sure at all of an elegant way
[1:46pm] GaborHojtsy: damiankloip_lapt: the operation would be "translate" btw
[1:48pm] GaborHojtsy: damiankloip_lapt: I can entity alter the views config entity and swap out the list controller, but that is not too friendly for other contribs
[1:48pm] GaborHojtsy: damiankloip_lapt: or I can use hook_page_alter stuff
[1:48pm] damiankloip_lapt: GaborHojtsy: Hmmm, yeah Currently you would have to override the getOperations method
[1:49pm] damiankloip_lapt: We don't have a better way to do these things for list controllers....
[1:49pm] damiankloip_lapt: WHich is why we need some sort of operations api
[1:49pm] GaborHojtsy: damiankloip_lapt: I think we'll need to, yeah
[1:49pm] damiankloip_lapt: GaborHojtsy: I think it will be quite important

Gábor Hojtsy’s picture

For counter-inspiration, this is the kind of code one needs to endure now to avoid altering the entity list controller (and therefore collide with other contrib modules).

/**
 * Goddamn ugly operations altering since there is no operations API.
 */
function config_translation_page_alter(&$page) {
  if (isset($page['content'])) {

    // Grab the first content item, which is the content block (hopefully :).
    $content_keys = array_keys($page['content']);
    $content = &$page['content'][reset($content_keys)];

    // Identify the views entity list page structure.
    if (isset($content['content'][0]['#attributes']['id']) && $content['content'][0]['#attributes']['id'] == 'views-entity-list') {
      $tables = &$content['content'][0];
      foreach (array('enabled', 'disabled') as $status) {
        foreach ($tables[$status]['table']['#rows'] as $id => &$row) {
          $row['data']['operations']['data']['#links']['translate'] = array(
            'title' => t('Translate'),
            'href' => 'admin/structure/views/view/' . $id . '/translate',
          );
        }
      }
    }

  }
}

Yes, this works (on my machine). Hope this helps make the case if not already :)

joachim’s picture

Eeek! :/

I'm going to reiterate what I said in #27: I think I've got something that's pretty neat, but I would like to get some other developers to look at it before I have a go at making a code patch, as I am sure there are parts of it I will be told are crack and need doing differently!

(EDIT: then again, it surely can't be worse than doing a hook_page_alter() to try to add things to entities generically. But still. I'd like a second opinion!)

Gábor Hojtsy’s picture

Here is a more complete counter-inspiration with more config entity types covered in core. Note some use forms, some use pages, some use 'data' inside operations, some don't, some use multiple tables, etc.

/**
 * Goddamn ugly operations altering since there is no operations API.
 *
 * @todo
 *   Rework this in favor of an operations API. See
 *   http://drupal.org/node/1839516
 */
function config_translation_form_alter(&$form, &$form_state, $form_id) {
  if (!user_access('translate configuration')) {
    // Do not alter forms with links to translations, if no access.
    return;
  }

  // Alter the filter admin overview to add translation links.
  if ($form_id == 'filter_admin_overview') {
    foreach ($form['formats'] as $id => &$format) {
      if (is_array($format) && isset($format['operations'])) {
        $format['operations']['#links']['translate'] = array(
          'title' => t('Translate'),
          'href' => 'admin/config/content/formats/' . $id . '/translate',
        );
      }
    }
  }

  // Alter the user roles form to add translation links.
  if ($form_id == 'user_admin_roles') {
    foreach ($form['roles'] as $id => &$role) {
      if (is_array($role) && isset($role['operations'])) {
        $role['operations']['#links']['translate'] = array(
          'title' => t('Translate'),
          'href' => 'admin/people/roles/edit/' . $id . '/translate',
        );
      }
    }
  }

}

/**
 * Goddamn ugly operations altering since there is no operations API.
 *
 * @todo
 *   Rework this in favor of an operations API. See
 *   http://drupal.org/node/1839516
 */
function config_translation_page_alter(&$page) {
  if (!user_access('translate configuration')) {
    // Do not alter pages with links to translations, if no access.
    return;
  }

  if (isset($page['content'])) {
    // Grab the first content item, which is the content block (hopefully :).
    $content_keys = array_keys($page['content']);
    $content = &$page['content'][reset($content_keys)];

    // Fingerprint the views entity list page structure.
    if (isset($content['content'][0]['#attributes']['id']) &&
        $content['content'][0]['#attributes']['id'] == 'views-entity-list') {
      $tables = &$content['content'][0];
      foreach (array('enabled', 'disabled') as $status) {
        foreach ($tables[$status]['table']['#rows'] as $id => &$row) {
          $row['data']['operations']['data']['#links']['translate'] = array(
            'title' => t('Translate'),
            'href' => 'admin/structure/views/view/' . $id . '/translate',
          );
        }
      }
    }

    if (isset($content['content'][0]['#rows'])) {
      $row_keys = array_keys($content['content'][0]['#rows']);
      $first_key = reset($row_keys);
      $rows = &$content['content'][0]['#rows'];

      // Fingerprint the contact module entity list page structure.
      // No good ID to match for unlike views, but we can reasonably be sure
      // with checking for category and recipients.
      if (isset($rows[$first_key]['category']) &&
          isset($rows[$first_key]['recipients'])
        ) {
        foreach ($rows as $id => &$row) {
          $row['operations']['data']['#links']['translate'] = array(
            'title' => t('Translate'),
            'href' => 'admin/structure/contact/manage/' . $id . '/translate',
          );
        }
      }

      // Fingerprint the menu summary table structure.
      if (isset($rows[$first_key]['title']['class']) &&
          in_array('menu-label', $rows[$first_key]['title']['class'])) {
        foreach ($rows as $id => &$row) {
          $row['operations']['data']['#links']['translate'] = array(
            'title' => t('Translate'),
            'href' => 'admin/structure/menu/manage/' . $id . '/translate',
          );
        }
      }

      // Fingerprint the shortcut summary table structure.
      if (isset($rows[$first_key]['operations']['data']['#links']['edit']['options']['entity_type']) &&
          $rows[$first_key]['operations']['data']['#links']['edit']['options']['entity_type'] == 'shortcut') {
        foreach ($rows as $id => &$row) {
          $row['operations']['data']['#links']['translate'] = array(
            'title' => t('Translate'),
            'href' => 'admin/config/user-interface/shortcut/manage/' . $id . '/translate',
          );
        }
      }
    }

  }
}

Horrifying enough? :)

joachim’s picture

I had a read of the D8 Plugin system docs yesterday and I'm really not sure how to make this with plugins.

We have operations such as 'view', 'edit', 'publish', 'translate'. Those should presumably be plugins.

But what I've found in my D7 module is that when an entity type makes use of an operation, it may need to tweak what that op does. Eg, some entity types could want 'publish' as a tab, some only want to expose it as a VBO/action.

My hunch was this would mean using plugin derivatives, but I started to get totally baffled about how these work and how they can be used.

Gábor Hojtsy’s picture

@joachim: yeah, currently entity operations for config entities (at least which are what I'm concerned about for this module) are defined in the config entity class via render API structures. There is no way to inject stuff into them at that point, and the menu tabs/items are separately defined. Your module seems to have 7.x code, which is in itself not very applicable to how Drupal 8 attempts to structure its code unfortunately.

joachim’s picture

Yes, I wrote my module for D7 because a) I needed this for a D7 project and b) I haven't got my head round D8 plugins. So I figured that a D7 version was a good place to start.

joachim’s picture

I'm actually wondering whether plugins are in fact the right thing to use here.

After all, Views handlers are now in core, and (AFAIK!) they are not plugins. And what we have here is somewhat analogous to those: there is a class which determines behaviour, which is used in multiple places, and the places which use it get to specify additional options. That's the same way that Views handlers have options set in hook_views_data().

tim.plunkett’s picture

All of views handlers are absolutely plugins. But since they are generic and can be reused, we need a higher level hook (hook_views_data()) to assign them to a use case.

Example:
The plugin \Drupal\views\Plugin\views\field\Boolean is used by the following columns:
node.status
node.promote
node.sticky
node_revision.status
user.status
comment.status
language.locked

joachim’s picture

Ah right. Sorry, I am totally out of touch with the new stuff in D8! :/

That's the same sort of architecture as we'd want here. Are they not derivative plugins then, just referred to by hook_views_data() in the same way that on D7 referred to the handler classes? How does the plugin get instantiated with the configuration data from the hook?

The 'view entity' plugin would be assigned by each entity type that wants a UI. The 'view revisions' plugin would only be assigned by entity types that support revisions and want a UI, and so on.

dawehner’s picture

That's the same sort of architecture as we'd want here. Are they not derivative plugins then, just referred to by hook_views_data() in the same way that on D7 referred to the handler classes? How does the plugin get instantiated with the configuration data from the hook?

There is still a method which create an handler instance by taking the $table and $field. As it then calls the plugin manager internally, you can indeed use derivatives for handlers without issues.

EclipseGc’s picture

There are two different things at play here. I don't know if this has been captured before, but we have "operations" as we're discussing here, and that is not to be confused with "actions". There are operations which might farm out their interaction to an action, but these are not the same thing, and that is worth noting.

An action is a piece of code (hopefully a plugin in D8, I'm looking at you bojanz) and an operation is, in all likeliness some sort of plugin which implements FormInterface and can be semi-generically attached to entities. To some degree, I feel like the entity form controller system can accomodate this. On the other hand, we have Gabor's code that I feel sort of proves the contrary. I really just wanted to throw out the fact that this distinction exists and see where the conversation goes from there.

Eclipse

Gábor Hojtsy’s picture

@EclipseGC: if entity list controllers would allow for any outside mangling with the operations, that would be great. Right now, they are all internally defined. No plugins, alters, hooks, or whatnot involved. Also entity list controller exposed operations have no direct relation to menu items, you still need to define the menu items separately for the operations. This was I believe deliberately set up this way to decouple them.

fubhy’s picture

I see operations as an aggregate of everything that you can do with a single entity or against multiple entities (bulk operations + lists). This is not limited to actions or things that come with a form. We should also put 'view' or 'list' into this mix.

joachim’s picture

> I don't know if this has been captured before, but we have "operations" as we're discussing here, and that is not to be confused with "actions". There are operations which might farm out their interaction to an action, but these are not the same thing, and that is worth noting.

I was initially confused about how to bring operations and actions together. But it turned out when I was coding the D7 module that I could quite nicely make actions a subclass of operations.

> Also entity list controller exposed operations have no direct relation to menu items, you still need to define the menu items separately for the operations. This was I believe deliberately set up this way to decouple them.

What I've done on D7 is provide an controller class for Entity API's admin UI. This then provides the operation links in the entity list, and could feasibly do the actions in the dropdown.

> I see operations as an aggregate of everything that you can do with a single entity or against multiple entities (bulk operations + lists). This is not limited to actions or things that come with a form. We should also put 'view' or 'list' into this mix.

Yes!

It would be *really* useful if people could look at my D7 module to see how I've managed to integrate things together.

EclipseGc’s picture

I was initially confused about how to bring operations and actions together. But it turned out when I was coding the D7 module that I could quite nicely make actions a subclass of operations.

Perhaps, though I'd be tempted to make operations extend the action instead when I needed that.

I see operations as an aggregate of everything that you can do with a single entity or against multiple entities (bulk operations + lists). This is not limited to actions or things that come with a form. We should also put 'view' or 'list' into this mix.

I'm still very much in "no" territory here. They're two different things, and conflating them will make this much much harder both conceptually and for the code. Separating them really just means they're separate and then when there's overlap, there will also be some overlap of effort. Often times an action might work for an entity, but sans a UI that does that setup (parsing derivatives and providing configuration) and operation plugin for the entity that wants to use the action can perform the same function within its own use case. With regard to "view" and "list" I think I'm on board there. It sounds to me like we're wanting to essentially remove most of the entity controllers in favor of another paradigm. Does that seem accurate?

@Gabor,

Basically your whole comment is, I think, solvable if we move the plugins direction. Metadata can be leveraged to create menu items, and the nature of plugins in general solved your other objections there.

Eclipse

fubhy’s picture

It sounds to me like we're wanting to essentially remove most of the entity controllers in favor of another paradigm. Does that seem accurate?

I don't think we want to remove them. I think what we want is to wrap them in a plugin layer that invokes them and controls the route(s) that they are attached to.

EclipseGc’s picture

ok, I consider that largely "same difference" but fair enough.

joachim’s picture

> Often times an action might work for an entity, but sans a UI that does that setup (parsing derivatives and providing configuration) and operation plugin for the entity that wants to use the action can perform the same function within its own use case.

What I found with practical applications of this was this:

If you're going to have an action, then you probably want it exposed to VBO.
That means it has to have some sort of means to expose a form.
Then if you can see the form for multiple entities, you feasibly could want it just for one.
Which means you might want it output as a form on the entity, or as a tab on the entity.

Hence I figured action operation handlers might as well subclass the page and form operation handlers, and so gain the benefit of being able to be exposed that way, should the entity type wish it.

An example of this for my project was an operation for a user to 'book' a custom entity, or 'assign' it to another user. Because of the way the handler classes work, this is available as:

- VBO for acting on entities en masse
- Services, for use from a mobile app
- a tab on the entity for acting on a single entity
- ... and the link to that tab is available as a Views field too.

tim.plunkett’s picture

As I pointed out in #1783964-20: Allow entity types to provide menu items, #1188388: Entity translation UI in core added a one-off for deriving the admin paths for entities. And apparently this issue is supposed to solve this?

joachim’s picture

If you mean this bit of that patch:

+ * To make Entity Translation automatically support an entity type some keys
+ * may need to be defined, but none of them is required unless the entity path
+ * is different from ENTITY_TYPE/%ENTITY_TYPE (e.g. taxonomy/term/1), in which
+ * case at least the 'menu_base_path' key must be defined. This is used to
+ * determine the view and edit paths if they follow the standard path patterns.
+ * Otherwise the 'menu_view_path' and 'menu_edit_path' keys must be defined. If
+ * an entity type is enabled for translation and no menu path key is defined,
+ * the following defaults will be assumed:
+ * - menu_base_path: ENTITY_TYPE/%ENTITY_TYPE
+ * - menu_view_path: ENTITY_TYPE/%ENTITY_TYPE
+ * - menu_edit_path: ENTITY_TYPE/%ENTITY_TYPE/edit

Then yes!

What I would propose is the following:

- entity info defines 'menu base path' and the loader wildcard
- if the entity type provides a 'view' operation, then you know you can view the entity at 'BASE/%LOADER/view'
- if the entity type provides an 'edit' operation, then you can see that at 'BASE/%LOADER/edit'.
- there'd probably be a helper function to get you an entity op's path, eg get_me_the_path_for($this_entity, 'view').

I'm hoping to rework my D7 module so the operations are defined in their own hook rather than an extra big array in hook_entity_info(), which I think will be needed for performance -- hook_entity_info data is always loaded, whereas entity operations data is mostly only needed when clearing caches. After that I'll start porting it to D8, but as previously stated, I would need some help with the plugins side of things!

Does anyone have any idea of where this new system should go? In entity module, or in a module of its own?

tim.plunkett’s picture

Issue tags: +Stalking Crell

We have a new Routing system. %foo is deprecated, and we have no common way of referring to Routes yet. This hack is going to be interesting to resolve.

Crell’s picture

YesCT’s picture

YesCT’s picture

Issue summary: View changes

Updated issue summary.

fago’s picture

discussed entity operations with effulgentsia and tim.plunkett today.

I try to summarize it below:
- we've various things that tie into entity operations, but no 100% match. Let's keep things separate and integrate them with each other where it makes sense.
- We need a "registry" of operations per entity type, that is modular extensible. For each operation we need a way to get the operation URL and a way to generate the link to it.

So the idea was to
- implement a simple EntityOperationsController (or handler) and move the current operation logic from the list controller into it
- inject the operations controller as dependency into the list controller
- provide a EntityDefaultOperationsController class, which provides basic view/edit/.. operations and can be extended as needed
- add a hook for adding further operations, e.g. hook_entity_operation
- We'll need to be able to check operation access during rendering, which could be solved by checking access via the routes?

Having the operations separated out allows us to inject customized operations in situations, where the usual operations might not fit.

fago’s picture

Issue summary: View changes

Updated issue summary.

dawehner’s picture

FileSize
32.24 KB

The current interface has just one method though I think we need one for getting the normal operations, one for getting them with the altered values
and one for applying access to them.

Gábor Hojtsy’s picture

Status: Active » Needs review

Send for testing. I see the current plan does not yet have a hook to extend operations across entities, my main interest from the config_translation module POV is to have that feature.

dawehner’s picture

Status: Needs work » Needs review
FileSize
15.47 KB
33.47 KB

Added some alter hooks and moved the access checking.

I'm wondering how to determine the router name of a generic operation. Maybe the access checking should better fake a request.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/EntityDefaultsOperationsController.phpundefined
@@ -57,4 +106,37 @@ public function getOperations(EntityInterface $entity) {
+    $this->moduleHandler->alter(array('entity_operations', $entity->entityType() . '_operations'), $operations);
+

Alter hook! Yes. We need this for the config translation module proposed for core!

One thing I'd definitely add to this is the entity itself as an argument. In config translation we want to add a translate operation to all config entity types. This is not really possible here, since we don't know the type of the entity (unless we implement each hook per entity which is not very scalable and not contrib-future-proof).

joachim’s picture

I don't really get this patch -- where are the actual operations?

Xano’s picture

Assigned: Unassigned » Xano

Going to add the entity to the alter hook, and hook documentation.

Xano’s picture

Assigned: Xano » Unassigned
Status: Needs work » Needs review
FileSize
34.14 KB

Alright. I haven't read the entire issue, but why don't we use the entity's access controller instead of the routing system? We are converting/have converted entity routing to the new access controller anyway, so this looks like an unnecessary and illogical layer of abstraction.

My apologies for not posting an interdiff. I usually make those by first committing the original patch, then my changes, and finally doing a commit diff, but the original patch did not apply and I am still too jetlagged to figure out how else to make interdiffs.

Xano’s picture

FileSize
33.45 KB
2.84 KB

As per my talk with @dawehner on IRC, I put the alteration after the access control, and updates the access control to leverage AccessibleInterface rather than the routing system.

Xano’s picture

FileSize
1.04 KB
33.24 KB

And now without unnecessarily injected services.

tim.plunkett’s picture

I'm not 100% sure I agree with coding around the routers. These are explicitly route-bound operations, I still think we should have those checks.

+++ b/core/lib/Drupal/Core/Entity/EntityDefaultsOperationsController.phpundefined
@@ -0,0 +1,123 @@
+   * @param \Symfony\Cmf\Component\Routing\RouteProviderInterface $route_provider
+   *   The route provider.
+   * @param \Symfony\Cmf\Component\Routing\DynamicRouter $router
+   *   The router.
+   * @param \Drupal\Core\Access\AccessManager $access_manager
+   *   The access manager

+++ b/core/lib/Drupal/Core/Entity/EntityListController.phpundefined
@@ -7,6 +7,8 @@
+use Symfony\Component\HttpFoundation\Request;

These are gone?

+++ b/core/lib/Drupal/Core/Entity/EntityDefaultsOperationsController.phpundefined
@@ -0,0 +1,123 @@
+    $this->entityInfo = entity_get_info($entity_type);

This is actually the plugin.manager.entity service

+++ b/core/modules/contact/lib/Drupal/contact/CategoryOperationsController.phpundefined
@@ -0,0 +1,41 @@
+    if (module_exists('field_ui')) {

module_handler service

amateescu’s picture

And how about using 'operation' (singular) instead of 'operations', like we do in the rest of core? :)

Xano’s picture

I'm not 100% sure I agree with coding around the routers. These are explicitly route-bound operations, I still think we should have those checks.

Personally, I don't like that we tie operations to routes, especially because we already have operation-based access control for entities that is pretty extensive.

tim.plunkett’s picture

Right, but these operations are physical links that take us to a route, they don't just trigger the operation directly.

Xano’s picture

I'm not too sure we should tie this API to hyperlinks anyway. Those may only be one use case.

dawehner’s picture

On the longrun the operations api should declare the routes not the other way round.

andypost’s picture

Suppose patch should care about URI-templates commited in #1970360: Entities should define URI templates and standard links

Gábor Hojtsy’s picture

D8MI is happy with #2004408: Allow modules to alter the result of EntityListController::getOperations so we don't have a pressing need for this.

Xano’s picture

Version: 8.x-dev » 9.x-dev

Moving to Drupal 9 as this breaks BC and it's past API freeze.

plach’s picture

Version: 9.x-dev » 8.x-dev

Parts of this can still be implemented as API additions, let's not corner ourselves :)

Xano’s picture

The existing, limited, operations 'API' was originally built for entity list controllers. This also introduced hook_entity_operations_alter(), which at the moment of writing is also independently called by field_ui.module. This is the first sign if reinventing entity operation management in core and I can only imagine how ridiculously complex and inconsistent this will become, especially in contrib, if we do not add a single unified approach to core, how light-weight it might be. Emphasis: I do not care as much about the built-in features as I do care about allowing contrib to easily extend this.

Are people still in favor of using an operations controller?

Xano’s picture

Title: Various subsystems require an Entity Operations API. Introduce one. » Introduce entity operation controllers
Status: Needs work » Needs review
FileSize
24.16 KB

This patch is a re-roll of the one from #63, with a few changes:

  1. hook_entity_operations() and hook_ENTITY_TYPE_operations() have been introduced.
  2. hook_entity_operation_alter() has been renamed to hook_entity_operations_alter() and the arguments have changed order for consistency with the newly introduced hooks.
  3. Field operations are no longer exposed through entity types' own operations controllers, but through field_entity_operations(), because this saves code, ensures consistency, and because Field also already defines the routes the operations point to itself.
plach’s picture

Xano’s picture

Status: Needs work » Needs review
FileSize
368 bytes
24.15 KB
Xano’s picture

Berdir on IRC:

field_entity_operations() => that belongs to field_ui, not field. Where the alter hook is already implemented, including the manage form display link that you are missing

This lead me to check out field_ui_entity_operation_alter(). Field UI performs access control there, but the patch uses $entity->access() for that for every operation. This will fail, because there is no code that grants access Field UI's operations. Searching for hook_entity_access() reveals that it is actually hook_ENTITY_TYPE_access(), which makes it useless for Field UI to use.

I'm inclined to also add a real hook_entity_access(), as it is a requirement for any module that provides operations for an undefined collection of entity types, such as all types.

Xano’s picture

AccessibleInterface, which defines the access() method, documents that it only supports checking access for CRUD operations. However, EntityAccessControllerInterface does not have this limitation, and being able to route access checks for any operation through entities' access controllers gives us two major advantages:

  1. Modules can grant or deny access to other module's operations.
  2. Access to operations can be checked without having to deal with the operations controller, which is for building links, rather than checking access.

What we can do is check access through entities' access controllers directly and avoid AccessibleInterface::access(). This still requires a hook_entity_access().

Xano’s picture

damiankloip’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
joachim’s picture

Status: Needs review » Needs work

> class EntityDefaultsOperationsController

We seem to have dropped the pattern of calling classes EntityDefaultFoo in D8.

> + public static function createInstance(ContainerInterface $container, $entity_type, array $entity_info) {

In the D8 classes I've seen so far, the pattern is to put this above __construct().

> getOwnOperations / getOperations

The names of these methods are confusing, and don't seem to relate to the difference between them as stated in the docs.

I think the patch might be simpler to follow if it just changed one core entity type over to the new system -- assuming that is possible to do without tests failing of course!

Also, I really think the operations controller should be providing routes for operations.

I have been spending some time on #2084301: Add a Config Entity example for Example module and I am discovering that defining a config entity is something that takes a lot of code. Having to define routes for add/edit/delete for each config entity type when these are just standard patterns based on the URI is repetitive.

Xano’s picture

Also, I really think the operations controller should be providing routes for operations.

I have been spending some time on #2084301: Add a Config Entity example for Example module and I am discovering that defining a config entity is something that takes a lot of code. Having to define routes for add/edit/delete for each config entity type when these are just standard patterns based on the URI is repetitive.

We may shoot ourselves in the foot by doing this, as we have no way of knowing that every operation URL can simply be accessed through entities' canonical URLs suffixed by the operation name. Some operations may need extra path fragments or GET data (such as against CSRFs).

joachim’s picture

> Some operations may need extra path fragments or GET data (such as against CSRFs).

I haven't got my head round the dynamic routes system -- only enough to see it looks really complicated :(

Could we allow for that possibility there?

Alternatively, I don't think its so bad a consequence if we don't allow that:

a. we don't yet have any operations that require it: we might never need it
b. if any operations DO need this in future, then providers of those operations are no worse off than they currently are, surely. They'd just have to define their own routes, which they do at the moment.

Xano’s picture

Core already provides many operations that require GET parameters, such enable and disable. Let's not go through the trouble of doing this and very likely shooting ourselves in the foot. If we want to define routes automatically, we should also call operations automatically through plugins. Otherwise we should just give developers the freedom to implement the routing the way they need.

dawehner’s picture

There should be one issue to convert the 'href' in operations to route names/parameters. Does it make sense to do that part of this issue, as it touches pretty much all operations already?

Xano’s picture

I know that we are busy converting all path-based code to routes and why we do it, but I haven't read anything about support for external URLs. How does that work?
A possible use case would be the user listing on groups.drupal.org. Accounts on g.d.o are linked to accounts on drupal.org and one operation may be "Edit on primary site", which will point to the URL on drupal.org, which is external.

Xano’s picture

Issue summary: View changes

Updated issue summary.

Xano’s picture

I updated the issue summary to clearly communicate that we are working towards adding an entity controller type.

Gábor Hojtsy’s picture

The plan in the issue summary sounds sane to me.

fago’s picture

The issue summary is reasonable, but does not specify how the solution should like.
Have you seen the summary of the discussion in portland, i.e. #53? It starts to outline a possible solution.

As regard to access the route access approach makes sense, but covering with extended entity_access() operations also. Do we have cases where the route access differs from sole entity_access()?

Xano’s picture

As regard to access the route access approach makes sense, but covering with extended entity_access() operations also. Do we have cases where the route access differs from sole entity_access()?

My plan is to use the entity access controller, and not AccessibleInterface::access(), as the later belongs to typed data and is for CRUD only, whereas access controllers are supposed to be handle any entity operation. My argument against using dummy routes for access control is that you can never be fully sure that the route you constructed will be properly interpreted by the access requirements; some routes may use GET parameters for access control, but you can't know that.

tim.plunkett’s picture

AccessibleInterface should have nothing to do with TypedData. That was a bizarre decision.
Similarly, AccessInterface is closely tied to routes, and has its own set of constants that are very helpful to understanding the difference between FALSE and NULL.

So let's please not add a 3rd system. Entity's usage of AccessibleInterface == the entity access controller...

I have to reread this issue tomorrow, hopefully Xano and I can chat about this soon.

fago’s picture

Entity access just builds upon the typed data Accessibleinterface + have the controllers to implement it. It's not like it is a third system?

Xano’s picture

See #2095125: Use access constants in every access control context to clear up that mess. If you want, we can move AccessibleInterface from typed data to \Drupal\Core\Access there too so it all makes sense again.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
6.69 KB
25.42 KB

This patch straight up doesn't work, it removes stuff like ViewListController::getOperations() and references ViewOperationsController without adding the file...
I have a lot of other thoughts abut the actual approach here, but I'll talk to Xano in person about it and then sum p here.

In the meantime, here's a partial reroll with some implementation tweaks.
These controllers *always* come from EntityManager, so we can set the stuff we need instead of forcing subclasses to inject it.

Xano’s picture

Xano’s picture

@tim.plunkett and I just had a long and emotional conversation about this:

  • We will not go for entity operation controllers, but add a plugin type and a plugin manager.
  • Entity operations plugins will be able to return multiple operations. This is more forwards compatible with D9 than introducing new hooks. Let's do this right the first time and it's the same amount of work.
  • All access control will go through the access controller, which in turn will invoke hook_entity_access() and hook_ENTITY_TYPE_access().
Xano’s picture

Title: Introduce entity operation controllers » Introduce entity operation plugins
joachim’s picture

> Entity operations plugins will be able to return multiple operations. This is more forwards compatible with D9 than introducing new hooks. Let's do this right the first time and it's the same amount of work.

Can you explain more about that?
How does it work if, say, nodes have basic operations provided by node module, then a translate operation provided by the translation module, and then further operations defined by different contrib modules?

fago’s picture

Entity operations plugins will be able to return multiple operations. This is more forwards compatible with D9 than introducing new hooks. Let's do this right the first time and it's the same amount of work.

What would do the plugin class then? I don't see a need for class here?

Xano’s picture

How does it work if, say, nodes have basic operations provided by node module, then a translate operation provided by the translation module, and then further operations defined by different contrib modules?

Any module can provide any number of plugins and any plugin can return any number of operations for a particular entity. Operations are not provided any other way anymore. The entity manager will force all operation access through $entity->access() for consistent security.

What would do the plugin class then? I don't see a need for class here?

Plugins are a consistent way of providing operations. We can't use YAML because of dynamic checks to see if operations are technically possible for a particular entity.

joachim’s picture

I'm still confused.

I'd assumed 1 plugin == 1 operation, so that the plugin that provides, say, the translate operation can be used across different entity types.

Is it more the case that a plugin provides a set of operations, and it's the set that's reusable? What kind of sets would there be?

fago’s picture

Plugins are a consistent way of providing operations. We can't use YAML because of dynamic checks to see if operations are technically possible for a particular entity.

But that's not a reason to go with a plugins, this is a reason to not to go with YAML. However, once there is no need for a plugin class this is an argument against plugins. What it means it should use a hook instead.

tstoeckler’s picture

Talked a lot about this problem space with effulgentsia, alexpott and EclipseGC. Not about this issue, but about config_translation.module where we also have things that are borderline plugins.

In this particular case, where you want standardized discovery but not instantiation, I think we should still create a plugin-ish manager that simply implements DiscoveryInterface instead of PluginManagerInterface. That still allows to use a simple info hook but you can easily change to YAML/Annotations/... and also have a standardized way to do caching, defaults applying etc. In fact all of the discovery-related stuff that DefaultPluginManager does could be split into a discovery-only class.

I think that would be way preferable to the old-style foo_info(), that contains a drupal_static() does a manual drupal_alter(), calls foo_defaults(), etc.

Xano’s picture

I am almost done converting this to plugins. One major problem I have run into is #2102129: Map entity operations to route names, which makes automatic link generation a pain.

Xano’s picture

As #2102129: Map entity operations to route names has received no response, I suggest we stick to exposing operations in a hook and in an entity controller, just like we do now. It is not as nice as plugins, but it will save us a lot of code, because otherwise we would have needed one plugin per operation per entity type, as URLs just cannot be predicted in core's current state. The only change I suggest we make to the current situation is the introduction of entity operation controllers that do (almost) every operation-related task that list controllers do now, but in a reusable way. Besides that, operation link access control will go through entity access controllers as planned.

If this is okay, I will write a patch this weekend.

dawehner’s picture

Shocking people don't respond to all the issues :)

dawehner’s picture

Issue summary: View changes

Updated the issue summary to reflect the current status.

Xano’s picture

Title: Introduce entity operation plugins » Introduce entity operation controllers
Issue summary: View changes
FileSize
26.87 KB

This patch goes back to the original approach of entity operation controllers, but with the major difference that it strictly entity access controllers to check user access to operations links. It's a work in progress, but the EntityOperationsController, ConfigEntityOperationsController and CustomBlockOperationsController classes are done.

Crell’s picture

Can we please not use the word "controller" here? We're trying to remove the places we're re-using that word, not add more of them.

Xano’s picture

Status: Needs work » Needs review
FileSize
59.46 KB

This patch renames OperationsController to OperationsProvider and converts all list controllers to use an operations controller. Some access controllers do need to be updated with access checks for some operations, most notably controllers for vocabularies and user roles.

Xano’s picture

Status: Needs work » Needs review
FileSize
64.68 KB
5.68 KB

Fixed the PHPUnit tests and the DI of ViewListController.

Xano’s picture

Title: Introduce entity operation controllers » Introduce entity operation providers
Status: Needs work » Needs review
FileSize
20.5 KB
84.3 KB

Most problems were caused by EntityListController, DraggableListController, and BlockListController not injecting all required services in their factory methods. These factory methods and constructors have been fixed, as well as those of list controller classes that aren't used as often. This should greatly reduce the number of test failures.

Xano’s picture

Status: Needs work » Needs review
FileSize
9.67 KB
87.09 KB

Not all operations providers extended from ConfigEntityOperationsProvider, and I fixed a DI issue with FilterFormatListController.

Xano’s picture

Status: Needs work » Needs review
FileSize
21.1 KB
98.13 KB
Xano’s picture

FileSize
406 bytes
97.74 KB
Xano’s picture

123: drupal_1839516_123.patch queued for re-testing.

podarok’s picture

Status: Needs work » Needs review

123: drupal_1839516_123.patch queued for re-testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
2.5 KB
99.69 KB

This should pass.

Xano’s picture

There are one or two @todo's left, which I will deal with tomorrow.

Xano’s picture

FileSize
16.8 KB
110.82 KB

I have fixed the @todo's.

theme_links() looks like it may support route names as well as hrefs, but as soon as the links use AJAX, they must have a href instead of a route name. Is there an issue about this we can reference, so EntityOperationsProviderInterface::getOperations()'s docblock can be updated as soon as the other issue is fixed?

This patch also introduces a handful of PHPUnit tests for operations providers that add links. There is one failure in ConfigEntityOperationsProviderUnitTest which has to do with mocking, but I can't pinpoint the exact cause of the failure.

Xano’s picture

#2129953: Abstract entity status out to an interface will clean up some up the code for to the enable and disable operations.

Xano’s picture

Status: Needs work » Needs review
FileSize
5.79 KB
111.24 KB

I wrapped the translation manager in a t() method as per timplunkett's recommendation over IRC.

klonos’s picture

Xano’s picture

Status: Needs work » Needs review

134: drupal_1839516_134.patch queued for re-testing.

Xano’s picture

FileSize
111.14 KB
1.18 KB

This should pass.

Xano’s picture

Status: Needs work » Needs review
FileSize
0 bytes

A man's gotta pull, when a man's gotta pull.

Xano’s picture

FileSize
114.11 KB

Oh, everybody look over there! A pink elephant!

klonos’s picture

...would be great if everyone got used to deprecate old patches/files once they upload new ones. We can do that after the D7 upgrade! ;)

Xano’s picture

Status: Needs work » Needs review

Oooh, shiny. Thanks for letting us know!

Xano’s picture

142: drupal_1839516_142.patch queued for re-testing.

Xano’s picture

Status: Needs work » Needs review

142: drupal_1839516_142.patch queued for re-testing.

Cannot reproduce test failures locally.

Xano’s picture

Now config_translation is part of core, we probably want to make it provide translate operations for config entities. We should perhaps also see if we can do the same for content_translation.

Xano’s picture

Status: Needs work » Needs review
FileSize
22.5 KB
129.93 KB
Xano’s picture

FileSize
1.49 KB
130.37 KB

Forgot to remove some old code.

Xano’s picture

Status: Needs work » Needs review
Xano’s picture

Issue summary: View changes
Xano’s picture

Status: Needs work » Needs review
FileSize
1.65 KB
132.63 KB
Xano’s picture

Test class properties that contain mocks will need to be documented with an additional |\PHPUnit_Framework_MockObject_MockObject.

Xano’s picture

Dear d.o, please update the issue summary when I ask you to.

Xano’s picture

Issue summary: View changes

Apparently we have issue summary summaries...

Xano’s picture

Issue summary: View changes
Xano’s picture

156: drupal_1839516_156.patch queued for re-testing.

Xano’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Stalking Crell
FileSize
130.2 KB

Re-roll, made sure there are no more occurences of operations controller (used operations provider instead), and fixed #157.

Xano’s picture

Status: Needs work » Needs review
FileSize
2.89 KB
133.51 KB
Xano’s picture

165: drupal_1839516_165.patch queued for re-testing.

Xano’s picture

Can someone please review this patch? I've been re-testing and re-rolling it for a while now. Committing this would also allow us to work on follow-up issues, such as adding a Views field handler plugin for multiple operations.

165: drupal_1839516_165.patch queued for re-testing.

Xano’s picture

Status: Needs work » Needs review

165: drupal_1839516_165.patch queued for re-testing.

andypost’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityOperationsProvider.php
@@ -0,0 +1,133 @@
+class EntityOperationsProvider implements EntityOperationsProviderInterface, EntityControllerInterface {
...
+  public function getOperations(EntityInterface $entity, AccountInterface $account) {
...
+    $operations = $this->getDefaultOperations($entity);

+++ b/core/lib/Drupal/Core/Entity/EntityOperationsProviderInterface.php
@@ -0,0 +1,41 @@
+interface EntityOperationsProviderInterface {
...
+   *   Keys are operation machine names and values are arrays with the following
...
+   *   - title: The link text.
+   *   - href: (optional) The link URL. If omitted, the 'title' is shown as a plain text
...
+   *   - html: (optional) Whether or not 'title' is HTML. If set, the title
...
+   *   - attributes: (optional) Attributes for the anchor, or for the <span>
...
+  public function getOperations(EntityInterface $entity, AccountInterface $account);

Why interface declares only getOperations() but all over core used getDefaultOperations() ?
Seems this one needs postponed follow-up to convert 'href-path' to routes

Xano’s picture

Why interface declares only getOperations() but all over core used getDefaultOperations() ?

getDefaultOperations() are the operations as declared by the provider itself. It is a protected method used to separate the definition of default operations and the hook-related logic. Child classes can now easily do whatever they want to the operations without interfering with the hooks and access control.

Seems this one needs postponed follow-up to convert 'href-path' to routes

Thanks for reminding me to do that!

Xano’s picture

Xano’s picture

FileSize
133.5 KB

Re-roll, because the patch no longer applied to entity.api.php. No production code has been changed.

damiankloip’s picture

What's the logic behind calling them Providers, but essentially just being another controller, and being stored like that?

Xano’s picture

andypost’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Avoid commit conflicts

I think it's ready, very solid and covered with tests!

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -Avoid commit conflicts

We don't use that tag anymore.

I'd like to have someone from the Entity team look at this, like amateescu or Berdir, before this goes in.

Xano’s picture

173: drupal_1839516_173.patch queued for re-testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
132.73 KB

Re-roll because of #2109303: Convert CSRF checks in controllers to the routing system, which touched ViewListController.

sun’s picture

Wow, this patch looks great and is definitely a big step forward as-is already! :)

I did not look into lower-level implementation aspects in my review. I do have some questions about the high-level architecture:

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityListController.php
    -      $operations['edit']['href'] = $uri['path'];
    
    +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityOperationsProvider.php
    +    $operations['update']['href'] = $uri['path'];
    
    +++ b/core/lib/Drupal/Core/Entity/EntityOperationsProvider.php
    +  protected function getDefaultOperations(EntityInterface $entity) {
    ...
    +    $operations['update'] = array(
    

    Could we rename the "update" operation back to "edit"?

    The way I see it, operations are abstract constructs that do not really map to CRUD operations.

    An "update" operation does not necessarily imply that an entity will actually be updated. Instead, the "edit" operation performs the necessary actions to allow you to edit the entity. That's most commonly an edit form (which allows you to update), but that is not a given.

    The edit operation of an entity may not necessarily check user access to update the entity either. A simple example for that would be an entity that cannot be updated, but instead, the edit operation might result in a new entity being saved/created.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityListController.php
    @@ -113,33 +136,6 @@ protected function getLabel(EntityInterface $entity) {
    -  public function getOperations(EntityInterface $entity) {
    
    @@ -164,31 +160,12 @@ public function buildHeader() {
    +      '#links' => $this->operations->getOperations($entity, $this->currentUser),
    
    +++ b/core/lib/Drupal/Core/Entity/EntityOperationsProvider.php
    @@ -0,0 +1,133 @@
    +  public function getOperations(EntityInterface $entity, AccountInterface $account) {
    

    Could we keep the getOperations($entity) method on the entity controllers to improve DX?

    In addition, can we default the second $account argument to the current user?

  3. +++ b/core/lib/Drupal/Core/Entity/EntityOperationsProvider.php
    @@ -0,0 +1,133 @@
    +  protected function sort(array $operation_a, array $operation_b) {
    

    Sad to see that we're still adding one-off uasort() helper functions for the common use-case of sorting by 1) weight and 2) title.

    It would be great to create a follow-up issue for adding a SortArray::sortByWeightAndTitleElement() method to eliminate these repetitive re-implementations.

  4. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockOperationsProvider.php
    @@ -0,0 +1,31 @@
    +    // The custom block edit path does not contain '/edit'.
    +    $uri = $entity->uri();
    +    $operations['update']['href'] = $uri['path'];
    +    $operations['update']['query']['destination'] = 'admin/structure/block/custom-blocks';
    

    It would be much better if all entity operations were based on routes (route names) instead of paths (including the destination).

    However, it looks like the basic conversion to entity operation providers is heavy and complex enough already, so it probably makes sense to defer the migration to routes to a follow-up issue.

  5. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockTypeOperationsProvider.php
    @@ -0,0 +1,29 @@
    +    // Place the edit operation after the operations added by field_ui.module
    +    // which have the weights 15, 20, 25.
    +    $operations['update']['weight'] = 30;
    

    The delete operation has a default weight of 100. Edit/update has a default weight of 10.

    Having to write out an entire class override like this for the 80% use-case of fieldable entities is cumbersome and unwanted in terms of DX.

    Let's simply change the default weight of the edit/update operation to 50 instead? :)

  6. +++ b/core/modules/config_translation/config_translation.module
    @@ -137,18 +138,28 @@ function config_translation_config_translation_info(&$info) {
    +function config_translation_entity_operations(EntityInterface $entity) {
    ...
    +  $operations['translate'] = array(
    

    Shouldn't this be limited to instanceof ConfigEntityInterface (or similar)?

  7. +++ b/core/modules/config_translation/config_translation.module
    @@ -137,18 +138,28 @@ function config_translation_config_translation_info(&$info) {
    +function config_translation_entity_access(EntityInterface $entity, $operation, AccountInterface $account, $langcode) {
    +  if ($operation == 'translate') {
    +    return $account->hasPermission('translate configuration');
       }
    +  return $entity::DENY;
     }
    

    Perhaps off-topic here:

    It looks and feels weird that an entity access hook has to return :DENY in case the hook doesn't apply to the entity.

    In addition, the code here returns TRUE and FALSE instead of :ALLOW and :DENY ?

  8. +++ b/core/modules/field_ui/field_ui.module
    @@ -92,6 +92,64 @@ function field_ui_permission() {
    +function field_ui_entity_operations(EntityInterface $entity) {
    ...
    +    $operations['manage-fields'] = array(
    ...
    +function field_ui_entity_access(\Drupal\Core\Entity\EntityInterface $entity, $operation, \Drupal\Core\Session\AccountInterface $account, $langcode) {
    ...
    +  if ($operation == 'manage-fields') {
    +    return $account->hasPermission('administer ' . $entity_type . ' fields');
    +  }
    

    Generally speaking, it looks a bit cumbersome to perform the access check for an operation so detached to the declaration of an operation.

    That is essentially the architecture of D7 and below - declarations/definitions, access checks, and other business logic living in detached function callbacks and hooks, typically with a much beloved $op parameter (which dates back even further to D6).

    I did not really understand the concerns that were raised against plugins in the comments above, as they have been too cryptic.

    However, from an architectural standpoint, it clearly looks like we

    1) discover/register available operations (beyond the entity type declaring module)
    2) check whether they apply
    3) check access
    4) map to paths/routes
    5) allow to override particular operations
    ...

    In essence, an operation is an atomic unit supplied by a certain provider, which performs custom business logic.

    I guess I'd compare entity operations to Filter module text filtering plugins — they're abstract, re-usable, partially configurable, involve access restrictions, are provided by many modules, and may follow their own business logic.

    That said, I do not think that usage of a plugin architecture should be a blocker for this issue, but I think we should at least have a follow-up issue to explore the migration (for D8).

Xano’s picture

Could we rename the "update" operation back to "edit"?

The way I see it, operations are abstract constructs that do not really map to CRUD operations.

An "update" operation does not necessarily imply that an entity will actually be updated. Instead, the "edit" operation performs the necessary actions to allow you to edit the entity. That's most commonly an edit form (which allows you to update), but that is not a given.

The edit operation of an entity may not necessarily check user access to update the entity either. A simple example for that would be an entity that cannot be updated, but instead, the edit operation might result in a new entity being saved/created.

The reason for doing this is that operations links need some sort of access control, and we want all entity access control to go through entity types' access controllers so there is a single point of failure, *and* we can use entity access for specifying the routes as well. I have found several places in core already where the current situation causes incoherent access checks, as the checks for the routes and the links are not kept in sync.
If you want to add another operation that is not update, you can either update the access controller or implement hook_entity_access() to take care of the access control.

Could we keep the getOperations($entity) method on the entity controllers to improve DX?

Can you explain what controllers you are referring to, and how that will improve DX?

In addition, can we default the second $account argument to the current user?

There has been discussion (notably with @dawehner) about no longer making the account optional for access checks. That's why it has been removed here as well. I am personally slightly in favor of requiring the account, as it is more explicit rather than telling developers their classes should default.

It would be much better if all entity operations were based on routes (route names) instead of paths (including the destination).

However, it looks like the basic conversion to entity operation providers is heavy and complex enough already, so it probably makes sense to defer the migration to routes to a follow-up issue.

See #2148625: Convert entity operation hrefs to route names and parameters.

Shouldn't this be limited to instanceof ConfigEntityInterface (or similar)?

No, because this is from the config_translation module and it does not apply to all config entities. It is also the straightest conversion from the current code.

It looks and feels weird that an entity access hook has to return :DENY in case the hook doesn't apply to the entity.

Can you explain why this looks weird to you? An explicit return value is required, and since those access constants are strings (this was changed not long ago), we can't just return nothing (DENY used to be NULL).

In addition, the code here returns TRUE and FALSE instead of :ALLOW and :DENY ?

Good catch!

Generally speaking, it looks a bit cumbersome to perform the access check for an operation so detached to the declaration of an operation. [and more about a conversion to plugins]

Ideally I'd like plugins to handle links generation, access control, and route registration. This would be easy for developers, but it would also require significant changes to entity access controllers, whereas the current approach leaves the current code base untouched, except for calls from some entity list controllers (easy change) and one hook change (easy as well). The rest are additions. I explored the possibility of using plugins for D8, but given that we can't make them do route registration and access control as well, their usefulness would be severely limited in D8, and their DX would be much worse than the current solution with providers and hooks, because we'd need many more route definitions.

Xano’s picture

FileSize
689 bytes
132.75 KB

Status: Needs review » Needs work

The last submitted patch, 183: drupal_1839516_183.patch, failed testing.

damiankloip’s picture

+++ b/core/modules/config_translation/config_translation.module
@@ -156,8 +156,8 @@
+  if ($operation == 'translate' && $account->hasPermission('translate configuration')) {
+    return $entity::ALLOW;

If this change was still passing before, we definitely need test coverage for this part. As that would throw an exception!

Xano’s picture

Status: Needs work » Needs review
FileSize
3.72 KB
134.86 KB

hook_entity_access() was not converted to use the access constants yet. Previous patches worked fine, because the constants were still bool|null anyway. I'm fixing this in #2151219: Entity and field access hooks must use the AccessInterface constants.

damiankloip’s picture

+++ b/core/modules/config_translation/config_translation.module
@@ -157,9 +157,9 @@
-    return $entity::ALLOW;
+    return TRUE;
...
-  return $entity::DENY;
+  return NULL;

ok, fair enough. As long as we are happy with this going in with uncaught exceptions being thrown... - This is not the case, mis interpreted the above comments, sorry! The EntityAccessCheck will do return its own access constants.

Opened #2151223: Add a method to SortArray to sort by weight and title; I thought we already had that - clearly not!

Xano’s picture

186: drupal_1839516_186.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 186: drupal_1839516_186.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review

186: drupal_1839516_186.patch queued for re-testing.

Re-testing, because I cannot reproduce any of the failures locally.

tim.plunkett’s picture

Discussed this again in IRC, and I wanted to qualify my earlier comments of support:

so yes. the idea sounds great, still. but the actual benefits are still vaporware, the method names are not intuitive, and it increases the burden for authors of an entity type

At this stage (and even 13 months ago like I said in #4), I think it is too late for this.

-1 from me.

Xano’s picture

the actual benefits are still vaporware

  • There is currently no way to add third party operations to an entity UI, except when abusing hook_entity_operation_alter() for this (which Field UI already does, but shouldn't), and even that is only called in a few places. With all the extensibility Drupal 8 offers, this still doesn't allow developers to easily expose additional features in existing UIs. This patch introduces hook_entity_operations(), which lets any module add new operations for any entity they want.
  • As mentioned before, hook_entity_operation_alter() is only invoked in some places. The fact that there is a 'global' hook which is not used 'globally' is confusing DX. This patch introduces a single base class for operations that invokes all documented hooks by default. It is easily extensible with additional operations, but the hook behavior cannot accidentally be changed by doing so. This makes the extensibility consistent.
  • Views now requires one field handler per operations link, which requires developers of new operations to write yet another plugin just to make the operation show up in the node listing that on their particular site is a view and not the node list controller. This patch introduces a single point to retrieve all available operations for an entity, which will allow us to write a single views handler that can be used to get and render all these operations. This makes adding new operations easier for core and contrib, and UIs more consistent.

the method names are not intuitive

If this is found to be so: method names can be changed. Proposals are welcome.

it increases the burden for authors of an entity type

The default operations provider classes can easily be re-used for any entity type. This patch requires authors of entity types who want to change default operations or add new ones to add an operations provider based on the default base class. This is one new file and a the usual code to define classes on top of defining operations through the list controllers like we do now. The moment the operations must also be exposed to Views, this patch actually saves developers work, because they will not have to write a new field handler plugin for every operation.

tim.plunkett’s picture

This patch introduces hook_entity_operations(), which lets any module add new operations for any entity they want.

Why not just do this and be done?

hook_entity_operation_alter() is only invoked in some places

That sounds like a standalone bug.

will allow us to write a single views handler that can be used to get and render all these operations

This would still be true for any custom operation. The ones done currently have their corresponding handlers. No new controller needed.

joachim’s picture

> it increases the burden for authors of an entity type

That doesn't sound good.

I'm a bit confused about how this all works. I had a look at the hook_entity_operations() that the patch adds, and I see this:

+  $uri = $entity->uri();
+  $operations['translate'] = array(
+    'title' => t('Translate'),
+    'href' => $uri['path'] . '/translate',
+    'weight' => 50,
+  );
+
+  return $operations;

How are operations actually made?

In the D7 contrib module I made for entity operations, each operation was a handler, which took care of everything.

This means that authors of an entity type have to do very little. You just list the operations you want for your entity type, like a shopping list.

tim.plunkett’s picture

Yeah this doesn't actually provide any functionality, or link to it in anyway, or even provide metadata. It just lets you tell it the *list* of operations that are valid for a given entity.

joachim’s picture

Ah ok. In that case I've misunderstood the scope of this issue, or it's changed along the way.

I'd be happy to keep maintaining https://drupal.org/project/entity_operations in D8. That could extend what's being done here to provide operation handlers, if those aren't on the cards for D8 core.

Xano’s picture

How are operations actually made?

Check the EntityType annotation class, and EntityOperationsProviderInterface and the classes that implement it, most notable EntityOperationsProvider and ConfigEntityOperationsProvider.

This would still be true for any custom operation. The ones done currently have their corresponding handlers.

It wouldn't. Like I said, we can write a single handler that is reusable and renders a list of multiple operations that it gets from the operations providers. This was discussed with @damiankloip as a good solution for the requirement of having to write one handler per operation that we have now. It directly decreases developers' workload.

No new controller needed.

I honestly do not get the hesitance for adding a new controller. We need operations outside list controllers as well, so it makes sense to split them off for reusability. Berdir agreed on this. The default classes are easy to use and usable for most, if not all, entity types and only requires developers to add one extra line of annotation *if* they want to show operations in the UI.

Xano’s picture

As this issue is not going anywhere, I opened #2165725: Introduce hook_entity_operation().

jibran’s picture

186: drupal_1839516_186.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 186: drupal_1839516_186.patch, failed testing.

tim.plunkett’s picture

Category: Task » Feature request

#2165725: Introduce hook_entity_operation() is RTBC as a feature request. Does that make this obsolete?

Xano’s picture

It does, but I wanted to keep this open until the other one is committed. I'll close this one as soon as the other one is done.

Xano’s picture

Status: Needs work » Closed (won't fix)

Closing this, because #2165725: Introduce hook_entity_operation() was just committed.