Clearly document the expected route name pattern for entities

Attached is a patch which does the following:

• The route name is no longer hardcoded into Entity::toUrl() and can be overwritten by custom entity types.
• There is still no documentation for the assumed route names, but maybe that's no longer required now?

I listed this as a bug because it caused my code to break. Having undocumented assumptions in your code is a very gray area that could be classified as a bug, I thought.

Anyways, attached is a patch which:

1. Renames the method to getRouteName().
2. Makes the method public, to avoid the same mistake as in #2651974: field_ui_entity_operation() cannot respect route parameters because of incorrectly named routes. People may want to actually use this to find out what the route name for a given link template is.
3. Therefore specifies the method on the interface.
4. Documents the default behavior in the method body.

Also: should this be against 8.0.x or 8.1.x? When will 8.1.0 be released because I kind of need this to be fixed ASAP to get my module ready for D8 :)

Some junk snuck into the previous patch. Attached is a rerolled version for 8.1.x.

And attached is the correct patch for 8.0.x.

Setting back to bug report vs 8.0.x as:

• This does not break the API
• It does break modules whose routes deviate from the expected pattern
• It's breaking stuff because it is undocumented
• Setting it to 8.1.x would mean it will only get fixed in 2-3 months

In any case, having a patch for both 8.1.x and 8.0.x will give the entity maintainers a choice :)

ViewUI doesn't implement the base Entity class... :) Updated the patch.

Seems like most of this type of work gets targeted for 8.1.x so setting back to that as per tstoeckler in #6.

