Problem/Motivation
Right now, entity lists are not standardized as a link template. We have an entity_list route thingy, we have a list builder handler, but they are not connnected. Route names are not standardized and there is no link template for it, which means that there is now way to get the list route for an entity type in a generic way.
We have two immediate use cases for this:
* Provide a useful default delete confirmation form that entity types can use without having to add a custom class. The patch over there already removes hundreds of lines of code, this will make it even better.
* The ability to generate the route definition in #2350509: Implement auto-route generation for all core entities and convert all of the core entities. (although we still need solve the problem of the missing plural label)
Proposed resolution
* Add a 'collection' link template. (As defined by http://www.iana.org/assignments/link-relations/link-relations.xml)
* Rename all routes to the now enforced route name pattern: entity.$entity_type.collection
Remaining tasks
None.
User interface changes
None.
API changes
The affected routes are renamed, direct references to them need to be updated. Existing entity types can opt in to this link template, they are not forced to use it.
Beta phase evaluation
Issue category | Task, enables us to make generic forms and route generation code more useful. |
---|---|
Issue priority | Major, because I think this is a missing part in our link template API, and the entity route generation is major as well. |
Disruption | See API changes, contrib modules that reference any of the renamed routes need to be updated. References to those from other modules are fairly uncommon, however. |
Note: Credit @larowlan for his work in #2401793: Rename entity list routes to entity.$entity_type.list to make autogeneration possible..
Comment | File | Size | Author |
---|---|---|---|
#37 | 2401505-37.patch | 186.59 KB | jibran |
Comments
Comment #1
BerdirNo idea if this is enough to define it :)
Comment #2
dawehnerWell, if we do that, we should rename the routes as well.
Comment #3
BerdirWe could do that, and we probably should if we want to be able to generate them, but having a link template means that it doesn't matter how it is named :)
Adding this is an API addition, renaming existing routes an API change, that's why I'm not sure..
Comment #4
dawehnerThis is not TRUE.
Link templates already supposed to be named
entity.$entity_type.$link_template
. Breaking this on purpose is a nogo.Comment #5
dawehnerMeh.
Opened up
#2401793: Rename entity list routes to entity.$entity_type.list to make autogeneration possible.
Comment #6
BerdirOh, I didn't mean this to be done already, just started with one example, want to add it for other entity types as well and probably this needs to be mentioned somewhere in the documentation?
Comment #7
BerdirThese are all that I could find, including a few usages of it.
Doesn't make much of a difference right now, but it will for #1728804: Introduce (Content)EntityDeleteForm and children to handle entity deletions.
Comment #9
BerdirFixing some tests.
Comment #10
amateescu CreditAttribution: amateescu commentedLooking at http://www.iana.org/assignments/link-relations/link-relations.xml, this looks more like a 'collection' link template to me :)
Comment #12
BerdirThat's fine with me as well ;)
Comment #13
larowlan#2100637: REST views: add special handling for collections
Comment #14
BerdirRenamed to collection. No interdiff, as every line in the patch changed ;)
Comment #15
amateescu CreditAttribution: amateescu commentedMissed a few :P
Comment #16
BerdirNoticed that myself and had it already fixed but forgot to re-create the patch...
:P yourself!
Comment #18
dawehnerWe use the same route for a collection as for editing an action? This can't be right
I still highly dislike that we don't adapt the route names if we introduce them here. I regret that we haven't enforced that somehow in our code earlier when we did all the conversions.
Don't we have to cary over the options as well? you never know
Comment #19
Wim Leers+1 for dawehner's feedback, but adding one nuance:
'collection' versus 'list', consistency would be nice.
Comment #20
amateescu CreditAttribution: amateescu commentedSee #10 for why the current patch uses 'collection' for the link template. Are you suggesting that we should rename the entity list handlers at this point? :)
Comment #21
Wim Leers#20: just renaming the routes would already be nice. Renaming entity list handlers is indeed probably not ok at this point :)
Comment #22
BerdirRe-rolled,
# 18
1. Fixed as part of the rebase.
3. Fixed, a redirectUrl() would be neat I guess ;)
2. Well, you won now, as you got the link issue in in time ;)
route names are now "validated", as in they completely break if they do not follow the pattern.
Did not do that yet, means that we have to merge #2401793: Rename entity list routes to entity.$entity_type.list to make autogeneration possible. into this. This will then obviously fail horribly, but wanted to upload it as a starting point.
Comment #23
BerdirForgot the patch. not bothering with the interdiff, the only thing that I didn't do as part of the reroll was the options thing.
Comment #25
BerdirYou leave me no choice ;)
Merged #2401793: Rename entity list routes to entity.$entity_type.list to make autogeneration possible. into this, did my best to revert the search.settings configuration strings, and renamed to collection.
Comment #27
BerdirFixing those test fails. My regex search & replace didn't work on some strings, no idea why and looks like @larowlan missed the node_type route in the other issue :) Also a few other fixes.
Comment #29
dawehnerIt looks pretty good! One small thing I noticed while reading the patch.
Huch, we don't use @url in the string. Not sure whether its in scope of this issue to fix it
Comment #30
BerdirFixed that test fail and removed the strange @url there, nice find. Also means we no longer need urlGenerator in there.
Comment #31
dawehnerGreat work, and sorry for the extra work, which nevertheless would had to be done.
Comment #32
BerdirI'll update the issue summary ASAP.
Comment #33
alexpottThis issue is a major task. To evaluate whether we should make this change during the beta, we need to update the issue summary according to https://www.drupal.org/core/beta-changes to explain what improvements it makes and what the disruption of the change would be. Can someone add Drupal 8 beta phase evaluation template to the issue summary.
Comment #34
BerdirUpdated the issue summary.
Not sure if and what we need in regards to change records.
Crossposted with @alexpott :) Back to you, your decision now.
Comment #35
BerdirComment #37
jibranReroll after #2091321: Update hook_help for Field and Field UI module
Comment #38
fagoThe patch looks great in general, but I'm not sure on how the collection vs list wording is best.
So the entity system goes with "list builder" already, but the link template standard imposes "collection" wording on us. Right now, we take over the collection wording on the route names, which then uses the list builders for implementation.
I'm wondering, whether it would be better to have the link-template collection already implementing to
entity.node_type.list
instead ofentity.node_type.collection
? That way, Drupal internally would use list everywhere, it's just using collection where it's directly going with links (where collection is specified). That seems to be a bit more consistent to me, what do you think?Comment #39
fagoAs berdir pointed out on IRC, entity.node_type.collection is directly connected with the link relation name and cannot easily be separed. As having parity of route names with the link relations is a plus as well, one could say entity.node_type.list is really more consistent than entity.node_type.collection. Given that, I think it's ok to move on with it as it is.
Comment #40
alexpottGiven that we want to auto-generate routes I think we want to proceed with this change.
Committed 3d14a7d and pushed to 8.0.x. Thanks!
Thank you for adding the beta evaluation to the issue summary.
Reverted these changes as they are out-of-scope.
Comment #42
andypostmajor follow-up #2413723: Toolbar icon for "People" item broken