Provide a ParamConverter than can upcast any entity.

This has been needed in a couple places now.

How is that different from what we do have already: we do pass along $defaults already.

Yes, let's provide such a param converter as is described in the issue summary.

Some generic mechanism might (or might not) be cool also, but something like upcasting /{entity_type}/{entity} will surely be required in contrib and let's not have 20 different contribs write that same paramconverter. So let's do that as a first step.

Here is an example which works quite fine, to be honest.

Working on this, I found the initial problem but uncovered another.

I don't understand the IS well enough to be certain it's perfectly on-topic, but it's at least related: {entity_type}/{entity} indeed works… as long as there is a single entity in a route!

Quick Edit has an example of multiple entities in a route:

quickedit.field_form:
  path: '/quickedit/form/{entity_type}/{entity}/{field_name}/{langcode}/{view_mode_id}'
  …

{entity_type}/{entity} is one, {view_mode_id} is another. I should have been able to write {view_mode}, but if I do that, there are two entities, and it blows up. Therefore I had to rename that parameter to {view_mode_id}, and load the view mode entity manually.

{entity_type}/{entity} is one, {view_mode_id} is another. I should have been able to write {view_mode}, but if I do that, there are two entities, and it blows up. Therefore I had to rename that parameter to {view_mode_id}, and load the view mode entity manually.

Well, this is another problem space. The current code just cares about the actual upcasting logic itself, not about the super bonus magic which is done to make life easier for developers.

Indeed, #7 is a separate (valid) issue.

With this change, EditEntityAccessCheck should be completely obsolete, and the $request->attributes->set('entity', $entity); in EditEntityFieldAccessCheck can be removed.

In the interdiff, I fixed the failures, and improve EntityConverter by encapsulating the entity_type_id logic in a new method. This includes an exception if the dynamic entity type is invalid.

The rest was fixing or expanding test coverage.

Ok — as I said, I didn't know if this would be on-topic, but it's related. It might be useful to keep this in mind while solving this issue. Or maybe not. No matter — carry on :)

I *thought* that return NULL was wrong.

This looks really nice

+++ b/core/modules/editor/editor.routing.yml
@@ -11,6 +11,9 @@ editor.field_untransformed_text:
+    parameters:
+      entity:
+        type: entity:{entity_type}

+++ b/core/modules/quickedit/quickedit.routing.yml
@@ -21,6 +21,9 @@ quickedit.field_form:
+    parameters:
+      entity:
+        type: entity:{entity_type}

@@ -31,4 +34,8 @@ quickedit.entity_save:
+  options:
+    parameters:
+      entity:
+        type: entity:{entity_type}

Having read this and re-read the IS, I finally understand what this issue is about! :)

Massive hurray for getting rid of all that custom code that Quick Edit had to introduce!

However… I would like to see at least *some* comment in here to explain what this does, because 99% of developers will not understand this I fear. Something like this would already be enough:

# Inform the routing system that the {entity_type} slug contains the entity type that should be used for upcasting.

If I'm mistaken and this *is* clearly documented somewhere, then feel free to set this back to RTBC immediately.

In any case, thank you for making this so much cleaner and more maintainable!

If I'm mistaken and this *is* clearly documented somewhere, then feel free to set this back to RTBC immediately.

I'd rather suggest to document it inside the proper routing documentation on drupal.org
and maybe on the EntityConverter itself.

If you mean https://api.drupal.org/api/drupal/core%21includes%21menu.inc/group/menu/..., that documentation lives in core.api.php, and hence should be updated in this patch.

Or are you referring to other documentation?

Here is a patch which includes some documentation.

I talked about https://www.drupal.org/developing/api/8/routing

That's already much better, thanks!

11 files changed, 269 insertions, 416 deletions.

I love patches like this. ;)

So I was initially nervous because the DX that was originally discussed in the beginning of this effort (not in this issue, but in some other issue I can't find atm) was scary as heck. However, this patch is extremely targeted and only requires some extra typing in case of ambiguity, which is fair enough. Omniscience will need to be a D9 feature. ;)

In addition to obliterating a lot of complex code in e.g. quickedit module that can now rely on standard core entity access-checking, etc. this also makes it so that various functions that act on entities take an EntityInterface versus a Request, and then trying to parse the entity info out of that, resulting in much more portable code.

The only thing that raised my hackles was:

+++ b/core/lib/Drupal/Core/ParamConverter/AdminPathConfigEntityConverter.php
@@ -64,8 +64,18 @@ public function __construct(EntityManagerInterface $entity_manager, ConfigFactor
+    // If the entity type is dynamic, confirm it to be a config entity. Static
+    // entity types will have performed this check in self::applies().
+    if (strpos($definition['type'], 'entity:{') === 0) {

strpos() makes baby kittens cry. ;( And unfortunately this is done in several places throughout the code.

However, in talking to Tim it seems like this is the only way to do it. The upcasted entity itself doesn't know whether it's dynamic or not, only the route definition can say this.

I also committed a small addition to the EntityConverter PHPDoc with tim's help that provides a bit more details on when you need to worry about this, since Drupal does the right thing in the 90% case.

Apart from that, no complaints. Ergo!

Committed and pushed to 8.x. Thanks!

Thanks! See #2326707: Use dynamic entity type upcasting for comment.reply for another usage of this.