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
- The addition of a new type of entity controller, which will not break anything.
- Introduce
hook_entity_operations()
, which will not break anything. - Replace
hook_entity_operation_alter()
withhook_entity_operations_alter()
to match the newly introduced hook. - 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)
Comment | File | Size | Author |
---|---|---|---|
#186 | drupal_1839516_186.patch | 134.86 KB | Xano |
#186 | interdiff.txt | 3.72 KB | Xano |
Comments
Comment #1
plachYes, we definitely need to address this from a more general point of view.
Comment #2
joachim CreditAttribution: joachim commentedI 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.
Comment #2.0
joachim CreditAttribution: joachim commentedUpdated issue summary.
Comment #3
sunAdded
to the problem space in the summary.
Comment #4
tim.plunkettThis 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.
Comment #5
plachI'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...
Comment #6
catchWe 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.
Comment #7
plachAgreed, I didn't change the priority because we are in better shape with the critical queue right now :)
Comment #8
fubhy CreditAttribution: fubhy commentedI 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.
Comment #9
sunGo 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.
Comment #10
sun@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).
Comment #11
fagoI posted my thoughts about the difference to actions here: http://drupal.org/node/1846172#comment-6758258
Comment #12
fubhy CreditAttribution: fubhy commentedFirst 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:
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.
Comment #15
joachim CreditAttribution: joachim commentedI 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 :/
Comment #16
fagoI 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.
Comment #17
tim.plunkettI just got bit by the difference between user_user_operations() and user_action_info(), I'm now officially +100 on this issue.
Comment #18
mitchell CreditAttribution: mitchell commentedWould 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.
Comment #19
fubhy CreditAttribution: fubhy commentedQuick 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.
Comment #20
Wim LeersI 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.
Comment #21
joachim CreditAttribution: joachim commentedI'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 :)
Comment #22
andypostIt 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)
Comment #23
fubhy CreditAttribution: fubhy commentedOkay, 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.
Comment #23.0
fubhy CreditAttribution: fubhy commentedUpdated issue summary.
Comment #24
Crell CreditAttribution: Crell commentedAny news here? This issue came up in discussion of #1889790: [Meta] Allow modules to register links for menus, breadcrumbs and tabs (if not with hook_menu).
Comment #25
Dave ReidI'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.
Comment #26
andypostThe 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
Comment #27
joachim CreditAttribution: joachim commentedIt 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.
Comment #28
podarok#1743686-74: Condition Plugin System commited, yay )))
Comment #28.0
podarokUpdated issue summary.
Comment #28.1
plachUpdated issue summary.
Comment #29
Gábor HojtsyI'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.
Comment #30
Gábor HojtsyFor 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).
Yes, this works (on my machine). Hope this helps make the case if not already :)
Comment #31
joachim CreditAttribution: joachim commentedEeek! :/
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!)
Comment #32
Gábor HojtsyHere 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.
Horrifying enough? :)
Comment #33
joachim CreditAttribution: joachim commentedI 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.
Comment #34
Gábor Hojtsy@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.
Comment #35
joachim CreditAttribution: joachim commentedYes, 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.
Comment #36
joachim CreditAttribution: joachim commentedI'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().
Comment #37
tim.plunkettAll 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
Comment #38
joachim CreditAttribution: joachim commentedAh 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.
Comment #39
dawehnerThere 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.
Comment #40
EclipseGc CreditAttribution: EclipseGc commentedThere 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
Comment #41
Gábor Hojtsy@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.
Comment #42
fubhy CreditAttribution: fubhy commentedI 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.
Comment #43
joachim CreditAttribution: joachim commented> 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.
Comment #44
EclipseGc CreditAttribution: EclipseGc commentedPerhaps, though I'd be tempted to make operations extend the action instead when I needed that.
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
Comment #45
fubhy CreditAttribution: fubhy commentedI 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.
Comment #46
EclipseGc CreditAttribution: EclipseGc commentedok, I consider that largely "same difference" but fair enough.
Comment #47
joachim CreditAttribution: joachim commented> 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.
Comment #48
tim.plunkettAs 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?
Comment #49
joachim CreditAttribution: joachim commentedIf you mean this bit of that patch:
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?
Comment #50
tim.plunkettWe 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.
Comment #51
Crell CreditAttribution: Crell commentedSee also: #1970360: Entities should define URI templates and standard links
Comment #52
YesCT CreditAttribution: YesCT commentedThis would also be useful in #1952394: Add configuration translation user interface module in core.
Comment #52.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #53
fagodiscussed 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.
Comment #53.0
fagoUpdated issue summary.
Comment #54
dawehnerThe 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.
Comment #55
Gábor HojtsySend 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.
Comment #57
dawehnerAdded 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.
Comment #58
Gábor HojtsyAlter 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).
Comment #59
joachim CreditAttribution: joachim commentedI don't really get this patch -- where are the actual operations?
Comment #60
XanoGoing to add the entity to the alter hook, and hook documentation.
Comment #61
XanoAlright. 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.
Comment #62
XanoAs 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.
Comment #63
XanoAnd now without unnecessarily injected services.
Comment #64
tim.plunkettI'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.
These are gone?
This is actually the plugin.manager.entity service
module_handler service
Comment #65
amateescu CreditAttribution: amateescu commentedAnd how about using 'operation' (singular) instead of 'operations', like we do in the rest of core? :)
Comment #66
XanoPersonally, 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.
Comment #67
tim.plunkettRight, but these operations are physical links that take us to a route, they don't just trigger the operation directly.
Comment #68
XanoI'm not too sure we should tie this API to hyperlinks anyway. Those may only be one use case.
Comment #69
dawehnerOn the longrun the operations api should declare the routes not the other way round.
Comment #71
andypostSuppose patch should care about URI-templates commited in #1970360: Entities should define URI templates and standard links
Comment #72
Gábor HojtsyD8MI is happy with #2004408: Allow modules to alter the result of EntityListController::getOperations so we don't have a pressing need for this.
Comment #73
XanoMoving to Drupal 9 as this breaks BC and it's past API freeze.
Comment #74
plachParts of this can still be implemented as API additions, let's not corner ourselves :)
Comment #75
XanoThe 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?
Comment #76
XanoThis patch is a re-roll of the one from #63, with a few changes:
Comment #78
plachPossibly related issue: #2040379: Define contextual entity operations through annotation; remove the context property from hook_menu items.
Comment #79
XanoComment #80
XanoBerdir on IRC:
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.
Comment #82
XanoAccessibleInterface, 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:
What we can do is check access through entities' access controllers directly and avoid AccessibleInterface::access(). This still requires a hook_entity_access().
Comment #83
XanoSee #2057377: Implement hook_entity_access() and hook_entity_create_access().
Comment #84
damiankloip CreditAttribution: damiankloip commentedComment #85
joachim CreditAttribution: joachim commented> 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.
Comment #86
XanoWe 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).
Comment #87
joachim CreditAttribution: joachim commented> 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.
Comment #88
XanoCore 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.
Comment #89
dawehnerThere 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?
Comment #90
XanoI 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.
Comment #90.0
XanoUpdated issue summary.
Comment #91
XanoI updated the issue summary to clearly communicate that we are working towards adding an entity controller type.
Comment #92
Gábor HojtsyThe plan in the issue summary sounds sane to me.
Comment #93
fagoThe 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()?
Comment #94
XanoMy 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.
Comment #95
tim.plunkettAccessibleInterface 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.
Comment #96
fagoEntity access just builds upon the typed data Accessibleinterface + have the controllers to implement it. It's not like it is a third system?
Comment #97
XanoSee #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.
Comment #98
tim.plunkettThis 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.
Comment #99
Xano#2057377: Implement hook_entity_access() and hook_entity_create_access() is in.
Comment #101
Xano@tim.plunkett and I just had a long and emotional conversation about this:
Comment #102
XanoComment #103
joachim CreditAttribution: joachim commented> 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?
Comment #104
fagoWhat would do the plugin class then? I don't see a need for class here?
Comment #105
XanoAny 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.
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.
Comment #106
joachim CreditAttribution: joachim commentedI'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?
Comment #107
fagoBut 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.
Comment #108
tstoecklerTalked 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.
Comment #109
XanoI 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.
Comment #110
XanoAs #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.
Comment #111
dawehnerShocking people don't respond to all the issues :)
Comment #111.0
dawehnerUpdated the issue summary to reflect the current status.
Comment #112
XanoThis 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.
Comment #113
Crell CreditAttribution: Crell commentedCan 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.
Comment #114
XanoThis 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.
Comment #116
XanoFixed the PHPUnit tests and the DI of ViewListController.
Comment #118
XanoMost 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.
Comment #120
XanoNot all operations providers extended from ConfigEntityOperationsProvider, and I fixed a DI issue with FilterFormatListController.
Comment #122
XanoComment #123
XanoComment #125
Xano123: drupal_1839516_123.patch queued for re-testing.
Comment #126
podarok123: drupal_1839516_123.patch queued for re-testing.
Comment #128
XanoThis should pass.
Comment #129
XanoThere are one or two @todo's left, which I will deal with tomorrow.
Comment #130
XanoI 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, soEntityOperationsProviderInterface::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.
Comment #131
Xano#2129953: Abstract entity status out to an interface will clean up some up the code for to the
enable
anddisable
operations.Comment #134
XanoI wrapped the translation manager in a
t()
method as per timplunkett's recommendation over IRC.Comment #136
klonosComment #137
Xano134: drupal_1839516_134.patch queued for re-testing.
Comment #139
XanoThis should pass.
Comment #141
XanoA man's gotta pull, when a man's gotta pull.
Comment #142
XanoOh, everybody look over there! A pink elephant!
Comment #144
klonos...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! ;)
Comment #145
XanoOooh, shiny. Thanks for letting us know!
Comment #146
Xano142: drupal_1839516_142.patch queued for re-testing.
Comment #148
Xano142: drupal_1839516_142.patch queued for re-testing.
Cannot reproduce test failures locally.
Comment #149
XanoNow 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.
Comment #150
XanoRe-roll because of #1952394: Add configuration translation user interface module in core.
Comment #151
XanoForgot to remove some old code.
Comment #153
XanoComment #154
XanoComment #156
XanoComment #157
XanoTest class properties that contain mocks will need to be documented with an additional
|\PHPUnit_Framework_MockObject_MockObject
.Comment #158
XanoDear d.o, please update the issue summary when I ask you to.
Comment #159
XanoApparently we have issue summary summaries...
Comment #160
XanoComment #161
Xano156: drupal_1839516_156.patch queued for re-testing.
Comment #163
XanoRe-roll, made sure there are no more occurences of operations controller (used operations provider instead), and fixed #157.
Comment #165
XanoComment #166
Xano165: drupal_1839516_165.patch queued for re-testing.
Comment #167
XanoCan 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.
Comment #169
Xano165: drupal_1839516_165.patch queued for re-testing.
Comment #170
andypostWhy interface declares only getOperations() but all over core used getDefaultOperations() ?
Seems this one needs postponed follow-up to convert 'href-path' to routes
Comment #171
XanogetDefaultOperations()
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.Thanks for reminding me to do that!
Comment #172
XanoAdded #2148625: Convert entity operation hrefs to route names and parameters as a postponed follow-up.
Comment #173
XanoRe-roll, because the patch no longer applied to
entity.api.php
. No production code has been changed.Comment #174
damiankloip CreditAttribution: damiankloip commentedWhat's the logic behind calling them Providers, but essentially just being another controller, and being stored like that?
Comment #175
Xano#1976158: Rename entity storage/list/form/render "controllers" to handlers and #113.
Comment #176
andypostI think it's ready, very solid and covered with tests!
Comment #177
tim.plunkettWe 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.
Comment #178
Xano173: drupal_1839516_173.patch queued for re-testing.
Comment #180
XanoRe-roll because of #2109303: Convert CSRF checks in controllers to the routing system, which touched
ViewListController
.Comment #181
sunWow, 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:
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.
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?
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.
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.
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? :)
Shouldn't this be limited to instanceof ConfigEntityInterface (or similar)?
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 ?
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).
Comment #182
XanoThe 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 implementhook_entity_access()
to take care of the access control.Can you explain what controllers you are referring to, and how that will improve DX?
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.
See #2148625: Convert entity operation hrefs to route names and parameters.
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.
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).
Good catch!
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.
Comment #183
XanoComment #185
damiankloip CreditAttribution: damiankloip commentedIf this change was still passing before, we definitely need test coverage for this part. As that would throw an exception!
Comment #186
Xanohook_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.Comment #187
damiankloip CreditAttribution: damiankloip commentedok, 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!
Comment #188
Xano186: drupal_1839516_186.patch queued for re-testing.
Comment #190
Xano186: drupal_1839516_186.patch queued for re-testing.
Re-testing, because I cannot reproduce any of the failures locally.
Comment #191
tim.plunkettDiscussed this again in IRC, and I wanted to qualify my earlier comments of support:
At this stage (and even 13 months ago like I said in #4), I think it is too late for this.
-1 from me.
Comment #192
Xanohook_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 introduceshook_entity_operations()
, which lets any module add new operations for any entity they want.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.If this is found to be so: method names can be changed. Proposals are welcome.
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.
Comment #193
tim.plunkettWhy not just do this and be done?
That sounds like a standalone bug.
This would still be true for any custom operation. The ones done currently have their corresponding handlers. No new controller needed.
Comment #194
joachim CreditAttribution: joachim commented> 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:
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.
Comment #195
tim.plunkettYeah 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.
Comment #196
joachim CreditAttribution: joachim commentedAh 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.
Comment #197
XanoCheck the
EntityType
annotation class, andEntityOperationsProviderInterface
and the classes that implement it, most notableEntityOperationsProvider
andConfigEntityOperationsProvider
.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.
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.
Comment #198
XanoAs this issue is not going anywhere, I opened #2165725: Introduce hook_entity_operation().
Comment #199
jibran186: drupal_1839516_186.patch queued for re-testing.
Comment #201
tim.plunkett#2165725: Introduce hook_entity_operation() is RTBC as a feature request. Does that make this obsolete?
Comment #202
XanoIt 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.
Comment #203
XanoClosing this, because #2165725: Introduce hook_entity_operation() was just committed.