Problem/Motivation

Entity API documentation in core states that:

Entity routes, like other routes, are defined in *.routing.yml files; see the Routing API topic for more information. Entities may alternatively use an auto route provider class;

This wording can be interpreted as saying that the route provider class and YAML file are mutually exclusive, and developers might commit time and effort to doing it one way or the other, but not both. However, core modules e.g. the node module have both a node.routing.yml and a NodeRouteProvider class mentioned in the entity class annotation.

Proposed resolution

  1. Confirm that routing YAML and routing annotations can co-exist for entity routes (confirmed by @dawehner in comments)
  2. Determine the order of preference for these. (following further discussion, confirmation that YAML "wins")
  3. Modify the documentation to make all this clear.

If both can co-exist, a simple rewording might be:

Entity routes can be defined in one of two ways: using *.routing.yml files, as discussed in the Routing API topic; or declaring an auto route provider class in the entity class' annotations. When both YAML files and a route provider class exist, then [tbc.]

(Is "auto" a recognized term here btw, or can it be dropped?)

Remaining tasks

  1. Get community agreement on proposed changes to documentation.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#12 entity_routing_via_yaml-2801409-interdiff-9-12.txt1023 bytesjp.stacey
#12 entity_routing_via_yaml-2801409-12.patch3.38 KBjp.stacey
#9 entity_routing_via_yaml-2801409-interdiff-5-9.txt732 bytesjp.stacey
#9 entity_routing_via_yaml-2801409-9.patch3.52 KBjp.stacey
#5 entity_routing_via_yaml-2801409-5.patch3.52 KBjp.stacey
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jp.stacey created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

Confirm that routing YAML and routing annotations can co-exist for entity routes.

Yes, they can totally coexist.

Determine the order of preference for these.

I would frame it like that: If the annotations/default classes help you, use them. Otherwise, feel free to do what you want.

jp.stacey’s picture

jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commented

@dawehner thanks re confirmation.

Regarding the other point, I wasn't thinking so much of "best practice advice" order of preference (is there any?) but rather - if you define a route in both places, what happens? Does one override the other?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

Oh, conceptually I'd argue it simply shouldn't be done. The one in the annotation will win, but its really not meant to be for altering :)

jp.stacey’s picture

jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commented
Status: Active » Needs review
FileSize
entity_routing_via_yaml-2801409-5.patch3.52 KB

@dawehner thanks for clarifying. Patch attached for discussion.

jp.stacey’s picture

jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commented
Issue summary: View changes
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented

Re #4: Are you sure? It's been a while since I tried it out but I'm pretty sure it's the other way around.

jp.stacey’s picture

jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commented
Status: Needs review » Needs work

@tstoeckler you're quite right: I've just trialled adding a nonsense entity.node.edit_form route to:

* core/modules/node/node.routing.yml
* modules/custommodule/custommodule.routing.yml

and in both cases, the existing route at node/{node}/edit 404s. I'll set this back to "Needs work", although I think it's just a rewording required.

jp.stacey’s picture

jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commented
Status: Needs work » Needs review
FileSize
entity_routing_via_yaml-2801409-9.patch3.52 KB
entity_routing_via_yaml-2801409-interdiff-5-9.txt732 bytes

Patch includes correction following experimentation with YAML/provider conflicts.

jp.stacey’s picture

jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commented
Issue summary: View changes
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

I really like these improvements!

+++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -358,11 +358,20 @@
+ * It's possible to use both a YAML file and a provider class for entity routes:
+ * if a route name is found in both locations, the one in the YAML file takes
+ * precedence. However, this can be confusing and should be avoided: if you
+ * want like to use a provider class for entity routes, configure all routes
+ * related to that entity through the provider class; leave only non-entity
+ * routes (e.g. settings pages) in the YAML file.

Well, I'd argue that providers at the moment are mostly there for autogenerating stuff. Making suggestions what the better approach here is, feels kinda arbitrary.

jp.stacey’s picture

jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commented
FileSize
entity_routing_via_yaml-2801409-12.patch3.38 KB
entity_routing_via_yaml-2801409-interdiff-9-12.txt1023 bytes

@dawehner thanks for the feedback. I added the advice because I felt there was a bit of a "best practice" gap there, and that's broadly how core works as far as I can see, but you're right that people might have reasons for splitting entity routes between the two.

So I've removed it, while retaining the note about avoiding route-name duplication, which I think is worth mentioning because:

* given the YAML file takes precedence, then it's not even as though autogeneration can e.g. sometimes override it, sometimes not; there's no advantage to duplication of route names that I can see.
* we weren't 100% sure when we started this thread what would happen, so it's easy to confuse other developers by duplicating route names: "what does this code do again?"

Updated patch and interdiff attached.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

@jp.stacey
Thank you!

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented 
+++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -389,19 +396,21 @@
+ * - \Drupal\Core\Entity\Routing\DefaultHtmlRouteProvider provides canonical,
+ *   edit-form, and delete-form routes.

I know this is just being moved, but this is actually incomplete as of now. We also generate add-form, add-page and collection routes.

I would be fine fixing this in a follow-up (thus, leaving at RTBC) but wanted to bring it up so we don't forget.

jp.stacey’s picture

jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commented

@tstoeckler granular commits would be best if that's OK, not least because I've discovered another mention of edit-form etc. elsewhere, so I'll detail that on the new issue #2820665: Entity routing documentation need other routes adding and we can discuss it there.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented

That's perfect, thanks for opening that issue!

alexpott’s picture

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

Committed and pushed 96152bf to 8.3.x and ea33c32 to 8.2.x. Thanks!

  • alexpott committed 96152bf on 8.3.x 
    Issue #2801409 by jp.stacey, dawehner, tstoeckler: Entity routing via...

  • alexpott committed ea33c32 on 8.2.x 
    Issue #2801409 by jp.stacey, dawehner, tstoeckler: Entity routing via...

Status: Fixed » Closed (fixed)

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