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

  1. Eliminate admin-form link entirely from MyEntity as it's not relevant and redundant.
  2. Add field_ui_base_route annotation key for EntityType to point a route for field_ui where attach its tasks.
  3. Refactor getAdminRouteInfo()

Remaining tasks

tbd

User interface changes

None.

API changes

new field_ui_base_route annotation key for EntityType

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Crell’s picture

Issue tags: +WSCCI
larowlan’s picture

Note field ui uses admin form to attach the manage fields, manage display tabs to bundles

Crell’s picture

larowlan: 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.)

Berdir’s picture

AFAIK, 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?

Crell’s picture

It 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.

jhodgdon’s picture

I 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)

andypost’s picture

Issue tags: +Entity Field API

I 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"

jhodgdon’s picture

I 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"?

Berdir’s picture

field-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.

jhodgdon’s picture

So 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.

Berdir’s picture

Yes, moving this outside of links is what I meant in #5.

andypost’s picture

Status: Active » Needs review
FileSize
16.21 KB

Initial 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()

Status: Needs review » Needs work

The last submitted patch, 13: 2309187-13.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
1.03 KB
16.21 KB

Status: Needs review » Needs work

The last submitted patch, 15: 2309187-admin-form-15.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
5.22 KB
16.69 KB

s/getKey/get and added protected property to annotation class

tim.plunkett’s picture

