Use some arcane magic to automatically discover typed data definitions for route parameters

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterTypedDataResolversPass.phpundefined
@@ -0,0 +1,36 @@
+ * Contains Drupal\Core\DependencyInjection\Compiler\RegisterTypedDataResolversPass.

+++ b/core/lib/Drupal/Core/EventSubscriber/TypedDataSubscriber.phpundefined
@@ -0,0 +1,56 @@
+ * Contains Drupal\Core\EventSubscriber\TypedDataSubscriber.

+++ b/core/lib/Drupal/Core/TypedData/Resolver/EntityResolver.phpundefined
@@ -0,0 +1,76 @@
+ * Contains Drupal\Core\TypedData\Resolver\EntityResolver.

+++ b/core/lib/Drupal/Core/TypedData/Resolver/ReflectionResolver.phpundefined
@@ -0,0 +1,246 @@
+ * Contains Drupal\Core\TypedData\Resolver\ReflectionResolver.

Missing "\"

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterTypedDataResolversPass.phpundefined
@@ -0,0 +1,36 @@
+    if (!$container->hasDefinition('typed_data_resolver_manager')) {
+      return;
+    }

Do we really have to care about that? I know this exist in other places as well, but without it, it might be actually better to throw the exception of $container->getDefinition

+++ b/core/lib/Drupal/Core/EventSubscriber/TypedDataSubscriber.phpundefined
@@ -0,0 +1,56 @@
+use Drupal\Core\TypedData\Resolver\ResolverManager;
+use Drupal\Core\TypedData\TypedDataDetector;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Drupal\Core\Routing\RoutingEvents;
+use Drupal\Core\Routing\RouteBuildEvent;

Let's order them properly, or at least put symfony somewhere else.

+++ b/core/lib/Drupal/Core/EventSubscriber/TypedDataSubscriber.phpundefined
@@ -0,0 +1,56 @@
+  public function onRoutingRouteAlterSetAccessCheck(RouteBuildEvent $event) {
...
+    $events[RoutingEvents::ALTER][] = array('onRoutingRouteAlterSetAccessCheck', 20);

That method name is a copy and paste from the access one. A better name would seriously helpful.

+++ b/core/lib/Drupal/Core/EventSubscriber/TypedDataSubscriber.phpundefined
@@ -0,0 +1,56 @@
+  /**
+   * Registers the methods in this class that should be listeners.
+   *
+   * @return array
+   *   An array of event listener definitions.
+   */

Haven't I reviewed that bit before? @inheritdoc

+++ b/core/lib/Drupal/Core/EventSubscriber/TypedDataSubscriber.phpundefined
@@ -0,0 +1,56 @@
+    // Setting very low priority to ensure typed data discovery runs after

Missing dot.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.phpundefined
@@ -55,6 +58,7 @@ class ParamConverterManager extends ContainerAware implements RouteEnhancerInter
+    $converter = substr($converter, strlen('paramconverter.'));

@@ -181,5 +163,27 @@ public function enhance(array $defaults, Request $request) {
+    return $this->converters[$converter] = $this->container->get("paramconverter.$converter");

So we remove it to be able to add later?

+++ b/core/lib/Drupal/Core/TypedData/Resolver/EntityResolver.phpundefined
@@ -0,0 +1,76 @@
+      list ($entity_type) = explode('.', $defaults['_entity_form']);

I kind of prefer strotk instead.

+++ b/core/lib/Drupal/Core/TypedData/Resolver/ReflectionResolver.phpundefined
@@ -0,0 +1,246 @@
+   * @param TypedDataManager $typed_data_manager

Should have the full namespace.

+++ b/core/lib/Drupal/Core/TypedData/Resolver/ReflectionResolver.phpundefined
@@ -0,0 +1,246 @@
+    $match = NULL;
...
+    return $match ?: NULL;

I don't see the need for the ternary here.

+++ b/core/tests/Drupal/Tests/Core/TypedData/ReflectionTestController.phpundefined
@@ -0,0 +1,48 @@
+  public function nodeController(Node $node) {
...
+  public function nodeUserController(Node $node, User $user) {
...
+  public function blockController(BlockInterface $block) {
...
+  public function blockNodeController(Node $node, BlockInterface $block) {
...
+  public function buildForm(array $form, array &$form_state, BlockInterface $block = NULL, Node $node = NULL) {

Is there any reason why we don't use the entity_test entity types instead? There are multiple of them.

I am filing as feature request although that makes me feel kinda uneasy as I hope we can still get this in for D8

Retitling the part of this that I think qualifies as a task, not a feature. Plus, I think it's major, due to the potential for collisions between entity type names and local parameter names once contrib is factored in, as explained in #1906810-20: Require type hints for automatic entity upcasting.

Also, adding the DX tag, since I think that current HEAD's approach of making assumptions about the type of a parameter based solely on its name is a WTF that doesn't match any conventional programming principles. Whereas, this issue's suggestion to use a PHP type hint to guide decisions about what type to upcast to both makes logical sense, and has precedent in Symfony.

I'm also retitling this issue to focus on entities for clarity of purpose and scope. If the solution here ends up working generically for all of TypedData, fine, but I don't think that's a requirement. We have #1906810: Require type hints for automatic entity upcasting for that.

Finally, I see nothing about this issue that would require it to be done before beta. There might be some minor API changes needed, but nothing that would affect any but a very tiny percentage of contrib modules, so I think it's in scope right up until RC.

If anyone disagrees with any of the above, please say so. Thanks.

We agreed a long time ago that #1906810: Require type hints for automatic entity upcasting won't be a generic solution but a specific one just for entity upcasting,
so I wonder whether we really want to keep this issue, or post the last patch from the other issue here and continue it here.

The other issue switched from generic TypedData to entities anyway, so moving this back.

I am really not sure whether this issue is helpful any longer.

I am really not sure whether this issue is helpful any longer.

This seems like it should now be a follow-up to #1906810: Require type hints for automatic entity upcasting. That one is, I think, keeping the current name-based logic for deriving the type but also adding a requirement that the controller type hint properly. Next step is to make it use ONLY the type hint, which presumably is this issue.

I'm going to wait for that issue to land, but I don't think @Crell accurately described the goal of the issue.
I need to look at what I have for page manager and what @EclipseGc has from a while back, and see if this issue is still 100% needed.

Can we get an update here? #1906810: Require type hints for automatic entity upcasting may have helped, but it's not clear to me what part of the problem remains.

Let's either close this issue or make clear what still needs to happen.

E.g. the one case that I remember being very frustrating and a huge WTF:

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

I had to type {view_mode_id} instead of {view_mode}, otherwise Drupal would try to load the corresponding entity, which caused Drupal to WSOD IIRC.

E.g. the one case that I remember being very frustrating and a huge WTF:

That particular problem was fixed with #1906810: Require type hints for automatic entity upcasting

This issue was more about doing crazy things on typed data level, but I don't think things like LoadableInterface will have a chance to happen.

I tested and confirmed that it is now possible to write {view_mode} instead of {view_mode_id} without crashing Drupal. So, from that POV, this is a duplicate.

Should we close this issue?

We can chat at devdays though.

I'd like to chat with you about things, indeed!

enhancing the paramconverter resolving based on the slug name with reflection (looking at the typehint of the controller arguments and their names)

Well, this is basically what we do now, but just on the entity level. Let's mark this issue as won't fix, okay?