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.
Problem/Motivation
As discovered in #2278567: Standardize node route names by relationship, there's both an inconsistency and a double-entry in the way entity management links are recorded.
The MyEntity class itself has a link admin-form.
IF the Entity is bundle-able (which is true of a subset of content entities) then there is also a MyEntityType class that must be defined. That class also has a link edit-form.
Both of those links point to the same place. That creates a number of problems:
- If they point to different routes, what happens?
- Once we switch to uri templates, what if they point to different routes?
- Once we start auto-generating routes based off of the link templates, we end up with two routes with the same path, but otherwise no difference. That's not good.
- Which one wins? It conceptually makes the most sense on... the the class that is optional.
Proposed resolution
- Eliminate admin-form link entirely from MyEntity as it's not relevant and redundant.
- Add
field_ui_base_route
annotation key forEntityType
to point a route for field_ui where attach its tasks. - Refactor
getAdminRouteInfo()
Remaining tasks
tbd
User interface changes
None.
API changes
new field_ui_base_route
annotation key for EntityType
Comment | File | Size | Author |
---|---|---|---|
#43 | 2309187-admin-form-43.patch | 20.87 KB | andypost |
#39 | interdiff.txt | 503 bytes | andypost |
Comments
Comment #1
Crell CreditAttribution: Crell commentedComment #2
Crell CreditAttribution: Crell commentedComment #3
larowlanNote field ui uses admin form to attach the manage fields, manage display tabs to bundles
Comment #4
Crell CreditAttribution: Crell commentedlarowlan: Would it be able to use the Type entity's edit-form link instead? (Auto-placing such UIs is rather the whole point of having them defined the way we're trying to do.)
Comment #5
BerdirAFAIK, field_ui is the only thing that uses admin-form.
It's also never a self-defined route, it is by design a reference to something else.
It points to a pre-existing route where field_ui should attach it's manage fields local tasks and routes.
For entity types without bundles, it can point to arbitrary route, for users, it's the people settings form. There's no way that field_ui can figure this out itself.
So, we do need something, but I think it doesn't have to be a entity link and it should possibly have a better name too?
That said, just moving out of the links section will still somewhat conflict with auto-generating routes as you will have to know the route for your bundle edit link?
Comment #6
Crell CreditAttribution: Crell commentedIt definitely sounds like admin-form is just not a link in the first place and should go away, being replaced by... something that's not in @links. (And therefore not something that as WSCCI lead I need to care about, so I'm not as picky about what it becomes.) Some new annotation key, I guess.
Comment #7
jhodgdonI ran into this, this past weekend... Field UI doesn't work at all unless admin_link is defined on the entity, and even then you also have to define a local task group for the Edit page if you want it to appear in the list of tabs... it was really confusing me why this wasn't working.
If we keep this how it is, we should at least make sure it is well documented.
See also #2316171: [meta] Improve DX of entity defining (if you want a UI)
Comment #8
andypostI think there's no way to remove that, but it makes sense to rename this link to be more descriptive.
Maybe better to extract this to own annotation key like "fields-manage"
Comment #9
jhodgdonI really like that idea, because it tells you exactly what this is used for... except that it's not pointing to the actual "Manage Fields" page, but instead to the "Edit" page in the tab/task group. What it really is is "Base page for manage fields, manage display, and manage form display tab/task group", but that is a little long. :)
Maybe something like "field-ui-base"?
Comment #10
Berdirfield-ui-base sounds good to me, one could argue that other modules might use that too in contrib, but they're free to do that, the main use case is the field UI.
Maybe explicitly include route, like field-ui-base-route, as that would make it clear that you have to specify a route there.. especially if the links/route change happens. as it would be different then.
Comment #11
jhodgdonSo you're saying that this one would be different from everything else in the "links" section?
If that is the case... and it's really not the same as the other "links" anyway... Let's move it out of "links" entirely and make it top-level, and yes then call it field-ui-base-route.
Comment #12
BerdirYes, moving this outside of links is what I meant in #5.
Comment #13
andypostInitial patch, I've removed admin-form to see how many tests would fail
Also no idea what to do with removed test case
EntityUrlTest::testUrlForAdminForm()
Comment #15
andypostComment #17
andyposts/getKey/get
and added protected property to annotation classComment #18
tim.plunkettSo just call it $admin_form in assignment.
Comment #19
andypost@tim.plunkett I think "admin_form" does not makes sense in context of routing, so here's a clean-up
Comment #20
Crell CreditAttribution: Crell commentedThe links in links[] are being converted to paths soon. I think it would make sense to use a path here too, no? That way we can auto-generate the various routes at and based on that path.
Comment #21
Crell CreditAttribution: Crell commentedComment #22
andypostIf we going to provide a paths so I've no idea how to generate all field_ui tasks!
that's would be tricky
Comment #23
Crell CreditAttribution: Crell commentedWe can probably have a standard naming convention for the generated route names, just as we're moving toward for the entities themselves.
Comment #24
andypostOnly block, comment, message and node could use some pattern.
But user and taxonomy (skip entity_tst) does not fits into any convention and it does not solve the issue that tasks needs routes.
maybe @dawehner or @pwolanin could explain a possible way forward?
hm
Comment #25
jhodgdonI took a look at the patch in #19, and the comments since then.
Crell #20 suggested using paths instead of routes for consistency -- but this is not the same thing as the links. For Links, you need to define the path pattern for the links. For this, you need to define the admin route so that the Field UI module can build its derivative routes and links -- the Field UI module actually needs to know the route name. See \Drupal\field_ui\Routing\RouteSubscriber::alterRoutes(). That method is the reason we need this annotation, the only reason, and it needs a route definitely. [that's basically what andypost was saying in #22 I think].
Crell #23 suggested a standard name for the routes, and andypost in #24 said it wasn't too easy to standardize. I agree with #24. The problem is that this annotation is really for "the page to attach all the manage fields, forms, and displays stuff to", and for different entities, this is a different page. For some/most, it may be that the logical place to attach it is the "Edit a bundle" page. For some, it's the "edit the settings" page (like User, which has no bundles). For some, it's another overview page (like Taxonomy, because the "list terms" page is the primary tab, not the "edit the vocabulary name" page). So... I think it's OK that these are not standardized route names.
Looking at the patch... I noticed one thing: In Annotations, you aren't supposed to have a trailing comma at the end of the annotation array. For instance in BlockContent:
I think that last comma in the field_ui_base_route needs to go away. I'm not actually sure why this is even working, I thought that would break the annotation parser. Anyway, that applies to all of the annotation chunks in the patch.
One other thing to fix: in entity.api.php, this documentation:
We need to take out the line about 'admin-form'. And I think this added part is unclear. I suggest:
If your content entity is fieldable, provide 'field_ui_base_route' annotation, giving the name of the route that the Manage Fields, Manage Display, and Manage Form Display pages from the Field UI module will be attached to. This is usually the bundle settings edit page, or an entity type settings page if there are no bundles.
So... Just 2 little things to fix, and I think this is good to go!
Comment #26
Crell CreditAttribution: Crell commentedThinking on it further, while I would still prefer a path in concept I can see where it's harder to do here than for the for-reals-links. This issue at least gets the value out of the links array so I can run with it for now.
Comment #27
andypostre-rolls and fix for docs, @jhodgdon thanx for suggestion
PS: Annotations allows trailing commas since #2138867: Allow dangling commas in annotations
Comment #28
jhodgdonLooks good to me! It probably needs a change notice before it can be committed though?
Comment #29
andypostRe-roll after node routes conversion, also it's not clean how document the new annotation after #1968970: Standardize module-provided entity info documentation and clean-up the @EntityType annotation
@jhodgdon Any idea what to write in CR?
Comment #30
jhodgdonIt looks like what we are supposed to do to document entity annotation is add properties to \Drupal\Core\Entity\EntityType (as opposed to the EntityType annotation class).
And... it looks like this is what you did. So that is good.
Oh... yeah, that issue. Sigh. It looks like you are supposed to document this on a hook_entity_info_alter() hook. Does Field UI even have that?
Regarding the change record, I think:
a) Make sure existing change records with examples of Entity annotation change from having admin-form to using field_ui_base_route (so that we are not providing obsolete examples in our change records). You can search for "admin-form" on the change records page... let's see... I guess there aren't any, so I guess that's OK?
b) I've added a draft change record for this change.
https://www.drupal.org/node/2320547
Comment #31
andypost1) field_iu module has no
hook_entity_info_alter()
so I think it's fine to leave the docs where they are2) search 'admin-form' within change records returns nothing
Comment #32
jhodgdonIt's actually hook_entity_type_alter() now... but still there is no field_ui implementation.
We need to reopen that "how to document entity annotations" issue though. It's not really possible to do it the way that was decided there, and it's horrible DX. Separate issue though.
Anyway... I think this is ready to go.
Comment #33
alexpottIt looks like the issue summary could do with an update before this committed since it still is suggesting multiple solutions.
Comment #34
andypostRe-roll after #2291829: Standardize user route names
Comment #35
yched CreditAttribution: yched commentedThis was based off 'admin-form' link template, and is now based off 'field_ui_base_route' entry, so it looks like the method name is now stale / misleadingly generic ?
Not a sentence :-)
Shouldn't this use the getAdminRouteInfo() (or whichever new name) method available on EM ?
Likewise ?
Likewise ?
Likewise ?
In short, if the method encapsulates 'field_ui_base_route', then the rest of the code should use that method to access the info ?
Comment #36
alexpottSetting to needs work for #35
Comment #37
andypostgetAdminRouteInfo()
is not used in core and actually outdated because this route does not require bundle entity type inroute_parameters
Comment #38
andypostI'd suggest simply remove the useless method from EM, because:
1) new annotation is only about
EntityTypeInterface
2) the key and route is needed only for field UI (tasks and routes)
Also I see no reasons to introduce new method in
EntityTypeInterface
because of 2)There's
FieldUI::getOverviewRouteInfo()
but for other needsPS: Patch also cleans
s/entity_type/entity_type_id
as code touchedComment #39
andypostFix #35.2
Comment #40
jhodgdonI think that the latest patch is good. +1 for RTBC...
Comment #41
Crell CreditAttribution: Crell commentedMe too.
Comment #43
andypostre-roll after #2285083: Rename contact category to contact form
Comment #44
alexpottCommitted f1713d5 and pushed to 8.0.x. Thanks!
Comment #46
yched CreditAttribution: yched commented'field_ui_base_route' annotation ? Shouldn't this rather say "provide a 'field_ui_base_route' entry in the @EntityType annotation" ?
We don't call "annotation" each of the entries in an @Annotation - or do we ?
Comment #47
jhodgdonOK, needs a quick follow-up then. Here or another issue?
Comment #48
jhodgdonI guess if we want to do this it should be a separate issue, because this wording is used elsewhere in that topic.
Comment #49
yched CreditAttribution: yched commentedOh ? - well, if that's a common wording, then never mind #46...
Comment #50
andypostFiled follow-up to discus #2323459: Change wording of annotation keys to properties