Problem/Motivation

Most Field UI forms need two things from the route parameters, an entity type id and a bundle name, yet the current code has to bend over backwards to get that bundle name because it currently receives either an (upcasted) entity of the actual bundle (e.g. a node_type entity) or a string for content entity types which don't use a config entity to manage their bundles.

Some Field UI routes have the bundle entity type id in their name (e.g. entity.node_type.field_ui_fields) and others have the content entity type ID if they don't use config entities for bundles.

Proposed resolution

- Add a Field UI route enhancer that always provides the bundle name parameter as a string
- Update all Field UI route names to be consistent and only include the ID of the content entity type

Remaining tasks

None.

User interface changes

Nope.

API changes

All Field UI route names will be consistent and always include the ID of the content entity type.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue priority Major because the current code is very confusing even for Field UI maintainers.
Prioritized changes The main goal of this issue is to reduce code fragility in the Field UI forms.
Disruption Minor disruption for contrib but Commerce 2.x (at least) will be very happy to see an improvement in this area.
CommentFileSizeAuthor
#6 interdiff.txt5.61 KBamateescu
#6 2428035-6.patch35.04 KBamateescu
#4 interdiff.txt6.37 KBamateescu
#4 2428035-4.patch34.04 KBamateescu
#1 sane_field_ui_routes.patch32.65 KBamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Active » Needs review
FileSize
sane_field_ui_routes.patch32.65 KB

Here's what I have in mind for cleaning up this mess.

Status: Needs review » Needs work

The last submitted patch, 1: sane_field_ui_routes.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Related issues: +#2346857: Set default property bundle_entity_type in EntityType to NULL

Great start! Thanx Andrey for posting that!

As I pointed in related issue we have a trick with bundle argument, there's 3 ways:
1) bundle is config entity
2) bundle is custom string
3) no bundle (user entity at least passes "user")

I see no principal changes and consistency in route naming, maybe need separate issue...
There's routes
1) overview (field list) - only for entities with bundles as config entity (attached to field_ui_base_route annotation)
2) add field, thanx @amateescu
3) edit field and storage - needs bundle as string to build a name
3) form, view display edit - needs bundle as string to build a name

So I think we need one more method to generate parameters for entity link templates

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Needs work » Needs review
FileSize
2428035-4.patch34.04 KB
interdiff.txt6.37 KB

Spoke to @dawehner in IRC about the current test fails and he suggested to just check if the path already contains {bundle} and don't add it to $defaults if so. That seems to work and allowed me clean up a few more things.

@andypost, I'm not sure what method you're talking about, this patch removes the last "field ui" link template weirdness :)

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented 
+++ b/core/modules/field_ui/src/Form/EntityFormDisplayEditForm.php
@@ -108,7 +107,7 @@ protected function getTableHeader() {
     return Url::fromRoute('entity.entity_form_display.' . $this->entity->getTargetEntityTypeId() . '.form_mode', [
-      FieldUI::getRouteBundleEntityType($entity_type) => $this->entity->getTargetBundle(),
+      $entity_type->getBundleEntityType() => $this->entity->getTargetBundle(),
       'form_mode_name' => $mode,
     ]);

That's example of using broken function result. In related issue I'm trying to clean-up the usage of this method
Because it's weird to return "bundle" (string) as result of EntityTypeInterface::getBundleEntityType() and flip/flop bundle entity type for user entity at least.

And what's about entities that bundles are custom?

amateescu’s picture

amateescu CreditAttribution: amateescu commented
FileSize
2428035-6.patch35.04 KB
interdiff.txt5.61 KB

@andypost, ok, I finally understood what's wrong with using EntityTypeInterface::getBundleEntityType().

The only nice way to handle it in this patch without bringing #2346857: Set default property bundle_entity_type in EntityType to NULL in scope is to re-introduce the FieldUI::getRouteBundleEntityType() helper that I removed above, and I think we can even make it a bit smarter, see attached interdiff. This way, there is a single call to getBundleEntityType() and #2346857: Set default property bundle_entity_type in EntityType to NULL can still do further cleanup.

And what's about entities that bundles are custom?

That's already handled in the patch, EntityTest entities do not use a config entity type for their bundles, so they just use a {bundle} route parameter.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs review » Reviewed & tested by the community

I think that's great to bound all field-ui routes to their entities not bundles

@amateescu thanx, that's simplifies code in related!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Totally agree with this - the field ui is managing fields on the the node entity not the node_type entity. Committed 4b8a3c4 and pushed to 8.0.x. Thanks!

Thank you for adding the beta evaluation to the issue summary.

  • alexpott committed 4b8a3c4 on 8.0.x 
    Issue #2428035 by amateescu: Bring some sanity into Field UI routes and...
yched’s picture

yched CreditAttribution: yched commented

@amateescu+++++++

Status: Fixed » Closed (fixed)

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