+++ b/core/modules/field_ui/src/Plugin/Derivative/FieldUiLocalTask.php
@@ -172,8 +172,8 @@ public function getDerivativeDefinitions($base_plugin_definition) {
+      if ($entity_info->isFieldable() && $route_name = $entity_info->get('field_ui_base_route')) {
+        $admin_form = $route_name;

So just call it $admin_form in assignment.

andypost’s picture

FileSize
3.27 KB
18.2 KB

@tim.plunkett I think "admin_form" does not makes sense in context of routing, so here's a clean-up

Crell’s picture

+++ b/core/modules/node/src/Entity/Node.php
@@ -49,13 +49,13 @@
+ *   field_ui_base_route = "node.type_edit",

The 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.

Crell’s picture

Issue tags: +TCDrupal 2014
andypost’s picture

If we going to provide a paths so I've no idea how to generate all field_ui tasks!

+++ b/core/modules/field_ui/src/Plugin/Derivative/FieldUiLocalTask.php
@@ -172,20 +172,19 @@ public function getDerivativeDefinitions($base_plugin_definition) {
+      if ($entity_info->isFieldable() && $route_name = $entity_info->get('field_ui_base_route')) {
+        $local_tasks["field_ui.fields:overview_$entity_type"]['base_route'] = $route_name;
+        $local_tasks["field_ui.fields:form_display_overview_$entity_type"]['base_route'] = $route_name;
+        $local_tasks["field_ui.fields:display_overview_$entity_type"]['base_route'] = $route_name;
+        $local_tasks["field_ui.fields:field_form_display_default_$entity_type"]['base_route'] = $route_name;
+        $local_tasks["field_ui.fields:field_display_default_$entity_type"]['base_route'] = $route_name;

that's would be tricky

Crell’s picture

We can probably have a standard naming convention for the generated route names, just as we're moving toward for the entities themselves.

andypost’s picture

Only 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?

+++ b/core/modules/block_content/src/Entity/BlockContent.php
@@ -52,7 +51,8 @@
+ *   field_ui_base_route = "block_content.type_edit",

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -48,9 +48,9 @@
+ *   field_ui_base_route  = "entity.comment_type.edit_form",

+++ b/core/modules/contact/src/Entity/Message.php
@@ -30,10 +30,8 @@
+ *   field_ui_base_route = "entity.contact_category.edit_form",

+++ b/core/modules/node/src/Entity/Node.php
@@ -49,13 +49,13 @@
+ *   field_ui_base_route = "node.type_edit",

+++ b/core/modules/shortcut/src/Entity/Shortcut.php
@@ -44,9 +44,9 @@
+ *   field_ui_base_route = "entity.shortcut.canonical",

+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTest.php
@@ -43,8 +43,8 @@
+ *   field_ui_base_route = "entity_test.admin_entity_test",

+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestBaseFieldDisplay.php
@@ -32,8 +32,8 @@
+ *   field_ui_base_route = "entity_test.admin_entity_test",

+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMul.php
@@ -40,8 +40,8 @@
+ *   field_ui_base_route = "entity_test.admin_entity_test_mul",

+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestNoId.php
@@ -20,9 +20,7 @@
+ *   field_ui_base_route = "entity_test.admin_entity_test_no_id",

+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestStringId.php
@@ -33,8 +33,8 @@
+ *   field_ui_base_route = "entity_test.admin_entity_test_string_id",

+++ b/core/modules/taxonomy/src/Entity/Term.php
@@ -42,11 +42,11 @@
+ *   field_ui_base_route = "taxonomy.overview_terms",

+++ b/core/modules/user/src/Entity/User.php
@@ -47,9 +47,9 @@
+ *   field_ui_base_route = "user.account_settings",

hm

jhodgdon’s picture

Status: Needs review » Needs work

I 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:

- *   bundle_entity_type = "block_content_type"
+ *   bundle_entity_type = "block_content_type",
+ *   field_ui_base_route = "block_content.type_edit",
  * )
  */
 class BlockContent extends ContentEntityBase implements BlockContentInterface {

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:

  *   - admin-form: Form for editing bundle or entity type settings.
  *   - Other link types specific to your entity type can also be defined.
+ * - If content entity provides annotation key 'field_ui_base_route' with the
+ *   route where field UI will attach it management forms.

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!

Crell’s picture

Thinking 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.

andypost’s picture

Status: Needs work » Needs review
FileSize
1.2 KB
18.57 KB

re-rolls and fix for docs, @jhodgdon thanx for suggestion

PS: Annotations allows trailing commas since #2138867: Allow dangling commas in annotations

jhodgdon’s picture

Issue tags: +Needs change notification

Looks good to me! It probably needs a change notice before it can be committed though?

andypost’s picture

FileSize
18.62 KB

Re-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?

jhodgdon’s picture

Issue tags: -Needs change notification

It 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

andypost’s picture

1) field_iu module has no hook_entity_info_alter() so I think it's fine to leave the docs where they are
2) search 'admin-form' within change records returns nothing

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

It'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.

alexpott’s picture

It looks like the issue summary could do with an update before this committed since it still is suggesting multiple solutions.

andypost’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
FileSize
18.71 KB
yched’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -333,9 +333,9 @@ public function getController($entity_type, $controller_type, $controller_class_
       public function getAdminRouteInfo($entity_type_id, $bundle) {
    -    if (($entity_type = $this->getDefinition($entity_type_id, FALSE)) && $admin_form = $entity_type->getLinkTemplate('admin-form')) {
    +    if (($entity_type = $this->getDefinition($entity_type_id, FALSE)) && $route_name = $entity_type->get('field_ui_base_route')) {
    

    This 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 ?

  2. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
    @@ -204,6 +204,13 @@ class EntityType implements EntityTypeInterface {
    +   * The route name where field UI will use to attach its routes.
    

    Not a sentence :-)

  3. +++ b/core/modules/field_ui/src/FieldUI.php
    @@ -28,7 +28,7 @@ class FieldUI {
    -    if ($entity_type->hasLinkTemplate('admin-form')) {
    +    if ($entity_type->get('field_ui_base_route')) {
    

    Shouldn't this use the getAdminRouteInfo() (or whichever new name) method available on EM ?

  4. +++ b/core/modules/field_ui/src/Plugin/Derivative/FieldUiLocalTask.php
    @@ -69,7 +69,7 @@ public function getDerivativeDefinitions($base_plugin_definition) {
    -      if ($entity_type->isFieldable() && $entity_type->hasLinkTemplate('admin-form')) {
    +      if ($entity_type->isFieldable() && $entity_type->get('field_ui_base_route')) {
    

    Likewise ?

  5. +++ b/core/modules/field_ui/src/Plugin/Derivative/FieldUiLocalTask.php
    @@ -172,20 +172,19 @@ public function getDerivativeDefinitions($base_plugin_definition) {
    +      if ($entity_info->isFieldable() && $route_name = $entity_info->get('field_ui_base_route')) {
    

    Likewise ?

  6. +++ b/core/modules/field_ui/src/Routing/RouteSubscriber.php
    @@ -41,9 +41,9 @@ public function __construct(EntityManagerInterface $manager) {
    -      if ($entity_type->isFieldable() && $entity_type->hasLinkTemplate('admin-form')) {
    +      if ($entity_type->isFieldable() && $route_name = $entity_type->get('field_ui_base_route')) {
    

    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 ?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Setting to needs work for #35

andypost’s picture

Issue summary: View changes

getAdminRouteInfo() is not used in core and actually outdated because this route does not require bundle entity type in route_parameters

andypost’s picture

Status: Needs work » Needs review
FileSize
6.2 KB
20.88 KB

I'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)

core/modules/field_ui/src/FieldUI.php:31:    if ($entity_type->get('field_ui_base_route')) {
core/modules/field_ui/src/Plugin/Derivative/FieldUiLocalTask.php:72:      if ($entity_type->isFieldable() && $entity_type->get('field_ui_base_route')) {
core/modules/field_ui/src/Plugin/Derivative/FieldUiLocalTask.php:175:      if ($entity_type->isFieldable() && $route_name = $entity_type->get('field_ui_base_route')) {
core/modules/field_ui/src/Routing/RouteSubscriber.php:44:      if ($entity_type->isFieldable() && $route_name = $entity_type->get('field_ui_base_route')) {

Also I see no reasons to introduce new method in EntityTypeInterface because of 2)

There's FieldUI::getOverviewRouteInfo() but for other needs

PS: Patch also cleans s/entity_type/entity_type_id as code touched

andypost’s picture

FileSize
503 bytes
20.88 KB

Fix #35.2

jhodgdon’s picture

I think that the latest patch is good. +1 for RTBC...

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Me too.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 39: 2309187-admin-form-39.patch, failed testing.

andypost’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
20.87 KB
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed f1713d5 and pushed to 8.0.x. Thanks!

  • alexpott committed f1713d5 on 8.0.x
    Issue #2309187 by andypost | Crell: Fixed double-link-entry between...
yched’s picture

+++ b/core/modules/system/entity.api.php
@@ -332,8 +332,12 @@
+ * - If your content entity is fieldable, provide 'field_ui_base_route'
+ *   annotation, giving the name of the route that the Manage Fields, Manage

'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 ?

jhodgdon’s picture

OK, needs a quick follow-up then. Here or another issue?

jhodgdon’s picture

I guess if we want to do this it should be a separate issue, because this wording is used elsewhere in that topic.

yched’s picture

Oh ? - well, if that's a common wording, then never mind #46...

andypost’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.