8.1.x will be closed in little over a week (https://groups.drupal.org/node/508968), let's not make this have to wait another couple of months please.

The work being done here is exactly the same as in #2651974: field_ui_entity_operation() cannot respect route parameters because of incorrectly named routes, which has passed several reviewers. Albeit on a different method in the same class.

I addressed all the remarks made by tstoeckler in #5, so RTBC?

You can do $entity->toUrl()->getRouteName() if you need that information from the outside.

Which will lead to a crash as per the issue summary. That's the whole issue: If a module does not name its routes entity.MY_ENTITY.LINK_TEMPLATE, then $entity->toUrl() crashes the website.

Unless you're talking about moving forward with this patch, but making the newly introduced method getRouteName() protected?

I marked this issue as RTBC because a D8 module I'm about to release depends on this bug being resolved and I'd hate to have to tell everyone to wait until D8.2.0 comes out before they can properly use it.

The ugly fix I'm using now is this:
http://cgit.drupalcode.org/group/tree/src/Entity/GroupContent.php?h=8.x-...

I just tried to change the getRouteName() method to be protected, but then ViewsUI crashes. Basically, any new protected method will crash ViewsUI because it wraps all Entity methods as shown below.

+++ b/core/modules/views_ui/src/ViewUI.php
@@ -1034,6 +1034,13 @@ public function toLink($text = NULL, $rel = 'edit-form', array $options = []) {
+  public function getRouteName($rel) {
+    return $this->storage->getRouteName($rel);
+  }

So I fixed that by not putting the method on the interface at all and marking it protected as requested above. Attached is a patch for that.

P.S.:
I'm sorry for marking it as RTBC myself, but I've spent a lot of hours debugging and writing patches for D8 lately and most of the issues have just been sitting there for days or even weeks. With the news of 8.1.x becoming frozen soon, I was getting a bit anxious (or even frustrated) and marked them RTBC to get them moving.

I don't mean to be rude by this or comment on the obviously large amount of time the core maintainers put into the D8 issue queue. It's just that I am close to finishing a very 'powerful' module that pushes D8 to its limits and thus tends to uncover the more obscure bugs. I realize this causes the issue to not pick up as much attention as a very common WSOD bug would.

It's just demotivating to not see your issues moving forward.

Also, seeing as this one won't be a public method, I'm removing and closing the related issue.

A good example of where this would need to be overridden is the Group module: It defines plugins that will get routes defined for them automatically on a GroupContent entity. Because there can be multiple plugins, the routes need to be namespaced to avoid collision.

So it really needs route names to be different from the default hard-coded pattern. For example:
entity.group_content.plugin_a.edit_form
entity.group_content.plugin_b.edit_form

If I were to simply use entity.group_content.{whatever the plugin specifies}, I'd obviously get name collisions real quick. I hope this example makes sense and clarifies why I really think this functionality needs to be in a separate, overridable method instead of being hard-coded in toUrl().

Also, thanks for moving this issue forward!

P.S.: The use case I have in Group is really a great example of how hard-coding the route names can severely limit a module. If you would like me to go into detail, I'll gladly write out a proper summary of why this change is needed.

I see what dawehner is getting at and actually, it is a clear case why the method I introduced should be public after all.

For example, he states it would break in DefaultHtmlRouteProvider:

  /**
   * {@inheritdoc}
   */
  public function getRoutes(EntityTypeInterface $entity_type) {
    $collection = new RouteCollection();

    $entity_type_id = $entity_type->id();

    if ($edit_route = $this->getEditFormRoute($entity_type)) {
      $collection->add("entity.{$entity_type_id}.edit_form", $edit_route);
    }

    if ($canonical_route = $this->getCanonicalRoute($entity_type)) {
      $collection->add("entity.{$entity_type_id}.canonical", $canonical_route);
    }

    if ($delete_route = $this->getDeleteFormRoute($entity_type)) {
      $collection->add("entity.{$entity_type_id}.delete_form", $delete_route);
    }

    return $collection;
  }

But if we introduced the method as a public function, the above could easily be rewritten along these lines:

  /**
   * {@inheritdoc}
   */
  public function getRoutes(EntityTypeInterface $entity_type) {
    $collection = new RouteCollection();

    $entity = /* create empty entity from $entity_type here */;

    if ($edit_route = $this->getEditFormRoute($entity_type)) {
      $collection->add($entity->getRouteName('edit-form'), $edit_route);
    }

    if ($canonical_route = $this->getCanonicalRoute($entity_type)) {
      $collection->add($entity->getRouteName('canonical'), $canonical_route);
    }

    if ($delete_route = $this->getDeleteFormRoute($entity_type)) {
      $collection->add($entity->getRouteName('delete-form'), $delete_route);
    }

    return $collection;
  }

Keep in mind that the above is off the top of my head. There's probably a case to be made to move the method to the entity type, so the calls would look like $collection->add($entity_type->getRouteName('delete-form'), $delete_route);

Then the Entity base class would call that from within a protected function, much like it does with ::linkTemplates():

  /**
   * Gets an array link templates.
   *
   * @return array
   *   An array of link templates containing paths.
   */
  protected function linkTemplates() {
    return $this->getEntityType()->getLinkTemplates();
  }

@Berdir, I was just mentioning that code to show that the patch won't necessarily break the code dawehner pointed out.

The use case I have doesn't really allow for "canonical" routes. Let me explain:

Group defines bundle entities called GroupType. On each GroupType, you can enable one or more plugins which allow you to add content to that group type's groups. Each enabled plugin generates a bundle for a GroupContent entity.

Now each of those plugins can define routes for the GroupContent entity. Most of them have the standard 'edit-form', 'canonical', etc. but each of them would preferably get their own path. So instead of group_content/{group_content}/edit, the paths would be group/{group}/member/{group_content}/edit, group/{group}/node/{group_content}/edit, etc.

One of them could also actively choose to opt out of 'delete-form' for instance. If we had "canonical routes" such as entity.group_content.delete_form, that would increase the complexity. But I guess it can be done with an extra access check _has_link_template or something.

Finally, they can each add additional link templates. So suppose two plugins define a link template called "special-stuff", but each plugin has a completely different implementation for that template, we would also get issues if the route name was just entity.group_content.special_stuff.

As you can see, it would be a big improvement to have more flexibility on the entity route names. In my case, giving the plugins their own route names would also increase the flexibility for other modules to alter just the one plugin's routes.

Just looked into this and I currently have this code in Group:

  protected function getRouteName($rel) {
    $route_prefix = 'entity.group_content.' . str_replace(':', '__', $this->getContentPlugin()->getPluginId());
    return $route_prefix . '.' . str_replace(['-', 'drupal:'], ['_', ''], $rel);
  }

Meaning that if the method got moved to the entity type, it would break my code as well because I can no longer ask an entity what plugin it's using (the entity type doesn't know this without an entity instance).

This sucks, I'm stuck :) I need varying routes because of different paths and permissions per "route clone", but I can't implement it without a crash if it comes to calling Entity::toUrl(). Even with my current implementation, it would still break in the places mentioned by dawehner.

What would you guys do in this case? Try a PathSubscriber to at least make the URLs look pretty?

On topic:
So maybe this issue needs repurposing as mentioned earlier: Just documenting the fact that every link template is expected to have an associated route name "entity.ENTITY_TYPE_ID.TEMPLATE_WITH_UNDERSCORES"? The annotation class for entity types may be a good place to document that?

On the other hand: Still putting the route name logic into its own method would mean I don't have to copy the entire Entity::toUrl() method to change 1 line. I can choose not to use the DefaultHtmlRouteProvider class and Url::fromEntityUri() method for group content entities.

After further inspection, the places where dawehner mentioned it would break are not really an issue:

• DefaultHtmlRouteProvider is optional and even mentions in its name that it's the "default". Meaning you can't expect it to cover edge cases like mine and would then have to provide your own routes.
• \Drupal\Core\Url::fromUri() is arguably bugged, actually, because it expects a "canonical" route to exist without even checking. Any module that would have a reason not to allow one of its entities to be viewed will get a crash if someone calls Url::fromUri('entity:my_unviewable_entity/1')

There could, of course, be other places where the undocumented route naming pattern is used. But right now, I don't see why we couldn't split it off into a protected method that can be overridden by custom entities.

Right, I've given this some thought and came to the following conclusion, asking to move forward with the patch from #22.

Fact: Entity types may specify a route provider and the default one core provides is DefaultHtmlRouteProvider
This means both Entity (the base class) and DefaultHtmlRouteProvider may make assumptions regarding the structure of their route names following the entity.ENTITY_TYPE.LINK_TEMPLATE pattern.

Fact: Entity types may choose to write their own route provider and -for whatever reason- could choose not to follow the aforementioned naming pattern. Especially as it isn't documented anywhere.
This should be possible, as a module may have a good reason to not follow said pattern. The patch from #22 would allow said module to be able to use Entity::toUrl() without crashing the site. I could provide several examples of why one may choose to use a different pattern.

Logical conclusion: Any non-route-providing code should not make assumptions on route names but instead ask the entity for a URL and then read the route name from that.
First mentioned by tstoeckler in #18. This means \Drupal\Core\Url::fromEntityUri() is currently bugged and should instead load the entity and read the route name from there. This is possible because it knows both the entity type and the entity ID.

Could this still go into 8.1.0 if I change the patch in #22 to:

• Add in the change to Url::fromEntityUri()
• Adjust the new method's documentation to mention:
  1. The route name assumptions are also found in DefaultHtmlRouteProvider
  2. Modules should always use $entity->toUrl()->getRouteName() to figure out the route name when an entity is available

I will update the issue summary if people agree with the above statements.

While I can follow the reasoning in #40, I disagree with the reasons stated to not make any changes. I just based a significant part of the rather huge Group module on the idea that we were free to implement custom entity route names. And in a way that makes total sense, too. I'll gladly explain it over IRC/hangouts/skype/slack/whatever if you want.

The thing with enforcing "standard" patterns is that if the pattern appears to be flawed afterwards, you have to fix it in a million places and end up not doing it at all because the task is too daunting. I mean, look at hooks: we still don't know how to fix naming collisions with those but we just ignore it because it only happens once in a blue moon. Yet when it does happen, it completely breaks a site.

So why don't we go ahead and fix this? There is no impact whatsoever for existing code. Old code will still work just fine and people using entity.$entity.$link can keep doing so until the end of times. Just those modules (like Group) that do have valid reasons to deviate from the default pattern should be able to do so.

If any contrib module, or core for that matter, does hardcode the route name pattern in a way that breaks those few modules that deviate from the pattern, we can still file a bug report then asking them to use $entity->toUrl()->getRouteName() instead.

I just spent months working on a solid D8 Organic Groups alternative that was really well received just last Saturday at Drupalcamp London. "Fixing" this issue would really help me out with removing a very ugly Entity::toUrl() overwrite that only exists to allow me to overrule the default naming pattern.

I mean, isn't that what contrib developers are beneficial to the system for? Finding bugs, flaws or DX issues in core while working on a module and then addressing those issues so Drupal can kick even more ass in the next iteration?

@tstoeckler in #43: Thanks for referring to the menu system issue. Even though it doesn't help my cause, it makes total sense to try and gain performance there.

if I understand it correctly, the use-case is to have a single relation map to different route-names based on the plugin ID

That is entirely correct, thanks for taking the time to investigate my use case!

The whole system behind my varying route names allows plugin authors to really customize "their" routes (tabs, local actions, access checks, ...) without having to worry about breaking the route for other plugins. It also helps them get "clean" URLs such as group/1/members and group/1/node/add/article instead of group/1/content/group_membership and group/1/content/add/group_node:article.

So while it surely isn't super nice, I think you can do the following:

Well, that would work for my code. But the standard entity list builder, for instance, uses edit-form and delete-form templates. The moment I "fixed" my local GroupContent::toUrl() was the moment links magically started appearing all over the place.

Still, thanks for the use case review. I really appreciate it.

Edit: I could look into the possibility of using canonical routes after all, but that would require me to set up path aliases for several routes every time a GroupContent entity is saved if I still want to keep the "clean" URLs. So that may actually make my module extremely complex for others to extend.

Okay so path aliases may work for creating clean URLs, but what about other route properties?

Right now, a group_membership and group_node overview both use the GroupContentListBuilder. If they share the same route and group_membership wants a different controller, it would also swap out the list builder for group_node. If they have separate routes, however, it would allow a plugin to swap out the controller without affecting the other plugins. Same goes for access checks, provided defaults, etc.

By the way, I'm really liking the feedback. It's got me thinking about revisiting the system I put in place, but I really need to know whether it will be as flexible using canonical routes. Having multiple routes for one link template is what currently gives Group's plugin system so much developer freedom. I'd hate to take that away from them.

I'm also contemplating something like below, but that would be a "Groupism" on top of a "Drupalism" so will probably break something somehow at some point in the future. It's basically the same idea tstoeckler had in #43.

public function toUrl($rel = 'canonical', array $options = []) {
  return parent::toUrl($this->mapLinkTemplate($rel), $options);
}

FWIW: I'm still not convinced having a hardcoded pattern all over core is the right way to move forward, but I can certainly understand the performance implications if we don't do it. The problem is that if we need to change such patterns down the line, it's already too widespread to be able to effectively do so.

I'd rather have it come from one single point, even if that were something ridiculous like EntityRouteNameProvider::getRouteName('node', 'canonical')

Wow, that's really useful information Crell. Thanks. Before I explain my use case, what would such a route filter look like? Any class I can look at specifically?

The use case of Group is this:
A Group entity is a fieldable entity with GroupType bundle entities. Then there is the fieldable GroupContent entity which allows you to add content entities into a group. Every GroupContent has two entity references among its base fields: one to reference a single group, the other to reference a single content entity.

The set up is that any content entity can be added to any group under a specific ruleset and with specific metadata (the fields on the GroupContent), e.g.:

• Add users as members, with the option of giving them roles and permissions in a group
• Add nodes with the option of making them private

Which content you can add to what groups is configured by enabling plugins on the group type. For instance, the plugin group_membership allows you to add User entities to Group entities as members.

Seeing as GroupContent entities are fieldable, you can add different fields to the "content relationship" between a group and a content entity per group type. So groups of type "Redaction" may ask their members about their writing skills, while groups of type "Playground" may want to know if their members can resist eating stuff from the sandbox.

To achieve this in a user-friendly way, a GroupContentType bundle entity is automatically generated for every plugin that is enabled. It then configures the non-group-referencing entity reference field to only allow the right content entities to be added. Furthermore, routes are generated for all GroupContent entities for that plugin.

In group_membership's case, it's:

• /group/{group}/member (collection)
• /group/{group}/member/{group_content} (canonical)
• etc.

In group_node's case, it's:

• /group/{group}/node (collection)
• /group/{group}/node/{group_content} (canonical)
• etc.

Now the key point is that every plugin generated route should be totally customizable by the plugin. They could add extra access checks, swap out the default controller, ... without affecting the route of another plugin.

Use case 1: The base plugin provides a general collection route, but the group_membership plugin may want to swap out the controller to allow for a bulk actions block at the top of the list.

Use case 2: The base plugin generates routes with a _group_permission sort of check where the site checks whether the user has the specific permissions to view GroupContent of the route configured plugin.

I've managed to achieve all this in a way that has great UX and DX: Site builders can choose exactly which content is available to which group types and what fields they want on the relationships. Developers do not need to know about the complex data structure under the hood, because they can simply write a very small plugin that will generate all of the basic structure for them.

I just gave a very well received session on this at Drupalcamp London, but sadly that recording won't be online for another week or two. The slides are here though: http://www.slideshare.net/KristiaanVandenEynde/group-drupalcamp-london-2016

The way I fixed it now, by the way is by overwriting toUrl() to match the most recent patch and then use this:

  protected function getRouteName($rel) {
    return $this->getContentPlugin()->getRouteName($rel);
  }

Which leads to this on the plugin:

  public function getRouteName($name) {
    $route_prefix = 'entity.group_content.' . str_replace(':', '__', $this->getPluginId());
    return $route_prefix . '.' . str_replace(['-', 'drupal:'], ['_', ''], $name);
  }

So entity.group_content.canonical becomes entity.group_content.group_membership.canonical, entity.group_content.group_node.canonical, etc. depending on what GroupContent::getContentPlugin() returns. And it works beautifully so far, yet requires an ugly toUrl() overwrite to do so.

I could do something like the code in #47, but that would require me to create route names like so: entity.group_content.group_node__canonical and would break massively if any plugin implementation decides to deviate from that pattern. (Because the default toUrl would then crash again because of the hardcoded pattern in there.)

Thanks for the warning about the interstitial entity. In D8 it seems to work great with views using relationships, but discussing that further may go too much off topic. If you wish to elaborate on how exactly that broke OG1, I'd gladly hear about it in a PM, over IRC, or whatever. Alternatively, I'll happily demo how it seems to work fine in D8.

To the point of this thread, it sounds like what you're ultimately trying to do is:
1) Make the path (and other route information) of an entity configurable rather than static in the source code.
2) Allow it to vary by bundle.

Well, almost. The route varies by plugin, whereas several bundles may use the same plugin. But ultimately, it boils down to the same thing: An entity having routes vary by a property on them (plugin ID instead of bundle in my case). In any case, I'm really starting to get convinced by what you've been telling me about filtering routes. Just a few more questions.

1. What about collection routes? For any route containing a GroupContent ID, I can find out the plugin ID from the loaded GroupContent. But collection routes don't have that, so how would I know how to filter /group/1/content to the right route for that path?

2. What about performance? You mention it's critical-path, so does loading an entity to read the Plugin ID from it count as "too expensive"?

Having an answer to question 1 would help me rewrite the entire routing logic in Group. Thanks a lot for the contribution so far Crell, I am learning a lot here :)

About this issue:
Should we repurpose this as "Clearly document the expected route name pattern for entities" then?