Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Follow-up to #2350503: Add route generation handlers for entities
Problem/Motivation
We now have support for route auto-generation for entities. Let's use it for all entities.
Proposed resolution
Implement any additional route generators we need, and remove all static routes in yml files.
Remaining tasks
Do it.
Also make sure the Entity topic is updated as part of this patch. (@defgroup entity_api in entity.api.php).
User interface changes
None.
API changes
None.
(This issue was approved by catch and alexpott in Amsterdam.)
Comment | File | Size | Author |
---|---|---|---|
#195 | interdiff.txt | 13.3 KB | dawehner |
#195 | 2350509-195.patch | 88.6 KB | dawehner |
#193 | interdiff.txt | 1.49 KB | dawehner |
#193 | 2350509-193.patch | 84.65 KB | dawehner |
#190 | interdiff.txt | 11.2 KB | dawehner |
Comments
Comment #1
Crell CreditAttribution: Crell commentedComment #2
larowlanWe already had an issue with a patch, any reason for new one?
Comment #3
dawehnerI do agree that the other issue had valuable discussions already, but it is quite outdated especially in terms what we plan now.
Comment #4
Wim LeersAFAICT this is blocked on #2281645: Make entity annotations use link templates instead of route names.
Comment #5
Wim LeersComment #6
dawehnerYes it is.
Comment #7
dawehnerUpdate the status.
Comment #8
dawehnerHere is a basic start.
Comment #10
BerdirShouldn't this use the link templates now? And be conditional based on that, e.g. don't generate a canonical route if the entity has no link template
Also thinking about patterns/ways to make it easy to override/customize, I think we talked about having a method per route or so..
Comment #11
dawehnerYou are absolute right!
It would be indeed good to think about it. Do you think it makes it easier requiring a new method compared to altering the collection directly?
Comment #13
tstoecklerI think at least having a
getBasePath()
- which defaults toreturn $entity_type;
would be quite useful. Especially for config entities...Comment #14
Berdir@tstoeckler: #2281645: Make entity annotations use link templates instead of route names landed, so now the link templates need to have the URL anyway, there's not much point in having an admin path or base path then?
Comment #15
tstoecklerOh, yes, sorry that was stupid. I had seen the commit, but it hadn't quite sunk in. Ignore me.
Comment #16
BerdirJust thinking out loud, what about looping over link templates and then have a method name pattern based on that?
My idea for this is that we can easily standardize the route name pattern. Subclasses can then decide if they want to override a method (to e.g. build a specific route completely custom), or they can still override getRoutes() and do whatever they want there.
Also, EntityTestRoutes might be a good example to convert to this instead of a route provider. My goal is still to have most of the entity test stuff being able to rely on default code. No idea why we have a custom controller for add/edit there, for example..
@tsteockler: See also #2401505: Add an entity collection template for lists , that will come pretty close to the idea of a base path for many, but not all entity types. But some are quite different, e.g. the collection url for users is now /admin/people
Comment #17
dawehnerComment #19
dawehnerFixed some failures and converted taxonomy over.
Comment #21
dawehner...
Comment #22
tim.plunkettThis was closed as a dupe of #2089757: Auto generate routing entries for entities based on an 'admin_path' and 'admin_permission' annotation which was major.
I would consider this a critical bug after #2281645: Make entity annotations use link templates instead of route names, but that's just me.
Comment #24
dawehnerCan you clarify why this blocks the release? Do you talk about the CRY argument of requiring the paths in multiple places? I am
not sure wether this is such a valid argument, given that previously it has been two places where the route name had to be entered.
Comment #25
dawehnerFixed some issues and converted the vocabulary entity type.
Comment #27
BerdirNot really correct :)
The canonical route needs to be optional as well, most config entities don't have one I think, they can't be viewed.
This is a bit weird that we have to set it to NULL. Can't we make the thing that converts those routes intelligent enough to not overwrite an existing _controller?
Should we rename the route provider to something like DefaultHtmlRouteProvider?
Also, in #2364157: Replace most existing _url calls with Url objects, I noticed that we have a canonical route in entity_test.routing.yml, but just for this entity type, not generic, no idea way. Anyway, you can remove that one too.
I might have asked this before, but what's the point of this _title, we also have a _title callback.
view controller title callback on the edit form? Should we have a useful default for this as well?
Comment #28
dawehnerTotal valid point.
Does that mean we should that uri relationships just one level up? I still don't understand why we have to special case node here at all.
Sold.
Given that the entity ID is required there is none.
I think the best we should do is to provide a editTitle method and set it by default? This is related to [#2403359]l
It might have been important in earlier times, but I don't think its helpful any longer.
Moved it to EntityController, as a more generic place.
Also converted views, and tried to increase the failure rate again.
Comment #30
dawehnerConverted roles and comment_type as well, and maybe fixed some failures.
Comment #32
dawehnerWhat does the bot say?
Comment #34
Wim LeersBot says no :(
This will then clash with #2403359: Use _title and _title_callback where possible, but that's fine :) It should remove a large, large portion of that patch. I bet many of the test fixes you will need to borrow from there.
.
.
Why not also
addTitle()
orcreateTitle()
?And perhaps renaming
title()
toviewTitle()
would make sense, too?These look strange. What about
or ?Wouldn't
Edit %entity_type %label
be more appropriate?.
s/empty/entity/? :D
Inconsistent whitespace.
I wonder if it'd be better to change those who are using
default
to usingedit
instead?Should be made consistent. Should all mention "HTML routes".
If we'd detect bundle entity types in
DefaultHtmlRouteProvider
, then we could set_admin_route: TRUE
automatically..
Why not change it to use
EntityController::title()
instead? (And then delete the custom title callback.)Why?
Dangling semi-colon.
Also: the only value of the custom controller is to set a bunch of HTML
<HEAD>
links. Why not just do that for all entities automatically? Then the custom code for Nodes can go away. No harm in that.Inconsistent code formatting, compared with the other route providers.
Hm, I thought this would automatically be the default? I.e. I thought it wouldn't be necessary to define it in the annotation if you wanted to use the default HTML routes?
This line can be deleted; the default title is superior. We also don't use the hardcoded title for editing terms anymore; no need to do it for deleting then.
Inconsistent code formatting.
Why keep this?
?
Accidental whitespace.
Comment #35
dawehnerThank you for your long
Mh, IMHO we don't have that link template defined yet, so its a little bit pointless for this issue to add it.
Went with the first one.
Well, this one seemed to be the one which is used most of the time, so I went with it. As we make things more consistent here, we can adapt it later, if needed.
Fixed
There is clearly no actual strategy for existing code, so I would vote to keep it like that an decide in an issue whether the concept of a default form makes sense. I guess it doesn, as
we need a form for the add case as well.
fixed.
I won't do that, because I don't get why we manually add it here. The
AdminRouteSubscriber
should take care of it, if possible. Not sure why its added here,but there is probably a reason.
Well, its tricky as the NodePreviewController uses a different param converter.
Well, we have a custom controller, which I wanna use here (maybe not anymore in the future, but for now the amount of changes should be limited, if the patch should keep reviewable).
Well, I think if we want to change the behaviour, we should do that separate, as it requires test coverage and what not. Its not in scope of this issue IMHO, which is about improving DX
for defining routes.
Well, if you look at ContentEntityType and EntityType you won't see many handlers added by default. I kinda prefer to opt IN the behaviour and not require a way, to opt out of that behaviour. I don't
see a strict requirement why every entity type would require it.
fixed
Oh, I first they do something different, but they do the same, just way more complex.
Alright, let's also fix stuff.
Comment #37
dawehnerAdd tag
Comment #38
Wim LeersFrom 2460 fails to 117! Nice! :)
Oh, ok, never mind then :)
Fair!
I see that angle, but the code introduced here is made more complicated because of that concept. I think we want input from more people about that.
Because:
and the routes in this patch with
_admin_route
set don't have paths that begin with/admin
.Ok. Could you document this? It was very puzzling to me why you'd set something to
NULL
.Hrm, ok. Because it's so tightly related it feels sufficiently in scope, but: fair.
Right; there could be content entities that don't have HTML routes. Makes sense; thanks!
Comment #39
cgalli CreditAttribution: cgalli commentedI have tested this on the Content Entity Example from the examples Module with https://www.drupal.org/node/2409195 applied. Removed view, edit and delete route from the annotation.
The View, Edit and Delete links work perfectly well.
Comment #40
dawehner@cgalli
Thank you for the manual testing. Good to know that it actually works :P
Right, but there are exceptions, for whatever reason( admin/structure/comment/*,admin/structure/block/* )
Sure.
Fixed all failures and later converted all the other entity types.
Comment #42
dawehnerHa, annotations <3
Comment #43
dawehner.
Comment #45
fagoOpened #2409691: EntityAccessControlHandlers miss entity admin access, which this issue can leverage once its there.
Comment #46
mglamanLooks likes tests
failedhad exceptions due to incorrect namespace for FeedHtmlRouteProvider in annotation.Comment #48
dawehnerGood catch @mglaman
For now just a reroll.
Comment #50
dawehnerSome more work, not really happy about some of the required changes.
Comment #52
dawehnerReroll.
Comment #54
tstoecklerThis fixes some fails, hopefully.
Comment #56
jhodgdonSo, I realize that this patch isn't done (tests don't pass), but I'm not convinced this is a step forward.
It looks like most entities covered in the patch are losing a few lines in the routing.yml file, at the expense of having to define their own route provider class and reference in in entity annotation, apparently just to override some defaults that would otherwise just be part of the route definitions. This doesn't seem like great DX. It's easier to provide the defaults in the routing file than to define a class, I think, and it's way more lines being added than lost.
There are a few entities (like Feed in Aggregator module) that also don't seem to have lost anything in the routing.yml file and have only added lines in the annotation and the new class. This may be a problem in the patch, but is definitely not great DX.
And the patch will eventually need to update the Entity topic docs, so that developers know about this. I added a note about this to the issue summary so it doesn't get forgotten.
Comment #57
tstoecklerRe @jhodgdon: The overrides that these entity types are just to be consistent with the current behavior: They will not be necessary for the majority of use-cases. Also the fact that this patch is uncovering all sorts of inconsistencies and weirdnesses between the different entity type is proof IMO that some form of generic implementation is needed or at least very helpful. We can always improve the base implementation in follow-ups so that maybe in the future less follow-ups will be necessary, but this is certainly a big step forward. I've been using this patch on a site I'm developing for a while now and it's a *big* timesaver.
Comment #58
Wim LeersI share some of jhodgdon's DX concerns. Let me repeat a few points that I made in #34:
to which dawehner replied
18.
to which dawehner replied:
Which is a pretty good reason.
Perhaps revisiting those things could help the DX?
Comment #59
dawehnerIf you look at the entity API you will see that its an explicit API, you specify what is done, and don't rely on the default
instead of just one single place.
If you would be extreme, you could say, let's skip whatever core its doing, but for custom entity types you REALLY want to use it, and have a consistent behaviour.
Comment #60
jhodgdonOK, I'll bow to your greater wisdom and tstoeckler's experience on this being better DX for defining your own entity type.
Regarding the complaint about having to specify using the default route builder, I think that in most other areas of the Entity API, such as the Access controller, we've made it so if you're using the default class, you don't have to specify it. So I think that should be the default here too? But that is not a huge deal, as it's only one line in the annotation.
Also, one note about the "admin_route" designation of routes... It seems to me that the default controller should and could choose more sensible defaults. For instance, it could designate either all routes, or just edit/add/delete routes (omitting "canonical"), for Config entities as admin_route. Alternatively, Core could provide two default classes, the first as it is now, and the second having this admin_route default set, so that entities could choose one or the other as their route provider without the necessity of defining their own class, and we wouldn't have to have a bunch of little classes floating around whose sole purpose is to set the admin_route flag on their routes. That would cut the size of this patch down considerably -- there are at least 4 classes in there whose sole purpose is to set admin_route defaults I think.
Another reason entities in this patch seem to be overriding the default class is to specify the title, but I think these overrides can actually be omitted. Most controllers for these things override the title for the page anyway, so the title in the current routing files is rarely if ever even used.
Anyway... just a couple of thoughts on how to possibly improve the DX in this patch.
Comment #61
Wim Leers+1
+1
Comment #62
dawehnerLet's try to be friends :)
That sounds like a good thing to experiment with!
Well, we do it for 2 cases: a) the storage controller and b) the access control handler ... For everything else we are explicit. I think one of the entity subsystem group should
tell us their opinion, given that they actually know the intended behavior of those annotations.
Note: we need the title on the routes for breadcrumbs.
Its good that you care about it?
Comment #63
jhodgdonRE: friends, of course! I really was not trying to be snarky, and I apologize if it came across that way. I see the wisdom of what you said and agree with it.
RE: route titles and breadcrumbs -- oh interesting. I hadn't thought of that. I guess that is important only for routes that are the base for local task/action groups then?
Comment #64
Berdir+1 for making it explicit, that you have to specify it. That was the condition that we agreed on in Amsterdam to allow introducing this so late, that it is an opt-in mechanism. Otherwise it is going to mess up existing modules.
As @dawehner mentioned, we only do it for just storage, which just works for all entities, and entity access, which doesn't do anything else but checking the admin permission as defined. This is too experimental to enable it by default, at least for a first version.
Also, as @dawehner and @tstoeckler mentioned, almost all our entity types have some special cases which is why the defaults don't work for them and they do need overrides.
I'm not sure if Configure is really something that we need to keep, but I wouldn't be surprised if we do have a test for this somewhere?
Agreed that we could consider to set _admin_route to TRUE by default for edit/delete, this for example does not set it for delete, which is very likely an oversight, possibly in HEAD as well. _admin_route TRUE for edit/delete seems like a good default. That said, that just means that node will have to override more, as that part is configurable there, because nodes like to be special.
I don't really see why this should be necessary though, those are /admin paths anyway? All config entities should be But setting it by default won't hurt either.
ups.
Looks like you found that problem here as well with the wrong prefix, that was afaik just fixed in HEAD.
Why is this suddenly necessary? entity_test route changes?
save is not necessary, drupalCreateUser() saves the user already.
Comment #65
tstoecklerI noticed some more routes and other things that can be removed. Did not tackle the tests yet.
Because Wim Leers' happiness is of ample importance to me, I wondered if it would make sense to introduce a
HtmlRouteProviderThatUsesTheAdminRouteOptionForEditAndDeleteFormRoutesByDefault
in order to consolidate the different route providers that set_admin_route
toTRUE
. (I think that is along the lines of what you proposed above, right?) It turns out, though, that each of those (but for one) also does something special like setting a different title or something. So it wouldn't actually be a net reduction of code (although it might be helpful for contrib?).Edit: The paragraph above is bollocks (except, of course, the first subclause <3 :-)), sorry. New patch coming up.
Comment #66
tstoecklerOk, here we go.
I just now saw #64, so this includes the fixes for this as well. I did not fix 1., though. I think when this is green we can see what fails if we remove the FeedHtmlRouteProvider, and then evaluate.
So this one - I think - will make @Wim Leers happy :-)
It introduces an
AdminHtmlRouteProvide
which is used directly byBlockContent
andTerm
and extended byFeed
andUser
. I think that is more explicit than simply making it the default. Then we would have to disable it for e.g. comments as well.Berdir was right and
CommentType
in fact does not need the override at all.Comment #67
dawehner...
Comment #69
tstoecklerWell, that was not too smart.
This installs at least.
Comment #71
Wim LeersNow postponed #2403359: Use _title and _title_callback where possible on this.
Comment #72
dawehnerJust a reroll for now.
Comment #74
dawehnerSome fixes here and there.
Comment #76
dawehnerMore fixes.
Comment #78
BerdirReroll.
Comment #80
Crell CreditAttribution: Crell at Palantir.net commentedOverall I like the direction this is going. Some comments:
In what circumstances would the entity not already be loaded in the parameter bag and therefore available for the reflection-based parameter passing? That seems like a bug; if there's a legit non-bug reason for it, it should be documented.
Let's take each of these blocks and turn it into a method call for better SRP. Ie:
That way getRoutes() becomes easier to read, and each discrete task is in its own (reusable!) method.
Additionally, if entity.entitytype.canonical is manually defined via YAML already we want to skip this entirely, IIRC. Is that check already happening somewhere else, and if not, how do we make it happen?
I think there's an extra k here. :-)
Ideally in a follow-up we can eliminate this exception. It's just there for links, which ought to be automated anyway.
This seems unrelated? (I'm not against it, jut just seems unrelated.)
Can't this be automated? We automate most entity permissions already, I believe...
This might be scope creep, but shouldn't we keep the route names/behavior as similar as possible? Account cancelation may involve deletion, so should it reuse the same route name for consistency, even if we change some of the parameters on it?
My bigger issue, though, is the heavy use of inheritance. Default->Admin->Specific seems like a bad way to do it, since there's no is-a relationship. What we actually want here is a way to mix and match common auto-routes.
Instead, perhaps we could have a CommonRoutesTrait and AdminRoutesTrait. Then a DefaultRouteHandler could use both of those, and its getRoutes method just calls the methods out of those traits and returns. (As noted above, that should be a lot of little methods rather than 1-2 big ones.) Then instead of extending and extending, each entity-specific implementation could use those same traits and call the methods it wants. If it doesn't have anything custom to do, it can just use Default which is fine. That allows, say, admin routes without the default routes. (Since "has admin routes" is not a strict special-case of "has the common routes", conceptually.)
It might be slightly more verbose, depending on the details, but I think it will be a clearer separation of concerns and give developers both more flexibility and better education about what's going on. Layers of base classes can be rather opaque in that regard.
Comment #83
Crell CreditAttribution: Crell at Palantir.net commentedWould it get this issue moving if we didn't do all the conversions at once? Just get the mechanism in place (and therefore available to contrib) and do the easy conversions. If some are tricky, we can punt on those or even leave them as manual; the way this is setup manual still works and will continue to work, by design, so we don't have to do it all at once.
But this is an important DX improvement for both entity devs and RESTful devs.
Comment #84
dawehnerI would have loved to do it that way, but I'm not sure who, but I remember there was a strong push forward to do them all ...
Comment #86
dawehnerYeah I don't remember at all, but it was kinda needed for some failures, this is what I remember.
Solved that.
Do you mind adding this into a follow up? We aren't doing that yet.
Alright, let's try to drop and see what fails.
OUT OF SCOPE, and no, this is like asking for automating the path for a canonical link template.
No, user deletion is NOT user cancelation, see https://www.drupal.org/node/8
Seriously, this is what you worry about? Seen worse things in my life and IMHO inheritance makes sense. Well, default should extend from a base, but then IMHO it totally makes sense
I think using traits would make it harder to read, because you'd still have a admin one, which calls out to the trait methods, which then do pretty much nothing. Feel free to rewrite the code though, but I think using traits would make it actually harder in terms of DX, which is IMHO the point of this issue.
Comment #87
dawehnerForgot to add one file.
Comment #89
tim.plunkett$revised_block->urlInfo()
Why use this approach for some and the other for others?
Comment #91
lokapujyaWhat should this comment really say? Provides the default admin routes for config entities?
Comment #92
dawehnerGood catch!!
I went with a single overridden method on the user case, because its extending the list in multiple other ways at the same time.
Feel free to disagree.
Ha, improved the comment.
Comment #94
dawehnerThere we go.
Comment #96
dawehnerMaybe now ...
Comment #97
willzyx CreditAttribution: willzyx commentedmanually tested shortcuts after #96 and this is the result for canonical route (empty page)
patch in #96 passes because it simply bypass checks in ShortcutLinksTest
looking at shortcut.routing.yml
entity.shortcut.canonical coincides with entity.shortcut.edit_form so probably we should create a dedicated class to handle it
Comment #98
willzyx CreditAttribution: willzyx commentedreverted changes in ShortcutLinksTest and added a dedicated class for shortcut
Comment #99
willzyx CreditAttribution: willzyx commentedComment #100
dawehner@willzyx
Well, I think shortcuts are pretty stupid, in the way how the configured routes look like,
so I'm not sure whether its really the best idea to let the canonical route point to the edit route as well, but sure, feel free to do this here.
So now we need some proper reviews ... its green!
Comment #101
Crell CreditAttribution: Crell at Palantir.net commentedI quite like this. However, in the past our "very standard" controllers were registered as services to make them a little cleaner. Should we do that here as well, rather than using ContainerInjectionInterface? (Basically no perf diff, but it means the route is a little simpler as it just needs a service name, not a full class name.)
(Need to come back and finish review later, sorry.)
Comment #102
dawehnerping crell!
Comment #103
andypost+1 rtbc, looks solid
NW for:
1) needs consistency in having of entity route providers - admin_route is not a default
2) change record (
NodeRouteProvider
renamed)Probably follow-up for entity translation routes
Also we need to link here #2407505: [meta] Finalize the menu links (and other user-entered paths) system
CommentHtml should be inherited from Admin provider
Same for comment type, menu link, node - we have mostly all edit operations in "admin" so maybe add "_admin_route: TRUE" in default?
Comment #104
dawehnerGood point!
I'm sorry but I don't think that this is an API.
Comment #105
Crell CreditAttribution: Crell as a volunteer commentedAs andypost noted, it seems like everything ought to be extending from AdminHtmlRouteProvider now. Which suggests that we shouldn't even have it. Just fold the _admin_route logic into the DefaultHtmlRouteProvider. That would also nicely sidestep my concerns about too much inheritance, because... we won't be inheriting as much. Problem solved. :-)
Other than that, a few minor nitpicks as noted below. Once those are addressed and AdminHtmlRouteProvider is folded into the parent, I think this is RTBC. dawehner, if you'd do the honors I'll try to not take so long to get back to you this time. :-(
I also agree there's no change record here. These are not API classes.
The outer-most parentheses here seem extraneous. Left-over from trying to chain everything, maybe?
(Same in other methods.)
"Use the edit form handler *if available*, otherwise default."
Hm. Something else I just realized. "route provider" is already the name of a class, and a rather important one. Using this key here is self-descriptive, but also likely to be confusing. :-(
Unfortunately, the key there and the class names are not introduced in this patch, so it may be too late to change it. Blast!
"Vocabularies use the *title* ..."
This ought to have a link defined in the annotation to piggy back off of, no? That would be consistent with all of the other routes. That we then generate the route for it custom here doesn't change that.
Comment #106
tstoecklerI don't think it makes sense for the admin one to be the default as then we would have to undo the _admin_route option for a bunch of entity types. I think having to undo a previous implementation is a sign of over-generalization and bad design.
I am also not a fan of huge inheritance chain, but IMO it's not fair to complain about that here when we have enormous inheritance chains all over core (Typed Data, anyone?). If we want to avoid that, let's do the following: Let's add a
getRouteOptions()
(or similar) method which is returns an empty array by default and then let's add a trait which implements this method and returns['_admin_route' => TRUE]
.Comment #107
Crell CreditAttribution: Crell as a volunteer commented@tstoeckler: Don't get me started on TypedData's inheritance tree... But just because one subsystem abused the extends keyword doesn't mean we have carte blanche to do it elsewhere.
What bunch of entities do we not want the edit-et-al pages to be admin-marked? Do you disagree with andypost that most if not all of them are/should be?
I don't follow your trait suggestion. Can you provide a brief code snippet to show what you mean?
Comment #108
willzyx CreditAttribution: willzyx commentedthe patch needs a reroll
Comment #109
willzyx CreditAttribution: willzyx commentedComment #110
dawehnerLet's stop talking BS and look at actual data:
The following ones aren't using the AdminHtmlRoutesProvider (for example extend the Html one because of other reasons):
The following are:
I think given that the default should not be the admin one, node and comment aren't a special case, if you ask me.
Comment #111
Crell CreditAttribution: Crell at Palantir.net commentedEh, let's move forward here with the admin routes as is. The other items from #105 still need to be addressed, though. (No interdiff on #109 so I am assuming it was a straight reroll.)
Comment #112
dawehnerOH yeah this is annoying.
Well, the FQCN for itself should describe things. We should have put the route provider into the namespace ...
Not sure what to respond, honestly.
Comment #113
Crell CreditAttribution: Crell as a volunteer commentedFor the last point, I mean that we should declare a "cancel" link relation on User, give it a URI in the annotation, and then use that data to generate the route just like we do for all of the other routes.
Comment #114
dawehnerSounds quite out of scope of this issue!
Comment #115
Crell CreditAttribution: Crell as a volunteer commentedNo it's not. Doing it right should never be out of scope.
Comment #116
dawehnerHa, I think given that we actually have a link relation already, not using it, is actually wrong :)
Comment #117
bojanz CreditAttribution: bojanz at Centarro commentedAs someone making a lot of entity types in contrib right now, I'm loving this patch :)
I'm also loving the consistency it adds to the core entity routes. The current _admin_path approach for edit/delete makes sense.
It's a bit weird that the $_entity parameter has an underscore in front, telling me it's somehow special, but no comment explaining why.
This will probably be triggered in Commerce, since we've opted out of the magical automatic entity upcasting.
Would it make more sense to take the last entity parameter instead of the first?
Sometimes you nest UIs, in which case taking the last parameter would cover more use cases.
Why do this? A comment might help.
Docs nitpicking:
Odd comment for a single class. Perhaps simply "Provides a generic entity controller."? Not sure if we want to focus on the title aspect since the class might grow in the future.
...with a title callback?
Don't override existing routes? Or already defined routes?
Comment #118
dawehnerThank you @bojanz!!
Nothing introduced in that patch, but sure, let's add some documentation
Well, I think because IMHO its more likely to do so. But honestly, I don't give a shit that you are doing things the wrong way.
All you need is to add the entity interface onto your controller, its not that hard.
Sure, let's do that.
Went with the first one.
Comment #119
Crell CreditAttribution: Crell at Palantir.net commentedTwi minor nitpicks, then we're done.
Remove the "a". Doesn't parse in English.
The first comma should be a semi-colon or period.
Comment #120
jhodgdonOne other thing... I don't have time to look at this in more detail right now, but there is no update to a .api.php file in this patch. I think that some Entity topic on api.drupal.org most likely needs an update about this change, and this should be part of the patch?
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Entity!entity.api...
It seems like probably several parts of this topic need to be updated.
Comment #121
kgoel CreditAttribution: kgoel at Forum One commentedreroll first
Comment #122
kgoel CreditAttribution: kgoel at Forum One commentedUgh, I feel like an idiot and I don't understand why patch in #121has INSTALL.txt, MAINTAINERS.txt, composer and so on. Patch file itself look fine on my local and don't have the above files. I was going to continue work on addressing concerns from #119 and #120but I think I need to wait until I or someone else can figure out what's going on.
Comment #123
kgoel CreditAttribution: kgoel at Forum One commentedanother reroll. let see.....
Comment #127
lokapujyaI was working on this issue, but didn't mean to post this here. Please ignore this patch.
Comment #128
lokapujyaComment #129
lokapujyaHere's a reroll of #118, with the #119 items.
If #127 passes, it might be the solution to some remaining test fails.Comment #132
lokapujyaTry this.
Comment #134
lokapujyaRevert the route change.
Comment #137
Crell CreditAttribution: Crell at Palantir.net commentedWith no interdiff in #123 or #127 I'm not clear what is wrong here. All that was left is docfixes, right?
Comment #138
lokapujya123 was a reroll.
127 can be ignored.
Some changes in head regarding caching caused some test fails. There were 3 in #129. Now there are 2.
Todo:
1.
I think we need to add 'languages:language_content' here.
2. For some reason, the MenuLinkContentDeleteForm does not have "/edit" on the Cancel Link (which it should according to the test.)
Comment #139
lokapujyaTry to get the cache contexts right.
Comment #141
jhodgdonI looked through the latest patch and I have some comments:
Still missing a docs update... Adding tag. Needs to have the Entity topic on api.drupal.org updated.
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Entity!entity.api...
Does this provide controllers or routes? I think it provides controllers, so don't document that it provides routes, right?
Looking at the code for this method, I think the @return could use some more documentation about what it is returning. It seems to be returning either the entity in $_entity, or any other entity object that is found anywhere in the route parameters. Right?
It might be helpful in this class's doc block to explain how this differs from its parent class, because I had to read the code to figure it out.
I think I'd add a line saying something like:
These routes are the ones provided by \Drupal\Core\entity\Routing\DefaultHtmlRouteProvider, but with the _admin_route route option set for the edit and delete routes.
This class is just providing routes, not title callbacks, right?
Oh... I see. The route definition has a title callback.
How about "including a" instead of "with" here? And it also provides an access callback...
So maybe this could be changed to say:
This class provides the following routes for entities, each with title and access callbacks defined:
- canonical
- edit_form
- delete_form
(I think it would be clearer to use the actual machine names of the routes, like canonical rather than 'view'.)
See, this definitely needs to be documented in the Entity topic.
Is this introducing a new path for custom blocks? It looks like previously the canonical and edit routes both went to block/ID, and now "canonical" is some kind of a view route at that URL, while eidt is now at block/ID/edit? This doesn't seem correct to me.
Why were these two lines removed from the test?
Same question here as on the Block Content entity: you're introducing a new URL here? Seems wrong.
As another note, the code for generating these routes seems to have logic in it that says "Generate canonical if it exists", so maybe you don't need "canonical" at all? In any case I don't think it makes sense to view a block or menu link entity on a page?
This URL is also changing???
Maybe you could just remove the entity.node.canonical entry from the collection instead of being so sad here? :)
Another introduction of a new URL here?
So you've removed this test... are we not needing to test this functionality any more, or could you just not get it to pass?
See, here you've set the canonical and edit-form routes to go to the same URL. If the canonical route is absolutely necessary to have defined (and that needs to be documented!), can't we also use the same URL as we had before in the block and menu link entity classes, rather than introducing new URLs?
It actually scares me that this kind of change might have been necessary to get the test to pass. Did the URL change or not? I would prefer the test to be testing that the URL is what is expected, rather than calling the urlInfo() function to get the URL, because probably as a regression test, something that introduces a change in URLs ought to be caught as a test failure, right?
This was also done in some of the other tests above. Really I think it's better for the tests to have a hard-wired drupalGet() URL so that we verify the URL is what we think it is.
Comment #142
lokapujyaReverting the cache context changes that I made. There is just something wrong with the MenuLinkContent conversion.
1. Got the documentation started.
2. We already know it's a controller by the class name, so how about just "provides: A title callback". Updated in patch.
3. How about: "The entity from the controller or the first entity found in the route." Although I'm not really sure "from the controller" is right; It's the entity passed to the route generator, but I'm not completely sure where the passed entity comes from.
4. Updated.
5. Agree that the rewording is more concise, but actually think view, edit, delete is more readable for a comment.
6. Duplicate of #1.
7, 9, 10, 11, 12,14: All related. I assumed that it was some standardization the the edit-form path should be /edit. But, now don't see any consistent pattern. Needs more discussion.
8. I assumed that it wasn't needed anymore. Adding that piece back (add the cachecontexts assert after it that I deleted in the reroll) to see what happen.
Results:
Get an error in the MenuLinkContent conversion:
Canonical route used to be: /admin/structure/menu/item/1/edit
With the patch it is: admin/structure/menu/item/1 (without the /edit) which gets Access Denied.
13. This isn't a test, but just code in a test module that isn't needed after the conversion to use the routeProvider.
15. Maybe also part of a conversion.
Comment #144
jhodgdonLooks like the tests do not pass... will wait to review most of the patch until they do.
Regarding the docs, This is the only change I'm seeing to an api.php file:
Some questions/comments:
a) Is this auto-route generation really optional?
b) This topic needs more updates:
- In the "Defining" section, it talks about needing to define routes, which needs some modification.
- The "Entity Routes" section also needs some modification
I think both of these should mention that in place of defining your own routes, you can use a route provider.
c) If this auto-route generation *isn't* optional... or even if it is optional... I am failing to see why someone would want to do this (define a class) in place of just defining the routes in a routing.yml file? The routing.yml file seems much easier. ??!?
Comment #145
dawehnerIMHO you really don't have to.
Yes its opt in, as most every other handler in there, like the views data.
Well, the point of this issue is in the title. You want to not have to care about it ... It is quite an effort to get view + edit + delete + access + etc. right, the automation is an advantage.
Comment #146
jhodgdonOK, fair enough. So #144(b) still applies - we need to mention this in two other places in the docs in the entity.api.php file.
Anyway, I took a look at the code as well, and it looks like my previous review comments were not addressed. They still exist. Here are a few; I gave up about half way through the patch because I think there are some real problems that have been overlooked.
This isn't really a generic entity controller and I'm a bit confused about the name... it really is just something you could use to get these title callbacks, but it doesn't do any route controller stuff by itself.
Maybe it should be called EntityTitleController and the first line should say something like:
Provides title callbacks for entity routes.
or something like that?
This method could use a bit more detail... maybe something like:
Returns the entity if it is either passed in directly in $_entity, or if there is an entity that is one of the parameters of the route.
or something like this? I had to read the code to figure out what this does.
Um... it's only the delete and edit routes that will use admin, not canonical/view routes, right? So please fix the docs.
Can we please call these what they are?
canonical
edit_form or edit-form
delete_form or delete-form
There is no such thing as a "view" route...
Actually here I think I wouldn't have just said "provides typical routes" because you can always define your own route handler to define your own, and I'd also mention the admin class.
This just needs a bit more detail.
I mentioned this in a previous review, but I don't think it's OK really to have a new URL here that didn't exist before. Previously, canonical and edit-form were the same URL path. Now you have two, but really there shouldn't be a "view" path for BlockContent entities. This is just wrong.
Again, in tests I think calling the urlInfo() method is wrong.
You want to verify in a test like this that if you go to a particular URL that is defined in the test as a literal string that it will work.
If you write the test like this you are not testing for regressions like what is introduced by the problem above, where the URL changed and it shouldn't have.
Really, when editing CommentType entities, you don't want to be on Admin?
Again I would avoid using urlInfo() in tests like this. Probably the URL changed here and the test failed and it would have uncovered a problem in the code, so instead someone changed the test. The test failure was real...
Again shouldn't this be using the admin route provider?
Again you've introduced a new URL path that should not exist. There should not be a way to view menu links on their own URL.
Comment #147
lokapujya#146.11. This also leads to the following failure with the code that jhodgdon mentioned was removed in #141.8:
The base test for translations is testing to see if the canonical link (default argument for urlinfo() ) is valid. But, MenuLinkContent doesn't have a view controller.
Comment #148
tstoecklerRe #146 8. and 10.: Those entities (and others) are completely managed on URLs under /admin, so they automatically get the admin theme. But we should not set _admin_route explicitly, that's why the default route provider is used
Comment #149
jhodgdonAh, OK. Maybe we should add a comment to the Admin route provider class that says this is only needed for entities whose paths are outside of /admin then?
Comment #150
dawehnerI don't get why this was assigned to me all the time.
Comment #151
tstoecklerWill take a stab at fixing the test fails. Not yet addressing any comments since #142.
Comment #152
dawehnertstoeckler++
Comment #153
tstoecklerSo the test fails were again related to the problem of content entities without a
canonical
link relation. I think I've found a fairly generic solution, which should work in most cases. InContentTranslationUiTestBase
we had an explicit reference tocanonical
, so I implemented a little workaround for that.Let's see what I broke with this.
Comment #154
andypostYep, looks this "missing canonical link" becomes noisy
Comment #156
tstoecklerHmmm.. actually now that I think of it, it should be possible to put the
EntityEditUrlTrait
stuff in a followup. That should make the tests pass.Will try to tackle that but not working on it right now so unassigning.
Comment #157
lokapujyaSo, if we do the EntityEditUrlTrait as a follow up: For now, we could extend DefaultHtmlRouteProvider for MenuLinkContent and copy the editroute code from DefaultHtmlRouteProvider into the canonical route or just not convert it yet?
Comment #158
lokapujyaStart on #146 1-5.
Assumes that we are doing the EntityEditUrlTrait as a followup. Extended DefaultHtmlRouteProvider for MenuLinkContent to try to fix remaining failures.
Comment #159
Crell CreditAttribution: Crell at Palantir.net commentedYay, back to green!
Mostly this looks good to me, with a few caveats noted below:
Does changing the machine name of the link have any upgrade path implications? I honestly don't know, but we should verify.
As I believe jhodgdon mentioned, it's debatable if using urlInfo() in a test is ideal. That means we lose the ability of the test to catch if the URL changes without us knowing. (This affects a large number of tests.)
Comment #160
jhodgdon+1000 for fixing #159 item 2 in all the tests. Remove all those changes from the patch and leave the URLs hard-wired as they were. That way, if we change URLs, we will consciously have to change the test and it's way more obvious. After release, changing URLs is bad, and our regression tests should detect this. It's part of why we have tests.
Regarding #159 item 1, changing route names does definitely have contrib code implications, because contrib modules might have defined local tasks/actions or route alters etc. based on route names. I don't think they require a hook_update_N(), because none of these things is config, but they do probably require a change notice.
And the issues I identified before are STILL there. This patch is not OK. We cannot be adding and changing URLs, and if this patch for some reason requires it, then it's not an OK way to fix things.
I read through about half the patch and gave up as in the last time because this stuff is still broken... Some comments:
I still do not understand why "generic entity controllers" and "EntityController" are good documentation/name for this class.
It's only providing title callbacks. How is this a "controller" or moreover a "generic entity controller"?
The edit page title is "Edit (label of entity)".
Why is the delete title "Delete (label of entity type)" and not "Delete (entity)" also?
This is good. I think it should go several lines up though, right after where it talks about the routing, not after it gives the names of the links:
nitpick: goes over 80 characters
I would reword this as:
Instead of putting the routes for your entity in a *.routing.yml file, you can instead use a route provider class. [sentence about Default and Admin ones]. You can also create your own class. To use a route provider, add the following to your entity annotation:
Again, this change looks wrong. The URLs should NOT BE CHANGING in this patch. Right?
This route/path for canonical is also wrong. Still. It should not exist.
This change also needs to go away. Still. Wrong.
I don't get why the links in the test entity classes need to change either. I would be MUCH more confident in this patch if nothing had to change in links. The fact that things are changing and that the tests are changing to use urlInfo() makes me distrust this whole effort.
Comment #161
Crell CreditAttribution: Crell at Palantir.net commentedI agree with switching the tests back to using hard-coded URLs for the reasons mentioned. However, I don't think that the fact a URL changes is itself a problem if the UI isn't changing. It means that the annotation and the existing routes were out of sync; moving the annotation to line up with the YAML routes, and then removing the YAML routes, is exactly what we should be doing, IMO.
Re comment 2: I agree, let's change that.
Re comment 7: Why shouldn't a canonical route exist?
Comment #162
dawehnerI'm sorry but I strongly disagree with this. It is a bug if you use the same URL for both the canonical representation and the edit form, so IMHO changing them is the absolute right thing to do!
Comment #163
jhodgdonUm, so what is the "canonical" URL for a menu UI item? They don't have a page where you can view them by themselves -- they shouldn't. There should only be edit/delete pages.
Comment #164
lokapujya#160
Hard coded URLS: It's easier to develop with the tests with the dynamic urlinfo() while things are in flux. It's convenient to not have to know the path. That should be the last thing to change after figuring out if the changed paths (#160-6,8,9) are correct. Although, I don't think it adds much to convert the urlinfo()s back: you could just as easily have the hardcoded url wrong.
#160.7 Why ARE we adding the canonical link? Is it for REST?
Comment #165
lokapujyaOops, somehow the old patch got attached.
Comment #166
lokapujyaComment #167
jhodgdonOK. So looking at the latest patch... let's take a look at the URLs that are changing or being added. And a few documentation things....
I know I've put this in about 5 reviews already...
But can the first line docs and probably the name of this class change? It is not a "generic entity controller". To me when I read "generic entity controller" I would think it would be a controller class (i.e. something ready to respond to routes by providing pages). But it isn't -- it is just a class that provides title callbacks for entity routes.
Let's make sure the documentation and the class name are appropriate for that.
This is nice! :)
This is good too.
I'm going to attach a suggested different patch for this documentation in entity.api.php. In a new comment though. This documentation needs some work.
Here's an entity whose URLs changed: BlockContent (custom block entities).
Previously, there were URLs:
- block/ID -- which was the canonical and edit route
- block/ID/delete
Now there are these URLs:
- block/ID - canonical
- block/ID/edit - edit
What happens if you actually go to the "canonical" route? I don't think there is a view controller for viewing a block on its own page. It doesn't make sense to have one. And there shouldn't be two URLs that both do the same thing either, if both block/ID and block/ID/edit are used for editing. There should just be one URL for editing and one for deleting, and nothing else. Right?
So if we cannot have two different links (canonical and edit-form) having the same URL template any more (why not? it apparently was OK before), then ... we probably need to get rid of one of them.
It just doesn't make sense for block-content items to have a full-page view and as far as I know there is no controller set up to view them on their own page.
So this change needs a review.
The menu link entity is another case exactly like the block content case. See above.
This documentation seems to have been copy/pasted from the Shortcut entity? Needs a quick edit.
Shortcut entity is a 3rd case like the other two above.
So here's a different case, the Taxonomy Vocabulary.
In this case, canonical has been added, but it uses the same URL as edit-form. Somehow this has been made to work.
My suggestion is that the same thing should be made to work for the other 3 cases above, as they were before -- they all before had canonical at the same URL as edit, and they all still should I believe?
Comment #168
jhodgdonHere is my suggested edit to the entity.api.php documentation, in the form of an interdiff from the latest patch.
Comment #169
dawehnerIts just so wrong. If you ever want to expose them via REST the canonical URl should be different from the HTML edit one.
Comment #170
jhodgdonHm. So you think that the following entities *should* have "view" URLs?
- Custom blocks
- Menu links
- Shortcuts
- Vocabularies
rather than just edit/link URLs?
I guess that is OK, ... Let's look at what happens with custom blocks with the current patch, in the admin UI. I tried this out on simplytest.me with a standard profile install:
- Went to the custom block library page (admin/structure/block/block-content)
- Clicked on Add a block (block/add ). Created. Saved. Back to library page.
- Clicked on the Edit link in the custom block library page. This takes me to block/1/edit
- Tried out the block/1 link (without the edit suffix). This takes me to a page where the text of the block is the main body of the page.
Without this patch, if you do the same things in the UI:
- The Edit link is just block/1
- block/1/edit gives me a 404
So I guess the question is whether this is what is desired or not. I guess you could say that the patch is OK, although I am not sure it makes a lot of sense to view a block at a page URL. But, not horrible. At least it seems to work.
However, the same does NOT work with menu links. Although this patch defines a "canonical" link, the URL gives me a Not Found. The patch has:
But with this patch, only the URL
admin/structure/menu/item/1/edit
works. The URL
admin/structure/menu/item/1
gives me a 404.
I also tested shortcuts. The patch has:
In the UI, if you click edit you get:
admin/config/user-interface/shortcut/link/1
This works kind of -- has no page title, but it takes you to an Edit page. The URL
admin/config/user-interface/shortcut/link/1/edit
works better, and has a page title.
So, this patch still needs some work. I'm still not sure whether it makes sense or not to have "view this in a page" URLs for these things. But if it does, they need to actually work.
Comment #171
tstoecklerI personally think we should not provide
canonical
routes at all for these entity types, which is what I tried to achieve in my patch above. We simply don't have a canonical route in any sensible meaning of that word for e.g. menu links or blocks, so it makes no sense to declare a path.Comment #172
dawehnerWell, I think from a REST request point of view, I think it would be fine to have a canonical path for them, but I agree with you, don't we should not have a canonical route,
which is done, when there is no view builder available.
In general though we cannot drop just canonical link templates, as
core/modules/content_translation/content_translation.module:139
relies on it.Comment #173
jhodgdonYeah, for translation I think we not only need a canonical link template, I think it also assumes that the canonical route exists and works, as well as a local task group based on it, right?
Anyway, given my manual testing in #170, I don't think that the current patch is OK -- and I didn't test it with translation either.... It seems to be a mix of having and not having various link templates and URLs work correctly (for instance, the Edit shortcut link from the Shortcuts page doesn't work right).
So. We still need to figure out what to do. If we need a canonical route for translation, but a canonical route doesn't make sense, then we have a problem that we need to figure out how to solve.
Comment #174
Crell CreditAttribution: Crell at Palantir.net commentedIt sounds like the only remaining issue here is which links/routes we define, right? Otherwise we're good now?
Proposal for the links:
1) Keep the addition of /edit to any edit-form links, for consistency and to allow for the following points.
2) Omit the canonical links for now.
3) If any of those entities need a canonical link for translation purposes, create a follow-up issue to discuss/add those. That way it doesn't block this issue with creepy scope.
Does that sound reasonable?
Comment #175
Wim LeersSounds good.
I'm no expert on this, but wouldn't it be sensible to have
content_translation
fall back to theedit-form
link if nocanonical
link is available? All content must surely be editable, so that is a safe fallback?Comment #176
jhodgdonRE the plan in #174, I do not think it is OK for this patch to break translation of blocks, menu items, vocabularies, or shortcuts. So I do not think that is the right plan.
One option for the present patch, in order to get it in and move on, would be to omit the 4 entities in question from the patch and make it "Implement auto-route generation for most core entities" instead of all, and then open a follow-up to deal with block content, menu link, shortcut, and vocabulary.
One other remaining issue we need to deal with before the patch is committed, is that all of the tests need to be restored to having their links hard-coded so that they remain being regression tests for URLs staying the same, and we can clearly see which URLs have changed.
Comment #177
tstoecklerRe #175: Con*fig* translation completely uses the edit-form link only from the get-go, I think that is a much saner assumption. Especially because for con*tent* translation the translation form actually *is* (a variant of) the edit form, so we *do* rely on an edit form being available while having a canonical route is an often true, yet completely unnecessary assumption. That is of course, not for this issue.
Comment #178
jhodgdonAh. OK then. Well.
So the entities MenuLinkContent, Shortcut, and BlockContent are content entities. They will need to have a canonical link still. IMO if we are going to have this link defined, then we should also make sure it does something sensible. BlockContent seems to have that based on my manual testing in #170; MenuLinkContent and Shortcut do not.
Vocabulary on the other hand is a ConfigEntity. Patch #164 is adding a canonical link to it and setting it to the same path as the edit link. It seems that it shouldn't need a canonical link until such time as we decide there is a need for one, and at that time we should create one, give it its own unique path, and make it do something sensible. But defining one now at the same path as edit, given all the above discussion, is probably not the right thing to do, right?
Comment #179
tstoecklerI don't think that is very sensible IMO. The effect that that page exists is just a weird side effect of the combination of various parts of the entity system together with this page.
1. Content entities have a view builder by default (@see ContentEntityType::__construct())
2. This patch generates a route if a canonical link and a view builder exist
Thus, with this patch such a "full-page block" route gets creates automatically even though it doesn't exist in HEAD and even though we don't explicitly do anything to make it appear. One more reason to kill off the canonical route. I guess for now, we can just explicitly set the view builder to NULL, to circumvent this temporarily.
Comment #180
jhodgdon@tstoeckler I am inclined to agree with you except that @dawehner points out that for REST it may make sense to have a route that returns the block text (not so much for full-page HTML requests but makes more sense if it's requesting just that block).
Comment #181
tstoecklerYes, I agree that it might be useful in the future. In fact, I thought that REST was the reason we had that route in HEAD in the first place .... until I went and checked, that we in fact don't (but that this patch "creates" the route). So regardless of REST or not, I don't think *this issue* should be introducing a canonical route for blocks?!
Comment #182
jhodgdonThat is what I've been saying for quite a while so I will not disagree with you. ;)
However, actually what this patch does is take the canonical link templates (which already existed for the 3 Content entities in question, block content, menu link, and shortcut), and change their URLs from being previously the same as the edit URLs to having new different URls. The canonical link templates had to exist previously for translation, it's just that they were rigged to be the same as edit and for some reason now they are not.
So. Once again. What we need to do is decide whether (given that these are content entities and currently need the edit and canonical link templates both to be defined in order for content_translation.module to work) we want to:
a) Make new URLs for canonical and make them actually do something somewhat sensible.
b) Make new URLs for canonical and have them not actually do anything sensible.
c) Keep the URLs for canonical and edit be the same, as they were prior to this issue.
I do not think (b) is OK, and that is what the current patch does.
I'm not sure what "sensible" means in (a) for these entities, but if someone can define what it should mean and make it work, that would probably be OK... but I don't understand why doing (a) is not out of scope for this patch since all it's supposed to do is convert existing entity routes to use an auto-generator, not change or add URLs that never existed before. Or so I would think.
Comment #183
tstoecklerWell for c) we would have to find a way to not generate 'canonical' routes for these entity types. The only way I can see that working is if we explicitly set the view builder to NULL for these entity types.
Comment #184
Crell CreditAttribution: Crell at Palantir.net commentedkgoel and I discussed this issue at the MWDS sprint. Using MenuLinkContent as an example:
Current status:
1) We define links for both canonical and edit-form, that point to the same place.
2) We define a route for only edit-form. There is no canonical route.
If we switch to auto-routing (this issue), and keep both routes equal, then we would get 2 routes defined with identical paths. That's going to confuse the routing system.
If we split the URLs, we need to figure out what to do with the newly-created canonical route.
So, let's punt. Proposed solution:
1) Add the /edit suffix, as discussed above, for consistency.
2) Keep both canonical and edit-form pointing to the same place (now with /edit).
3) For those entities that want to double-up their relationships like that, they use a custom provider and unset the canonical route so that it does not get generated. This is fairly simple code. (Just subclass and add an unset() statement.)
4) Net result: Aside from the path changing (in a logical fashion) nothing changes from the current status quo. However, if we ever figure out a meaningful use for those canonical routes later we can move them to the non-/edit path and put that meaningful code there. And that can be done case-by-case and, IMO, is an 8.x safe thing to do.
jhodgdon, does that work for you?
Comment #185
jhodgdonMy concern is just that we should not have routes or paths that don't work, and the latest patch has several that do not work.
The proposal in #184 seems to be basically "Make an auto-route generator that maintains the status quo of no canonical route" for these entities. I'm OK with that; it seems that others are not though, given the lengthy series of patches and discussions here.
And whatever happens... Please also see #159 item 2 -- fix the tests so that we can explicitly see what URLs are changing, and so that our tests are regression tests for the URLs changing in the future. It *may* be OK to change the URLs at this point in Drupal 8 development (although I don't see why that is even necessary), but I doubt it would be cool to change them later on after 8.0.0 releases, and worse yet to change them and not even notice that it had happened.
Comment #186
Crell CreditAttribution: Crell at Palantir.net commentedTurns out, it makes sense to centralize the special handling of canonical/edit-form double handling. So I did and, I think, converted the tests back to raw URLs. Testbot will find all the places I got that wrong.
Comment #188
jhodgdonWe're running into the "content entities must have canonical routes" problem in some of the tests, it looks like?
Comment #189
dawehnerWorking on it
Comment #190
dawehnerIt SIMPLE does not work to just have one of the two route.s There is no way that this works, seriously. You need the canonical route for URls,
and the edit one for translation support. Here is some intermediate work.
Comment #191
andyceo CreditAttribution: andyceo commentedComment #193
dawehnerLet's try something out.
Comment #195
dawehnerAfter looking into it for a while I realized that moving duplicated routes to /edit has one major flaw: The passed based breadcrumbs break with that.
Previously you could resolve on admin/structure/taxonomy/{example}/translate to admin/structure/taxonomy/{example} which is no longer possible.
Comment #197
dawehnerThis is sadly what this issue tries to achieve
Comment #198
dawehnerSplitted up issue into an issue which deals with just providing such a tool, see #2578955: Implement auto route generation but DON'T use it for all core entities
We don't need to provide tools for broken core entity types, IMHO
Comment #199
Wim LeersFocused scopes FTW.
Comment #200
Wim Leers#2578955: Implement auto route generation but DON'T use it for all core entities landed (hurray!), this now needs to be rerolled on top of that.
Comment #201
Crell CreditAttribution: Crell as a volunteer commentedSince it's mainly the config entities that are problematic here, should we split this issue again and auto-generate just the content entities (node, user, term, etc. all seem fairly safe and un-controversial), which should be fairly straightforward, and then keep discussing how to figure out the config entities?
Comment #202
dawehnerYeah I think this would totally make sense to extract the entity types which are easy to do.
Here is another subissues we could do base upon the experience in https://github.com/Jaesin/content_entity_base/blob/master/src/Entity/Rou...
see #2596095: Provide a route provider for add-form of entities
Comment #203
bojanz CreditAttribution: bojanz at Centarro commentedHow about we close this 200-comment issue, and continue with the conversions (which are probably 8.2 material at this point) elsewhere?
We now have add-form, edit-form, delete-form, and add-page is RTBC.
Comment #204
dawehner+1
Comment #205
Crell CreditAttribution: Crell at Palantir.net commentedYep. Spin up some new issues, link 'em, then close this beast. :-)
Comment #207
tstoecklerHere's a list of all entities that could get the route provider treatment, as far as I see. I grouped them by their respective base class.
ConfigEntityBase
- Action
- Block
- ConfigurableLanguage
- DateFormat
- EntityFormMode
- EntityViewMode
- FilterFormat
- ImageStyle
- Menu
- ResponsiveImageStyle
- Role
- SearchPage
- View
ConfigEntityBundleBase
- BlockContentType
- CommentType
- ContactForm
- NodeType
- ShortcutSet
- Vocabulary
ContentEntityBase
- Block
- Comment
- MenuLinkContent
- Shortcut
- Term
Will open an issue for the content entity ones, to get started with that, but opened #2723579: [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider as a first step.
Comment #208
dawehnerNo reason for that being assigned to me.
Comment #209
tstoecklerOpened #2723615: [PP-1] Use entity route providers for all content entity types and #2760653: Use an entity route provider for taxonomy terms.
I think it's fine to keep this open as a tracking issue for now, so made this the parent of the three sub-issues for now.
If people want to mark this closed/fixed, that's also fine of course.
Comment #215
apaderno