Improve ParamConverterManager and developer experience for ParamConverters

FYI http://drupal.org/documentation/git/interdiff

+++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
@@ -57,49 +56,46 @@ public function __construct(EntityManager $entityManager) {
-  public function process(array &$variables, Route $route, array &$converted) {
+  public function process(array $defaults, Route $route, array &$converted) {

No. We deliberately changed it from defaults to variables because we wanted to decouple this system from Route enhancers. $defaults makes no sense in any other context. (It barely makes sense in that context, frankly.) Leave it at $variables.

That said, accepting a variable and then returning rather than passing by reference I fully support.

I'm also wondering why we need the converted tracking. Why can't we just say "once it's an object, other converters are required to ignore it, period"?

Otherwise this looks reasonable. I would like to move forward with this patch regardless of typed data upcasting, although I agree with getting reviews on that one, too.

I'm also wondering why we need the converted tracking. Why can't we just say "once it's an object, other converters are required to ignore it, period"?

Well, the Views UI converter can only operate if the Entity converter has run. So it skips that check, unlike the Entity converter.

The current patch doesn't change that, I'm just ensuring that we don't break it by enforcing what @Crell suggested.

Views UI has its own converter? This is news to me...

How are values in an array not variables? :-) They may be objects, but they're always variables.

fubhy_: Uh. I think you're horribly confused. :-) There is *no difference* between what Symfony calls $defaults and what our ParamConverters call $variables except that $defaults doesn't make sense as a name except in the route property context. The distinction you're drawing between "slug converters' and "defaults" is nonexistent. Symfony doesn't differentiate between values that come from placeholders and from the route defaults array. They're all the same by the time they get to a route enhancer.

Uploading my patch from #1798214-135: Upcast request arguments/attributes to full objects. I think it addresses much of what's being discussed here.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.php
@@ -61,27 +61,31 @@
+    // For each parameter without explicit options, default its type to the
+    // parameter name.
+    foreach ($route->compile()->getVariables() as $parameter_name) {
+      if (!isset($parameter_options[$parameter_name])) {
+        $parameter_options[$parameter_name] = array('type' => $parameter_name);
+      }
+    }

I want to particularly call out this part. What it does is move the magic defaulting of (entity) type from slug name from EntityConverter to ParamConverterManager. I actually wrote that with #1906810: Require type hints for automatic entity upcasting initially in mind, though reading recent comments on that issue, it might be the case that simply defaulting a 'type' might not be enough. I need to delve into that issue a bit more to understand what other metadata defaults would be needed.

With ViewUIConverter converted.

interdiff from 15 to 19.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterInterface.php
@@ -17,13 +17,24 @@
-   *   altered by a converter.
+   * @param array $defaults
+   *   The complete array of parameters returned by
+   *   \Symfony\Component\Routing\Matcher\RequestMatcherInterface::matchRequest()
+   *   and processed by prior route enhancers and parameter converters.
+   *

Passing the original array in would allow for things like upcasting one value based on another no? (I recall someone asking about that; I think Klausi?)

From a read through this seems sensible, but there may be details I'm missing. :-)

This looks infinitely preferable to #1906810: Require type hints for automatic entity upcasting.
Rerolled for the Views UI move, and added an isset (not all placeholders need upcasting)

Thanks guys! Sorry for dropping the ball on this after #19. Very happy that you picked it up.

I can't speak to the internal changes much, but the new EntityConverter code is very easy to understand. I like the idea of mimicking AccessChecks.

+++ b/core/modules/views_ui/lib/Drupal/views_ui/ParamConverter/ViewUIConverter.phpundefined
@@ -10,7 +10,6 @@
-use Drupal\views\ViewStorageInterface;

@@ -36,72 +35,40 @@ public function __construct(TempStoreFactory $temp_store_factory) {
+      if ($value->status()) {
+        $view->enable();
       }
+      else {
+        $view->disable();
+      }
+      $view->lock = $temp_store->getMetadata($value->id());
...
+    else {
+      $view = new ViewUI($value);

This is all Views specific stuff, so I'm not surre why we don't check for the interface anymore.

+++ b/core/modules/views_ui/views_ui.routing.ymlundefined
@@ -68,12 +80,18 @@ views_ui.autocomplete:
 views_ui.edit:
   pattern: '/admin/structure/views/view/{view}'
   options:
-    tempstore:
-      view: 'views'
+    parameters:
+      view:
+        entity: 'view'
+        tempstore: 'views'

This is cool, and makes sense

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.phpundefined
@@ -24,66 +23,134 @@
+  public function setConverters(RouteCollection $routes) {
+    foreach ($routes as $route) {
+      $converters = $this->applies($route);
+      if (!empty($converters)) {
+        $route->setOption('_parameter_converters', $converters);
+      }
+    }
   }

Probably not for this issue, but could that lead us to a point where individual routes could specify custom, ad-hoc converters for their params ? Like, this line could support existing 'parameter_converters' specified in the route definition rather than just ovewriting the option ?

From what @tim.plunkett explained to me on IRC, the current situation is that custom converters are discouraged because we currently load *all* existing converters on all requests.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.phpundefined
@@ -24,66 +23,134 @@
+  protected function applies(Route $route) {
+    $converters = array();

Not sure having the same method name than ParamConverterInterface::applies() is a good idea. The methods are related, and the verb works well in the case of ParamConverterInterface, but not really on the manager. Maybe something like getApplicableConverters() ?

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.phpundefined
@@ -24,66 +23,134 @@
+    $parameters = $route->getOption('_parameter_converters') ?: array();

Misleading var name, those are converters keyed by parameter name, not parameters themselves.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.phpundefined
@@ -24,66 +23,134 @@
+        if (empty($this->converters[$service_id])) {
+          $this->loadConverter($service_id);
+        }

If I get the code flow right, it seems the converters will always be loaded in ParamConverterManager::applies() ?

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.phpundefined
@@ -24,66 +23,134 @@
+  /**
+   * Lazy-loads access check services.
+   *
+   * @param string $service_id
+   *   The service id of the access check service to load.
+   *
+   * @throws \InvalidArgumentException
+   *   If the given service has not been registered as a converter.
+   */
+  protected function loadConverter($service_id) {
+    if (!in_array($service_id, $this->converterIds)) {

I guess the phpdoc is copy/pasted from access services and needs to be adjusted ? ;-)

AccessCheckInterface::applies() are called by AccessManager::applies(), so that pattern follows.

(yched) Probably not for this issue, but could that lead us to a point where individual routes could specify custom, ad-hoc converters for their params ? Like, this line could support existing 'parameter_converters' specified in the route definition rather than just ovewriting the option ?
(fubhy) Are we talking about closures here? Interesting idea, but out of scope for this issue indeed. Not sure if we want closures for this though as that would bring us back to a place where the final value would be totally unpredictable.

Not sure how closures relate to this ? My point was to have a way to specify "for *this* param of *this* route, use MyVerySpecificConverter, but don't bother loading the class for any other route that doesn't explicitly specify it". Something like:

    parameters:
      view:
        converter: 'my.custom.converter'
        some_custom_option: 'foo'

Converters being currently loaded on all routes de facto means custom ad-hoc converters are discouraged, since loading them all would be a performance drag when contrib goes wild. In that sense, adding a custom converter is being a "bad citizen", since it impacts everyone outside of my specific case, and things would be a mess if everyone did the same. "Extensible by being a bad citizen" doesn't really match "extensible" :-)
This in turn forces us to come up with a limited set of generic converters that need to fit 99% cases (and are tried successively on each argument until one works), since adding a custom one is a "heavy" choice. This in turn means that we don't really control our URLs (as #1946404: Convert forms in field_ui.admin.inc to the new form interface showed, where admin/structure/types/manage/article/fields/field_foo had to be turned to admin/structure/types/manage/article/fields/node.article.field_foo).

Sorry if this adds noise here - then again it seems kind of related to your "one converter per argument" line of thought above.

This requires knowledge of the paramconverter service id so not sure about DX here but technically this feels more stable.

Why is it technically more stable? I think the DX is worse, and that the earlier patch, where a converter applies based on which keys are used (e.g., 'paramconverter.entity' is used if the parameter uses an 'entity_type' option) was cleaner.

only allow a single converter per argument (I still think that nesting those might get us into more trouble then it would benefit us)

I haven't thought about the pros/cons of this, but even if we want it, can't we achieve it via priorities? So, of all the converters that return TRUE for applies(), use the highest priority one?

Agreed that

parameters:
      view:
        converter: 'my.custom.converter'
        some_custom_option: 'foo'

is worse DX, but only if specifying a converter service ID is turned into a requirement for all routes/parameters - but that doesn't have to be the case ?

We'd want to *allow* specific routes to specify a custom converter, most likely an ad-hoc converter built specifically for the route (or set of routes), in which case knowing the service id of the converter you just custom coded is not a real drag.

This doesn't prevent us from shipping "default converters that cover the 80% use cases" like direct value passing or the current EntityConverter, that would still fire by default as they currently do, when no explicit converter is specified for the argument ?

I like #34, although the stacking is awkward and feels like the responsibility of a converter.

I agree with #48 that you should be able to specify a custom callback. It could follow the same conventions as _controller.

I also don't see a use case for more than one key on a parameter. Having a simple 1:1 mapping like parameters: {view: 'my.custom.converter'} makes sense to me.

This would be the first time we're coupling routes to the DIC, which makes me very uneasy. I'm almost leaning toward #32's approach there instead, but not firmly decided. Other than that, and the verbosity of the route definition, #42 is actually looking pretty good.

I don't know whether #51.3 will end up being too limiting or not, but I think it's fine to start off that way, so +1 to #51.

Agreed on #51.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterInterface.php
@@ -15,15 +16,40 @@
+   * Allows to convert parameters to their corresponding objects.

"Converts a parameter to its corresponding object."

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.php
@@ -24,66 +23,142 @@
+  public function setConverters(RouteCollection $routes) {
+    foreach ($routes as $route) {
+      $converters = $route->getOption('_converters') ?: array();
+      $parameters = $route->getOption('_parameters') ?: array();
+      foreach ($parameters as $parameter => $options) {
+        // Do not override manually registered converters.
+        if (!empty($converters[$parameter])) {
+          continue;
+        }
+
+        if ($service_id = $this->applies($route, $parameter, $options)) {
+          $converters[$parameter] = $service_id;
+        }
+      }
+
+      if (!empty($converters)) {
+        $route->setOption('_converters', $converters);
+      }

Am I reading this correctly that we'd still allow for "automatic" conversion, since a converter can decide if it should apply if the route itself hasn't claimed one? I'm a bit confused here.

Edit: Wait, I think I get it. You can specify "convert with this service ID" with _converters, OR specify "this should be object type X" with _parameters, and the latter will get turned into the former when building routes. Do I have that right?

OK, that makes sense to me. We just need to document it well, and note that using _parameters is the preferred mechanism.

Overall, +1, but how about...

+++ b/core/modules/aggregator/aggregator.routing.yml
@@ -18,6 +19,9 @@ aggregator_feed_items_delete:
+    _parameters:
+      aggregator_feed: 'entity.aggregator_feed'

1. Do we need 'parameters' to have a leading "_"? The leading underscores are needed in subkeys of 'defaults' and 'requirements', since those can also contain URL parameter names. But within 'options', I'm not clear what the purpose of the "_" is.

2. I suggest one more level of nesting after the parameter name. Like this:

parameters:
  aggregator_feed: 
    type: 'entity.aggregator_feed'

Or:

parameters:
  aggregator_feed: 
    entity_type: 'aggregator_feed'

Because...

+++ b/core/modules/views_ui/views_ui.routing.yml
@@ -71,35 +80,35 @@ views_ui.autocomplete:
+    _parameters:
+      view: 'views.tempstore'

This can then be:

parameters:
  view:
    entity_type: 'view'     # or type: 'entity.view'
    tempstore: 'views'

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.php
@@ -24,66 +23,142 @@
+      $converters = $route->getOption('_converters') ?: array();

And then _converters wouldn't need to be a separate option, but could be part of each parameter's definition. For example:

parameters:
  view:
    entity_type: 'view'     # or type: 'entity.view'
    tempstore: 'views'
    converter: 'my_custom_module.views_converter'

+++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
@@ -35,72 +35,25 @@ public function __construct(EntityManager $entityManager) {
+  public function convert($parameter, $value, array $defaults, Request $request, Route $route) {
+    // Extract the entity type from the route options.
+    $options = $route->getOption('_parameters');

Since $options will likely be needed by all converters, let's pass it as a parameter to convert(), after $value. The rest: $defaults, $request, and $route should rarely be needed. In fact, let's make it $parameter_options (i.e., the manager passes it $options[$parameter]).

I am pretty sure we DON'T want to allow multiple layers of param converters stacked on top of each other

I didn't mean to imply that multiple option keys means multiple converters. I'm assuming in that example that my_custom_module.views_converter has a high priority and therefore wins. But that perhaps that converter might also be interested in those other keys (maybe the custom converter is flexible enough to be reusable for other entity types and tempstores).

So, simply going with something like "type: 'tempstore.views'" should be sufficient

Are objects loaded from tempstore collections always of a different class than entities loaded from permanent storage? If so, then I'm okay with type: "entity.view" and type: "tempstore.views" syntax, since they are in fact different types. If the objects can be of the same class though, and only differ by storage location, then I'd want to be cautious about overloading type to mean more than just the type.

One more request on this. Can we change type: 'entity.aggregator_feed' to type: 'entity:aggregator_feed' (i.e., just change the "." to a ":")? The reason is that I still would like to leave open the possibility that we do #1906810: Require type hints for automatic entity upcasting after July 1, which we can do as long as it's not API breaking. And ":" is the standard plugin derivative delimiter, so using it now gives us the ability to later swap in a TypedData based converter without breaking any APIs.

Not sure how that tag got removed.

I like the new, simplified direction that we're taking with this. The syntax seems a lot easier. Looking forward to a patch; hopefully it comes pretty soon so we can still get it in.

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -81,11 +81,7 @@ public function __construct(array $values, $entity_type) {
-    $entity_type = substr($definition['id'], strlen('entity:'));
+    $entity_type = substr($definition['type'], strlen('entity:'));

The type also includes the bundle if it's known, e.g. when calling getType() on a loaded entity.

+++ b/core/lib/Drupal/Core/ParamConverter/TypedDataConverter.phpundefined
@@ -37,18 +38,48 @@ public function __construct(TypedDataManager $typed_data_manager) {
+    if ($context->validate()) {

validate returns an object that contains the number of validations, see EntityValidionTest

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterParamConvertersPass.phpundefined
@@ -23,26 +23,14 @@ class RegisterParamConvertersPass implements CompilerPassInterface {
-    $services = array();
     foreach ($container->findTaggedServiceIds('paramconverter') as $id => $attributes) {
       $priority = isset($attributes[0]['priority']) ? $attributes[0]['priority'] : 0;
-
-      $services[$priority][] = new Reference($id);
-    }
-
-    krsort($services);
-
-    foreach ($services as $priority) {
-      foreach ($priority as $service) {
-        $manager->addMethodCall('addConverter', array($service));
-      }
+      $manager->addMethodCall('addConverter', array($id, $priority));

The sorting is now done in the actual manager, so this is a just a cleanup.

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterTypedDataResolversPass.phpundefined
@@ -0,0 +1,36 @@
+ * Contains Drupal\Core\DependencyInjection\Compiler\RegisterTypedDataResolversPass.
...
+  public function process(ContainerBuilder $container) {
+    if (!$container->hasDefinition('typed_data_resolver_manager')) {
+      return;
+    }
+
+    $manager = $container->getDefinition('typed_data_resolver_manager');
+    foreach ($container->findTaggedServiceIds('typed_data_resolver') as $id => $attributes) {
+      $priority = isset($attributes[0]['priority']) ? $attributes[0]['priority'] : 0;
+      $manager->addMethodCall('addTypedDataResolver', array(new Reference($id), $priority));

Follow up: This seems to be a really common pattern so somehow this could be moved to a common baseclass on the longrun.

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -77,6 +78,26 @@ public function __construct(array $values, $entity_type) {
+
+    if (!$storage = $container->get('plugin.manager.entity')->getStorageController($entity_type)) {
+      throw new \InvalidArgumentException(sprintf('Could not retrieve storage controller for the %s entity type', $entity_type));
+    }

Follow up: Throw exceptions in the entity manager instead.

+++ b/core/lib/Drupal/Core/EventSubscriber/ParamConverterSubscriber.phpundefined
@@ -0,0 +1,58 @@
+class ParamConverterSubscriber implements EventSubscriberInterface {

+++ b/core/lib/Drupal/Core/EventSubscriber/TypedDataSubscriber.phpundefined
@@ -0,0 +1,56 @@
+class TypedDataSubscriber implements EventSubscriberInterface {

It is a bit hard to understand why we need both param converter subscriber and type data resolver subscriber, because the param converter should know of these typed data resolvers? The problem is simply that it is basically impossible for a user to understand what is going on.

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

Simply use {@inheritdoc}

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterInterface.phpundefined
@@ -17,13 +18,34 @@
+  public function convert($definition, $name, array $defaults, Request $request);

As far as I leared it, the rule is to put the most changing variable to the front, which is probably request or defaults, and then the other ones. Or in other words: the signature tells to convert variables, so these variables are stored in defaults/request so let's start with them. It just makes it easier to understand.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterInterface.phpundefined
@@ -17,13 +18,34 @@
+  public function applies($definition, $name, Route $route);

Similar argumentation as before.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.phpundefined
@@ -24,66 +23,165 @@
+  public function setRouteParameterConverters(RouteCollection $routes) {
+    foreach ($routes as $route) {

Let's use $routes->all() instead of the magic.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.phpundefined
@@ -24,66 +23,165 @@
+      if (!$parameters = $route->getOption('parameters') ?: array()) {
...
+    if (!$parameters = $route->getOption('parameters') ?: array()) {

Is there any reason for this ?: array() operator? I don't see why tbh. It makes it damn hard to read.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.phpundefined
@@ -24,66 +23,165 @@
+   *   If a variable has been converted to NULL.

Can we describe what NULL means? (it could not been upcasted)

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.phpundefined
@@ -24,66 +23,165 @@
+  protected function getConverter($converter) {
+    if (isset($this->converters[$converter])) {
+      return $this->converters[$converter];
+    }
+    if (!in_array($converter, $this->getConverterIds())) {
+      throw new \InvalidArgumentException(sprintf('No converter has been registered for %s', $converter));
+    }
+    return $this->converters[$converter] = $this->container->get("paramconverter.$converter");

So this is just a similar pattern what the access manager does at all. Lazy load stuff and throw an exception as early as possible.

+++ b/core/lib/Drupal/Core/ParamConverter/TypedDataConverter.phpundefined
@@ -0,0 +1,76 @@
+ * Contains Drupal\Core\ParamConverter\TypedDataConverter.

This probably should live in the typed data directory.

+++ b/core/lib/Drupal/Core/ParamConverter/TypedDataConverter.phpundefined
@@ -0,0 +1,76 @@
+  public function convert($definition, $name, array $defaults, Request $request) {

Could definition be something else then an array?

+++ b/core/lib/Drupal/Core/ParamConverter/TypedDataConverter.phpundefined
@@ -0,0 +1,76 @@
+    if (isset($definition[$key])) {
+      $class = $definition[$key];
+    }
+    elseif (isset($original[$key])) {
+      $class = $original[$key];
+    }

And if nothing exists throw maybe an exception?

+++ b/core/lib/Drupal/Core/ParamConverter/TypedDataConverter.phpundefined
@@ -0,0 +1,76 @@
+    // If the given typed data type is not loadable, just create an instance.
+    if (!is_subclass_of($class, 'Drupal\Core\TypedData\LoadableInterface')) {
+      return $this->typedDataManager->create($definition, $defaults[$name]);
+    }
+
+    // Try to load the typed data object via the typed data manager.
+    return $this->typedDataManager->load($defaults[$name], $definition);

My personal feeling would be that this logic is connected with the typed data manager itself, so should live there.

+++ b/core/lib/Drupal/Core/TypedData/Resolver/EntityResolver.phpundefined
@@ -0,0 +1,74 @@
+    // Check if there is a variable that matches the provided entity type.
+    if (!in_array($entity_type, $route->compile()->getVariables())) {
+      return;
+    }

It feels like this line is potentially a good way to break stuff. It just returns and does nothing. Can't we set the type on the route and then let the converter fail? This feels also wrong, but maybe provides better and faster developer feedback.

+++ b/core/lib/Drupal/Core/TypedData/Resolver/ReflectionResolver.phpundefined
@@ -0,0 +1,123 @@
+    if (is_array($callable)) {
+      $reflection = new \ReflectionMethod($callable[0], $callable[1]);
+    }
+    elseif (is_object($callable) && !$callable instanceof \Closure) {
+      $reflection = new \ReflectionObject($callable);
+      $reflection = $reflection->getMethod('__invoke');
+    }
+    else {
+      $reflection = new \ReflectionFunction($callable);

Do we need this at all? $callable will never be an object or a closure according to self::resolveParameterTypes

+++ b/core/lib/Drupal/Core/TypedData/Resolver/ReflectionResolver.phpundefined
@@ -0,0 +1,123 @@
+    // Ignore request object type hints.
...
+    $request = 'Symfony\Component\HttpFoundation\Request';
+    if ($name == $request || $class->isSubclassOf($request)) {
+      return;

Is there an explicit need to check that? It seems to be that this is already covered by the rest of the code below, but it is maybe useful to optimize this because of the performance.

+++ b/core/lib/Drupal/Core/TypedData/Resolver/ResolverManager.phpundefined
@@ -0,0 +1,95 @@
+/**
+ * Generates typed data definitions for route parameters.
+ */
+class ResolverManager {

Can we explain a bit better the usecases for the resolver resolver manager? (not only upcasting but also being able to store the information so things like princess can use it).

+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.phpundefined
@@ -45,7 +48,14 @@ class TypedDataManager extends DefaultPluginManager {
+  public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, LanguageManager $language_manager, ModuleHandlerInterface $module_handler, ContainerInterface $container) {

@@ -53,6 +63,14 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
+
+  /**
+   * {@inheritdoc}
+   */
+  public function setContainer(ContainerInterface $container = NULL) {
+    $this->container = $container;
   }

Do we need both injection via constructor and setter?

+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.phpundefined
@@ -382,4 +400,48 @@ public function getConstraints($definition) {
+  public function load($id, array $definition) {

This full method has a lot of the logic of TypedDataConverter, is there a need for that?

+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.phpundefined
@@ -382,4 +400,48 @@ public function getConstraints($definition) {
+
+    // The instance class has to implement the LoadableInterface.
+    if (!in_array('Drupal\Core\TypedData\LoadableInterface', class_implements($class))) {
+      throw new \InvalidArgumentException('The given class has to implement the LoadableInterface.');
+    }

Is there any usecase where you want to have a typed data on a route without being available via being loadable?

+++ b/core/modules/views_ui/lib/Drupal/views_ui/ParamConverter/ViewUIConverter.phpundefined
--- a/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php
+++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.phpundefined

There is an explicit test: ViewUIObjectTest, so please add a test coverage as well.

+++ b/core/tests/Drupal/Tests/Core/TypedData/EntityResolverTest.phpundefined
@@ -0,0 +1,110 @@
+  /**
+   * Tests \Drupal\Core\TypedData\Resolver\ResolverManager::resolveParamaterTypes().
+   *
+   * @dataProvider providerTestParameters

I like to use @see, so you can directly access it on api.drupal.org

+++ b/core/tests/Drupal/Tests/Core/TypedData/EntityResolverTest.phpundefined
@@ -0,0 +1,110 @@
+  public function providerTestParameters() {

Let's also provide a route without _entity_form or _entity_type but with a node ...

+++ b/core/tests/Drupal/Tests/Core/TypedData/EntityResolverTest.phpundefined
@@ -0,0 +1,110 @@
+
+    $this->entityResolver = $this
+      ->getMockBuilder('Drupal\Core\TypedData\Resolver\EntityResolver')
+      ->disableOriginalConstructor()
+      ->setMethods(NULL)
+      ->getMock();
+
+    $property = new \ReflectionProperty('Drupal\Core\TypedData\Resolver\EntityResolver', 'typedDataManager');
+    $property->setAccessible(TRUE);

+++ b/core/tests/Drupal/Tests/Core/TypedData/ReflectionResolverTest.phpundefined
@@ -0,0 +1,117 @@
+
+    $this->reflectionResolver = $this
+      ->getMockBuilder('Drupal\Core\TypedData\Resolver\ReflectionResolver')
+      ->disableOriginalConstructor()
+      ->setMethods(NULL)
+      ->getMock();
+
+    $property = new \ReflectionProperty('Drupal\Core\TypedData\Resolver\ReflectionResolver', 'typedDataManager');
+    $property->setAccessible(TRUE);
+    $property->setValue($this->reflectionResolver, $this->typedDataManager);

I don't get why you have to mock the typed data manager, as it is just a simple constructor argument.

+++ b/core/tests/Drupal/Tests/Core/TypedData/ReflectionResolverTest.phpundefined
@@ -0,0 +1,117 @@
+  public function providerTestParameters() {

Same as before.

+++ b/core/tests/Drupal/Tests/Core/TypedData/ResolverManagerTest.phpundefined
@@ -0,0 +1,133 @@
+    $resolver = $this->getMock('Drupal\Core\TypedData\Resolver\ResolverInterface');
+    $resolver->expects($this->exactly($collection->count()))
+      ->method('resolveParameterTypes')
+      ->with($this->isInstanceOf('Symfony\Component\Routing\Route'))
+      ->will($this->returnValue(array('magic' => 'yes')));
+
+    $manager = $this->getMockBuilder('Drupal\Core\TypedData\Resolver\ResolverManager')
+      ->setMethods(array('getTypedDataResolvers'))
+      ->getMock();

It would make sense to setup the manager at least, in a setup method.

+++ b/core/tests/Drupal/Tests/Core/TypedData/ResolverManagerTest.phpundefined
@@ -0,0 +1,133 @@
+        ->setMockClassName($data['name'])

Can you explain why we need this here?

Discussed this with @EclipseGC and @Crell...

I don't think we should be doing magic guessing in the resolvers here... doing this will reduce risk and complexity. So for example, the user_role_edit route should look something like this:

user_role_edit:
  pattern: '/admin/people/roles/manage/{user_role}'
  defaults:
    _entity_form: user_role.default
  requirements:
    _entity_access: user_role.update
  options:
    user_role:
      type: Entity:user_role

The point being that when the developer writes a route they exactly what their slugs should be upcast to... Drupal guessing for them is complex and risky.

Belated partial review of #91:

+++ b/core/core.services.yml
@@ -208,7 +208,25 @@ services:
     class: Drupal\Core\TypedData\TypedDataManager
     arguments: ['@container.namespaces', '@cache.cache', '@language_manager', '@module_handler']
     calls:
+      - [setContainer, ['@service_container']]
       - [setValidationConstraintManager, ['@validation.constraint']]

Typed Data Manager doesn't seem like it has a reason to be container aware...

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterInterface.php
@@ -17,13 +18,34 @@
   /**
    * Allows to convert variables to their corresponding objects.

Side note: As long as we're here we should fix this German idiom. :-) It's not valid English.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterInterface.php
@@ -17,13 +18,34 @@
+   * @return mixed|null
+   *   The converted parameter value.

Document here what a null return means.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.php
@@ -24,66 +23,166 @@
+    $converter = substr($converter, strlen('paramconverter.'));

I know we're doing it in one or two other places, but this is a really crappy way of doing converter IDs. If we can do something else, we should.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.php
@@ -24,66 +23,166 @@
+    foreach ($routes->all() as $route) {

I disagree with Daniel on this. :-) $routes is iteratable for a reason. It's totally reasonable to iterate a collection.

The overall architecture in #105 looks good to me. However, to get the DX right, I suggest splitting this into 3 parts:

- Part 1, this issue: Just the change to ParamConverterManager and ParamConverterInterface, but leave EntityConverter and ViewUIConverter in tact, only changing them as needed to conform to the updated interface.
- Part 2, #1983100: Provide a LoadableInterface for Typed Data objects.
- Part 3, replacing EntityConverter and ViewUIConverter with TypedDataConverter. Perhaps reopening #1906810: Require type hints for automatic entity upcasting and doing it there.

What do you think? Otherwise, my concern is that there's too many intricate pieces being changed here, and not enough analysis on whether each piece is truly necessary or what the DX impact of it is. For example, whether LoadableInterface is really necessary, and whether a 'class' override in the routing.yml file is the most logical way of loading from the tempstore.

Thanks for being willing to split this up. I'm still in favor of getting the whole set of issues in, including #2041907: Use some arcane magic to automatically discover typed data definitions for route parameters, but this way, each one can get properly detailed and focused reviews.

+++ b/core/modules/action/action.routing.yml
@@ -18,6 +18,10 @@ action_admin_configure:
+  options:
+    parameters:
+      action:
+        type: 'entity:action'

Since in #2041907: Use some arcane magic to automatically discover typed data definitions for route parameters our plan is to make explicit type declaration not needed in 90+% of cases, I'm concerned about making them needed as an interim step, since we're technically past API freeze already, and routing.yml files are an API that affects nearly every contrib module. I think within this issue we should retain the name/type correspondence automagic, despite the flaws with it pointed out in #1906810-20: Require type hints for automatic entity upcasting, and remove it in #2041907: Use some arcane magic to automatically discover typed data definitions for route parameters. That way, we provide minimal disturbance to contrib modules who've begun early porting.

diff --git a/core/modules/system/lib/Drupal/system/Tests/ParamConverter/UpcastingTest.php b/core/modules/system/lib/Drupal/system/Tests/ParamConverter/UpcastingTest.php
deleted file mode 100644

Why remove this test?

Here's a patch that addresses the above. I haven't yet reviewed the rest in depth, but wanted to get this up here in the meantime.

Also, removing the "needs tests" tag, since I think restoring the original tests, along with the new ParamConverterManagerTest, is sufficient. Please correct me though if you think there's something else that needs a test.

Ok, finished reviewing the rest. Only found nits.

+++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
@@ -30,74 +29,34 @@ class EntityConverter implements ParamConverterInterface {
+      return $this->entityManager->getDefinition($entity_type);

Let's cast this to a boolean before returning it.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterInterface.php
@@ -17,13 +18,34 @@
+  public function convert($definition, $name, array $defaults, Request $request);

What about adding $value as a first parameter? That way, most converters wouldn't even need to deal with $defaults.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.php
@@ -11,79 +11,175 @@
+    if (!$parameters = $route->getOption('parameters') ?: array()) {
+      return $defaults;
     }

Since we're returning, we don't need the ?:.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.php
@@ -11,79 +11,175 @@
+    foreach ($parameters as $parameter => $definition) {

Can we change $parameter to either $name or $parameter_name?

+++ b/core/modules/views_ui/lib/Drupal/views_ui/ParamConverter/ViewUIConverter.php
@@ -7,23 +7,38 @@
-  protected $tempStoreFactory;
+  protected $tempStore;

This seems wrong.

Thanks. This looks great to me now, but due to #108, it would probably be best for someone else to RTBC.

This patch looks pretty cool. Here are a few things, shoot them down as necessary :)

+++ b/core/core.services.ymlundefined
@@ -292,13 +292,25 @@ services:
+  paramconverter_subscriber:
...
+  route_subscriber.entity:

Seems like we should settings on our naming conventions if we can. We currently mix underscores and '.' in service names. Out of scope of this issue, but probably worth talking about.

+++ b/core/lib/Drupal/Core/EventSubscriber/EntityRouteAlterSubscriber.phpundefined
@@ -0,0 +1,78 @@
+      $parameter_definitions = $route->getOption('parameters') ?: array();

Does this mean it will call the method twice? or does PHP not do that? Would be nice if getOption() could specify a default, like ParameterBags can.

+++ b/core/lib/Drupal/Core/EventSubscriber/EntityRouteAlterSubscriber.phpundefined
@@ -0,0 +1,78 @@
+      if ($parameter_definitions) {

Use !empty() or something here? I'm not a fan of just using it bare, but this is not important :)

+++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.phpundefined
@@ -30,74 +29,29 @@ class EntityConverter implements ParamConverterInterface {
+    $entity_type = substr($definition['type'], strlen('entity:'));
...
+      $entity_type = substr($definition['type'], strlen('entity:'));

Is there a reason to do it this way? Seems more confusing using strlen() like this.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.phpundefined
@@ -11,79 +11,175 @@
+   * For each route, saves a list of applicable converters to the route.

Maybe just 'Saves a list of applicable converters to each route.' would be better.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.phpundefined
@@ -11,79 +11,175 @@
+        foreach ($this->getConverterIds() as $converter) {
+          if ($this->getConverter($converter)->applies($definition, $name, $route)) {
+            $definition['converter'] = $converter;
+            break;
+          }
+        }

Here we should try to do something similar to #2012382: Improve efficiency of access checker matching on routes if possible, so we don't iterate over all the things. Or is this the other issue you mentioned? Sorry if it is.

Out of scope of this issue, but probably worth talking about.

Yeah, I pretty much meant that should be a follow up if it isn't already. That could unfortunately be an api change though :/

Yeah, but that's a general issue. We don't have a proper standard for service names. Definitely follow-up.

Follow ups should be filled. Please just do it and link it in the summary.

Here we should try to do something similar to #2012382: Improve efficiency of access checker matching on routes if possible, so we don't iterate over all the things. Or is this the other issue you mentioned? Sorry if it is.

Yeah, agreed. We might have 'static' converters. However, this is not as important as for access checks as we aim to cover 90% of the use-cases with the typed data converter.

As you said, 90% use-case is the typed data converter, so there will be simply not many data converters and based upon that, there is no real performance issue. No critique but it made the access manager not really more readable. Let's stay simple.

RTBC +1 from me.

Adding tag

Possibly out of scope for this issue, but I'd like to see a follow-up where we do a better job of separating the "real" Request attributes that are being upcast here, and the extra ones like 'account' that we are sticking in there. We should really subclass Request to have a different property to hold those, or at least decide on a clear naming convention that will avoid collisions between the 2. I don't really think the symfony leading "_" pattern is very robust, since _ is valid in identifiers and path pattern slugs I think?

Reroll looks good.

Awesome! Glad to see everyone agree. Committed to 8.x.

We still need to create the follow-up issues and create a change notifications so setting to 'active'.

Updated issue summary.

Updated follow up for service name standard #1943846-112: Improve ParamConverterManager and developer experience for ParamConverters in issue summary. We got few more follow up request, but not sure everyone agreed on them. Here is the list:

1. In #85

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterTypedDataResolversPass.phpundefined
@@ -0,0 +1,36 @@
+ * Contains Drupal\Core\DependencyInjection\Compiler\RegisterTypedDataResolversPass.
...
+  public function process(ContainerBuilder $container) {
+    if (!$container->hasDefinition('typed_data_resolver_manager')) {
+      return;
+    }
+
+    $manager = $container->getDefinition('typed_data_resolver_manager');
+    foreach ($container->findTaggedServiceIds('typed_data_resolver') as $id => $attributes) {
+      $priority = isset($attributes[0]['priority']) ? $attributes[0]['priority'] : 0;
+      $manager->addMethodCall('addTypedDataResolver', array(new Reference($id), $priority));

Follow up: This seems to be a really common pattern so somehow this could be moved to a common baseclass on the longrun.

2. In #85

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -77,6 +78,26 @@ public function __construct(array $values, $entity_type) {
+
+    if (!$storage = $container->get('plugin.manager.entity')->getStorageController($entity_type)) {
+      throw new \InvalidArgumentException(sprintf('Could not retrieve storage controller for the %s entity type', $entity_type));
+    }

Follow up: Throw exceptions in the entity manager instead.

Are they good/worth creating follow up? if not, we can remove 'Needs followup' tag.

Yep, worth filing issues. Also see #119, as well as #112 and following for the first snippet in that review.

In general, it would be good if patch authors got in the habit of filing followups right away based on feedback from their reviewers when they want to de-scope something from the issue. The issues don't have to be beautiful; If you're in a hurry, just reference the comment from the parent, quote enough of the issue that it makes sense, and tag it "Needs issue summary update". (If you're not in a hurry, take the time to explain the issue, so that we don't end up with issues that no one understands sitting around.)

Basically, what @dawehner said. :)

Follow ups should be filled. Please just do it and link it in the summary.

#2083415: [META] Write up all outstanding change notices before release

Change notice started here: https://drupal.org/node/2084329

Disclaimer: I know absolutely nothing about this patch except for what I learned reverse engineering it fixing a WSCCI conversion. Change notice is as bare bones as it gets, but hopefully it helps someone else not spend a couple of hours trying to figure out how this works. That being said, it's been over a month since this patch got in. Can fubhy or someone else involved in this patch please get this change notice written up?

Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release.

The patch for this issue was committed on July 24, 2013, meaning that the change record for this issue has been missing for more than six months.

I don't really think that this issue is worth to change-notice, given that there was no real change for the outside world.

We at least need documentation how it works and how you can add your own upcaster, and having a change notice for that is a first step to get someone to write documentation based on it :)

Tiny tag field--

https://drupal.org/node/2217281

The change record https://drupal.org/node/2084329 filed at September 8, 2013

So the question is about "Needs followup" tag

#127.0 already provides link to follow-up #2055857: [policy, then patch] Standardize service names

https://drupal.org/node/2217281 is a 404 for me.

https://drupal.org/node/2084329 doesn't really qualify as a decent change notice to me. As #126 says, it's just an initial (rough) version.

https://drupal.org/node/2084329 should be improved with whatever is needed. I deleted the draft at https://drupal.org/node/2217281 at @andypost's request; it was essentially just a duplicate of the other.