Upcast request arguments/attributes to full objects

Tagging for PNW sprint

Rerolled against HEAD with a added try/catch to ignore cases where we don't have a registered param converter such as 404. 404 results in a ExceptionController being passed into the ParamConverterManager which causes an uncaught exception to be thrown causing all sorts of weird stuff in _drupal_log_error(). The caught exception just returns out of the param converter manager without doing anything allowing Drupal to continue to do its thing.

Still sprinting at the PNW WSCCI Sprint :)

The green got me excited :( but empty patches always pass... trying again.

Yeah I had the feeling that wasn't going to fly :)

I am not familiar with the lifecycle of a ExceptionController, it seems to be handled elsewhere. Is it safe to assume that it has no business in the ParamConverterManager? If that's the case I guess there should be logic in the ParamConverterSubscriber to prevent going further for controllers that don't require param converting. Are there other types of controllers where this is the case?

Heh yeah, you're right, it is bleedingly obvious. I figured out ParamConverterManager just needs to check for one more condition; that whether the parameter actually needs converting to whatever the type hint says it should be. Tests are green on my local, just cleaning up the patch.

Here's patch & interdiff from Crell's original patch

whoops, not sure how that typo got in there :-\ trying again while on the bus back to Vancouver

Still getting familiar with Drupal's testing framework but things are starting to make sense :).
I have the ParamConverter & ParamConverterManager unit tests green now though there is a 'Verbose message' debug notice - not sure what to do with that.

Just working on rerolling patch against latest 8.x code...

oh dear, sorry for my flailing around - still working on my patch workflow. That last patch was missing one change (the change existed on my local but wasn't staged when i made the patch). Trying again.

While reviewing this I wrote a test to see if it upcasts user id to a user object, found out that only Node class was registered with Converter service. I've tried to make it generic by looping over entity info and registering the class name for all entities. I'm not sure if that was the right thing to do, there must be a better way. Also I saw refactor @todo for the time entity is injectable, for now maybe this could work? Attached is a interdiff and a patch, would love feedback.

Thanks

This is indeed breaking Drupal installation because of the changes in CoreBundle.php, not sure why?

Update: Fatal error: Call to undefined function field_read_fields() in core/modules/field/modules/field_sql_storage/field_sql_storage.instal

Yes, that code does not belong in the core bundle. What we need is a compiler pass for processing these paramconverters. If you look in /core/lib/Drupal/Core/DependencyInjection/Compiler you'll see a few examples of compiler passes. Looking through the patch, it's not actually immediately obvious to me how to do it, because it's not as if each entity-type-providing module (like node module) is supposed to register a new tagged service (which is what you usually do with compiler passes if you look at the other examples). But hey, figuring that bit out will be the fun part, right? :-P

Moved call to entity_get_info() to a compiler pass, seems like the obvious approach.
Will need someone more familiar to pass judgement on whether it's the 'right' way to do this.

So I guess calling entity_get_info() in a compiler pass is functionally the same as doing it directly in CoreBundles.php :P

Hmm, how gauche an idea is this: each module that provides an entity type *does* register a separate service to the DIC in its bundle, but the service is created using a factory method that always returns the same instance of the entity paramconverter. And then it's tagged with the 'paramconverter' tag and a 'classname' attribute whose value is the fully-qualified class name of the entity. Something like:

$container->register('paramconverter.node')
  ->setFactoryClass('Drupal\Core\ParamConverter\EntityParamConverterFactory')
  ->setFactoryMethod('getParamConverter')
  ->addTag('paramconverter', array('classname' => 'Drupal\node\Node'))

Then in the compiler pass you go through all services tagged as 'paramconverter' and add the services to the paramconverter_manager.

Worth a shot?

I don't really get that. Why isn't there a mechanism for converting dynamic request path parameters into data resources built-in in the Routing component? Doesn't it support dynamic arguments à la {node} already?

Or are you aiming for #1821662: Register entity controllers with the DIC ?

An abstract "ParamConverter" service would not make sense to me. The point we need that, we've abstracted too much, way too much.

I'm not sure I understand why a ParamConverter service is too abstract. Different types of parameters require different methods of loading - it's no different in concept to the *_load() method in D6/7.

In saying that, Kat's proposed approach seems a little obscure. I'm still getting familiar with the services and the DIC but it I'm worried that it won't be inherently obvious to entity-providing module devs exactly why they should jump through those hoops just to declare a param converter.

While I think @tnightingale does make a valid point about making things complicated for entity-providing module devs, in the absence of any alternative suggestions, I've gone ahead and implemented my suggestion in #33. I've added a doc block to the NodeBundle which hopefully does a decent job of explaining what's going on. Really, all they'll have to do is copy and paste that code and change the service id and class name.

+++ b/core/modules/node/lib/Drupal/node/NodeBundle.php
@@ -0,0 +1,37 @@
+      ->addTag('paramconverter.entity', array('classname' => 'Drupal\node\Node'));

+++ b/core/modules/user/lib/Drupal/user/UserBundle.php
@@ -0,0 +1,24 @@
+      ->addTag('paramconverter.entity', array('classname' => 'Drupal\user\User'));

This duplicates and hard-codes entity info into service container parameters. From an architectural entity system perspective, I do not think we want that.

Closely related:
#1763974: Convert entity type info into plugins
#1821662: Register entity controllers with the DIC

This particular change proposal still leaves me in a deeply skeptical mood. Providing the equivalent of router argument loaders is one thing. Turning that into an overly abstract, generic service that isn't even remotely related to the router, is an entirely different thing. I do not see, nor do I understand why we'd want to have an abstract service that is in itself only a meta-factory that (ab)uses the service container by leveraging service tags to retrieve an actual factory that is able to instantiate the class, which in turn, is ultimately responsible for turning a simple string into typed data. — Seriously?

Additionally, we're turning the service container more and more into a true God Object. This is guaranteed to get us into a bloody race condition hell. And not only that — it will render sites unusable, in unrecoverable WSOD states. You will need a Drupal core developer to recover from the broken cross- and intertwined-interdependencies that resulted in the un-buildable and un-parseable and/or un-compileable God Object. The only other remotely possible way to generally recover from that will be a /rebuild.php that performs loads of conditional checks on all subsystems, extensions, and services to figure out what exactly went wrong and what needs to be done to get the God Object functional again. Given where we're heading currently, that rebuild.php is almost guaranteed to be in core for D8, and, it will be frequently needed.

So back to the point and this issue: Can we pretty please limit this?

- First and foremost, the class belongs into Drupal\Core\Routing.
- Second, why isn't this a property on the route that defines a callable, like we do have today?

In the end, why can't we have this?

# node.routing.yml:
node_view:
  path: node/{node}
  _controller: yadayada
  arguments:
    node: node_load

node_view_fancy:
  path: node/{node}
  _controller: yadayada
  arguments:
    node:
      - Drupal\node\NodeBundle
      - getNodeByID

node_view_abstract:
  path: node/{node}
  _controller: yadayada
  arguments:
    node:
      - Drupal\Core\Entity\EntityBundle
      - getEntityByID
        - node

If not, then I really need a clear and concise explanation for why this insane amount of abstraction is needed.

While I think your concerns of service container abuse are valid, they should probably be discussed in a place more visible than this issue.

As to requiring a callable property for each argument on each route, that isn't like what we have today. Today we have a single callback per argument type that is based on a global argument/function naming convention. I would much rather see required argument objects created (consistently) based on reflection than force (allow?) developers to provide a unique callable for every route parameter they define. Not only is it overly verbose, but it opens itself for abuse not too dissimilar from that which we are seeing with the service container.

+++ b/core/lib/Drupal/Core/EventSubscriber/ParamConverterSubscriber.php
@@ -0,0 +1,70 @@
+   * @param ParamConverterManager $param_converter_manager

parameter type should include the full namespace. Variable name could be just $manager?

+++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
@@ -0,0 +1,58 @@
+ * Each entity type needs to be reistered on ParamConverterManager

Should be "registered".

+++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
@@ -0,0 +1,58 @@
+      throw new \InvalidArgumentException(sprintf('No entity found that uses class %s', $class));

Why do you need sprintf() here? No long arguments are used, so it hurts readability here.

+++ b/core/lib/Drupal/Core/ParamConverter/EntityConverterFactory.php
@@ -0,0 +1,38 @@
+    if (!isset(self::$converter)) {
+      self::$converter = new EntityConverter();
+    }
+    return self::$converter;

Do not use "self" where possible, use "static" to fully leverage PHP's late static binding.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.php
@@ -0,0 +1,127 @@
+      $new_value = $this->convertTo($param_value, $param->getClass()->getName());

So that means upcasting only works for concretely type-hinted parameters like "Node $node", but not for "EntityInterface $entity". So even if I have "{node}" in the route I will not get a loaded node into $entity :-(. I would get an error anyway, because {node} could not be mapped to $entity as the variable name differs. I conclude that it will not be possible to use upcasting with generic entity controllers?

+++ b/core/modules/system/lib/Drupal/system/Tests/ParamConverter/ParamConverterManagerTest.php
@@ -0,0 +1,108 @@
+/**
+ * Basic tests for the ChainMatcher.
+ */
+class ParamConverterManagerTest extends UnitTestBase {

wrong comment?

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/ParamConverterTest.php
@@ -0,0 +1,58 @@
+/**
+ * Basic tests for the ChainMatcher.
+ */
+class ParamConverterTest extends WebTestBase {

wrong comment

+++ b/core/modules/system/tests/modules/router_test/lib/Drupal/router_test/RouterTestBundle.php
@@ -0,0 +1,41 @@
+  public function build(ContainerBuilder $container) {
+
+  }

empty function?

+++ b/core/modules/system/tests/modules/router_test/lib/Drupal/router_test/RouterTestBundle.php
@@ -0,0 +1,41 @@
+class TestParamConverter implements ParamConverterInterface {

each class should be in its own file, PSR-0 ...

+++ b/core/modules/user/lib/Drupal/user/UserBundle.php
@@ -0,0 +1,24 @@
+class UserBundle extends Bundle {

And that has to be repeated for every entity type? I think there should be just one EntityBundle that registers all entity classes available in the system.

Minor clean-up pretty much along the lines of #42.

Additional changes:

• Forwarding request object into the param converter for more custom use-cases.
• Adding proper NotFoundHttpException handling -> Upcasting should also contain the previous functionality of load callbacks from hook_menu() where, in case a parameter could not be loaded, we directly got a 404 instead.

And that has to be repeated for every entity type? I think there should be just one EntityBundle that registers all entity classes available in the system.

That is just temporary. Currently we can't loop on entity_get_info() / $container->get('plugin.entity.manger')->getDefinitions() because of the dependencies that comes with. #1821662: Register entity controllers with the DIC is basically blocked due to the same problems.

Two (important) things that this patch currently doesn't cover:

1. Upcasting of values that depend on a second {slug} on the route (e.g. '/{entity_type}/{entity}'). I already had code for that in place which used the now also forwarded Request object to read out the 'entity_type' from the attributes: $request->attributes->get('entity_type'). {entity} would had a registered param converter for EntityInterface in that case. However, that doesn't work because that requires the parameter to always be named {entity_type} and it also doesn't work if the path has 2x {entity_type} in it. Oy.
2. Validation of parameters that are not objects. E.g. if the route depends on {entity_type} I don't want to have to check for the existence of that entity_type in my request controller. But yeah... This isn't really bound to 'converting' parameters and therefore not really an upcasting issue... but it is closely related. In simple cases it obviously works to just do something like: implode('|', array_keys(entity_get_info())); and put that into the regex validation. However, that doesn't work anymore once you have multiple related arguments in the path that are built on each other (e.g. "{entity_type}/{bundle}/{field}").

Opened #1837388: Provide a ParamConverter than can upcast any entity. as a follow-up. I think that upcasting and validating whether or not a path actually exists are closely related to each other.

Rerolled against head - test routes are now in yaml.

Noticed some of the style and commenting issues pointed out in #43 still hanging around. Updated patch.

It should also be public static function getParamConverter(), with public first.

Can I just say that this block makes me feel warm and fuzzy inside? :-)

Yahr, now that you point that out though I think we have to throw that out :(. Doesn't play well with entity info caching. We should not copy that data. Instead we should just do:

  /**
   * Implements ParamConverterInterface::convert().
   */
  public function convert(Request $request, $value, $class, array $arguments = array()) {
    // Try to retrieve the entity type of the given entity class.
    foreach (entity_get_info() as $key => $entity_info) {
      if ($entity_info['entity_class'] == $class) {
        $entity_type = $key;
        break;
      }
    }

    // Throw an exception if we don't know about the class we're being asked to
    // convert to.
    if (!isset($entity_type)) {
      throw new \InvalidArgumentException("No entity found that uses class $class.");
    }

    // Load the entity loader, then use it to load the object.
    // Once again, this should be coming from the Service Container eventually.
    if ($entities = entity_get_controller($entity_type)->load(array($value))) {
      return reset($entities);
    }
  }

After all, that is still just a temporary solution. entity_get_controller(), entity_get_info(), etc. should be removed from that at some point anyways.

Addressed the other concerns as well and updated to "Contains" instead of "Defintion of" in @file.

Whoops.

Cool. Now that this is RTBC I would like to draw some attention to #1837388: Provide a ParamConverter than can upcast any entity.! :)

+++ b/core/modules/user/lib/Drupal/user/UserBundle.php
@@ -0,0 +1,28 @@
+  public function build(ContainerBuilder $container) {
+    $container->register('paramconverter.entity.user', 'Drupal\Core\ParamConverter\EntityConverter')
+      ->setFactoryClass('Drupal\Core\ParamConverter\EntityConverterFactory')
+      ->setFactoryMethod('getParamConverter')
+      ->addTag('paramconverter.entity', array('classname' => 'Drupal\user\Plugin\Core\Entity\User'));
+  }

Why don't we use container parameters instead of separate service definitions per entity type for this?

I.e., why not simply this?

$container->setParameter('paramconverter.entity.user', 'Drupal\user\Plugin\Core\Entity\User');

@sun: We won't have the ability to override upcasting for specific entity types anymore (not sure if that is even a use-case but still... we would remove that option). Also, parameters should rather be used for injecting single values into the DIC. We would (ab)use them as tags with this. The only way to filter out the right parameters would be to iterate over all of them and then doing something like strpos() to filter out the correct ones.

However, I am uploading a patch which uses parameters instead. Not suggesting we should use it, I am just uploading it for people to see what the difference would be.

Oh well, the Factory would be redundant too of course.

Yes, I understand that the entity parameter converter has dependencies.

A single entity parameter converter service is vastly different from a dozen or more copies of the exact same entity parameter service, which, each, need to be defined in a very special way, involving a non-trivial amount of expected arguments. Failing to meet that exact, custom expected service registration for any particular entity type results in broken implementations. This is undocumented and not "documentable" in the first place — it's an extrapolation of arrays of doom to registered services on the DI container.

What I do not understand is why we have to re-register and re-define the entity parameter converter for every single entity type that exists in the system.

This adds a large amount of duplicate code all over the place, involving a poor DX, and registers a dozen or even more services that do not differ in any way.

On top, the manually specified class name for the entity parameter conversion is architecturally not exactly right, since it duplicates the entity type metadata/information in a spot that is completely detached from the rest of the entity system. I do not understand why we're hard-coding the entity class for each entity type into the router to begin with, and why this construct does not rely on the existing entity type metadata/information instead.

a clean way for bundles to dynamically register services based on some other information

Those keywords essentially trigger a whole other question: Is it even legit to register entity-type-specific services in a bundle?

Clearly, without hard-coding and duplicating entity info into those bundles, the bundles would not be able to register anything at all, since the information they require to register anything in the first place can only be supplied by other services that may be registered already or not — a clear race condition and circular dependency.

Which in turn leads to the question whether entity-type-specific param converters should be registered through bundles on the DIC at all.

The proposed change looks and feels like a big mistake to me.

(Btw, I also don't understand the rush here — these kind of changes are not bound to feature freeze, we have to do them anyway.)

Note I've not done a full review here but there's several things on a quick look through the patch that make me uncomfortable, so I'm setting back to CNR for more discussion at least

It's also missing any input from the existing menu system maintainers. Only sun has done significant work with the existing menu system in this issue and his concerns appear to have been brushed over (although they were a bit dramatically stated, but still there's plenty that's valid in there).

First the off-topic bit:

@Crell

2) I see nowhere in here that there is a race condition. Unless there's a specific example you can point to, I call FUD. Nor do I see how there's going to be a registry-esque circular dependency of doom.

That's exactly what #1831350: Break the circular dependency between bootstrap container and kernel is. This issue isn't the same problem as that, but it's not as if these issues don't exist. While the DIC/module list dependency issue is 'fixed' (via a hard-coded config file storage call, not exactly pretty and that was my idea at BADCamp :(...), I don't think we're out of the woods there by any means. Most of the stuff we're using was not designed for code being dynamically loaded based on configuration in the way Drupal does and we're running into issues everywhere with that. For another example #1846376: Namespaces for disabled modules are registered during the first request after a module is disabled. It's a shame to see those issues being dismissed so easily as FUD.

Now the patch:

+/**
+ * Defines the node module bundle.
+ */
+class NodeBundle extends Bundle {
+
+  /**
+   * Overrides \Symfony\Component\HttpKernel\Bundle\Bundle::build().
+   *
+   * The job of converting an entity id to an entity object for controllers
+   * expecting an object is done by an EntityConverter. We register a service
+   * using the EntityConverterFactory (which ensures only one EntityConverter
+   * is instantiated for all entity types) and tag it so that the compiler
+   * pass will add it to the ParamConverterManager, using the classname
+   * attribute to tell it that the 'Drupal\node\Plugin\Core\Entity\Node' class
+   * can be converted using the EntityConverter.
+   *
+   * @see \Drupal\Core\ParamConverter\EntityConverter
+   * @see \Drupal\Core\ParamConverter\ParamConverterManager
+   * @see \Drupal\Core\DependencyInjection\Compiler\EntityParamConverterPass
+   */
+  public function build(ContainerBuilder $container) {
+    $container->register('paramconverter.entity.node', 'Drupal\Core\ParamConverter\EntityConverter')
+      ->setFactoryClass('Drupal\Core\ParamConverter\EntityConverterFactory')
+      ->setFactoryMethod('getParamConverter')
+      ->addTag('paramconverter.entity', array('classname' => 'Drupal\node\Plugin\Core\Entity\Node'));
+  }
+
+}

I don't think this hunk is acceptable at all, it's a massive, massive developer experience regression compared to having a function call mything_load() and pointing the menu callback at it. Why does this have to be completely alien to the route definition?

It also goes completely against the aims of #1838676: Support dynamic entity type management, which just about every Entity system patch is slowly getting us closer to being able to do.

So let's discuss this some more and see if we can't have the entity system handle it centrally based on entity info.

Apart from that, I don't see any discussion in this issue on how this will integrate with access checks for menu links. The only way (at least at the moment, and no-one's proposed anything different that I know of) to check access on a menu link is to load the full object then run access checks on it - since the access check usually depends on the object, especially for entities.

For some discussion of how things currently work and some of the performance issues with it see #638070: Router loaders causing a lot of database hits for access checks or #881468: Performance: Static cache output of views_arg_load() (or many other issues).

So to get the object from a menu link, it looks like we'd have to mock the request object now?

+  public function convertTo(Request $request, $value, $classname) {
+    return $this->getConverter($classname)->convert($request, $value, $classname, $this->converterIds[$classname]['arguments']);
+  }

I also don't understand the hurry here at all, it's a regression that the new router system can't do this, it might even be critical unless we decide that's a feature we want to drop.

As for dynamically defining new Entity types, I'm happy to go on the record as thinking that's actually a bad idea and is taking flexibility to an absurd degree, which would run into problems such as, yes, circular dependencies in registration.

There's already a Drupal 7 module that people use which does this, which has just under 1,000 users, so are we saying that's not possible even in contrib then? And not only that, are we saying it's impossible to define anything dynamically that will require upcasting request arguments?

I assume we could get around this in some way by having a uri like entity/node/1.

If that's doable it might be an acceptable limitation, but if not this needs a lot more consideration before committing a regression.

For menu links... I don't have a solution there.

OK well we need to come up with one...

or we would have to make the upcasting system convert the ParameterBag into an array, run conversions, and then convert it back, something I didn't think would be viable for performance.

That sounds roughly like what I had in mind, we should try it, profile it and see whether it's viable for performance or not.

Having menu links that point to a 403 page is not going to be releasable. We're likely to take a performance hit with menu links as entities anyway, so we'll need to find some ways to ameliorate that (probably more aggressive caching at this point unless someone comes up with something amazing). The object / array conversion doesn't sound like it'd be particularly bad - it's certainly not in the magnitude of having a menu link pointing to a view with 100 displays.

This is a reroll of #53 to chase HEAD. Also renamed EntityParamConverterPass to RegisterEntityParamConvertersPass for consistency with other compiler pass names. CNR for bot.

#68: 1798214-68.patch queued for re-testing.

g.oeschler is working on a new direction here based on the WSCCI meeting last week.

Yay. I haven't worked on this at all other than the #68 reroll. I just asked for a retest, cause I can't reproduce the test failure that bot crashed on last time. I'm looking forward to seeing what g.oeschler comes up with.

Back to needs work, since that's the real status. #68's status change was just for bot.

I'm assuming none of the Drupal committers will agree to release Drupal with 2 routing systems, one that upcasts and one that doesn't, so I think that makes this critical.

we have #1848648: [meta] Convert menu system to new routing system as critical placeholder.

This showcases the idea of upcasting based on guessing the class we want to convert to from the variable name.

Let's suppose that most of the time controllers will take common things like users and nodes as parameters. So given a routing pattern /operate/on/some/{node} we could just guess that Drupal\node\Plugin\Core\Entity\Node was meant.

For sure there will be cases where this will not work, so we need to give the ParamConverter a hint what it should do. This is achieved by adding options to the route definitions. That's why this patch contains #1855204-14: RouteBuilder needs to respect route options set in yaml file.

router_test_node_node:
  pattern: '/router_test/test_node_node/{node}/{other_node}'
  defaults:
    _content: '\Drupal\router_test\TestControllers::test_node_node'
  requirements:
    _access: 'TRUE'
  options:
    converters:
      other_node: 'Drupal\node\Plugin\Core\Entity\Node'

This implementation actually manages to upcast {node} by guessing and {other_node} by hinting, but is catching an Exception elsewhere. See ParamConverterTest::testCanRoute().

This is just a prototype! I'm fully aware that this passes the target class name, rather than using the respective paramconverter.entity.* services.

rerolled

This is apparently blocked on #1874500: CMF-based Routing system.

It's also apparently required for SCOTCH.

yep, confirming this is necessary for SCOTCH...at some point. not immediately.

Conditions is moving along and has the beginnings of a legitimate context system in it. The sooner this is in, the sooner we get to figure out how routing can feed that sort of context system and by proxy, blocks will likely inherit the exact same mechanism. Sooner is much much better than later.

Eclipse

Here's upcasting based on the new Symfony-CMF Routing.

Right now there are three phases for upcasting:

1. Upcast everything which is defined explicitly:

   some_route:
     pattern: '/some/{pattern}'
     defaults:
       _content: '\SomeController::someMethod'
     options:
       converters:
         pattern: 'node'
     ...
   

   This will upcast the node id from pattern to a node.

2. Upcast evertything which has the name of an entity type:
   

   some_route:
     pattern: '/some/{node}'
     ...
   

   This will upcast node id from node to a node.

3. Mapping of url arguments to controller parameters:
   For the signature SomeController::someMethod($article) the pattern '/some/{pattern} would return an error because the Symfony router expects a parameter called 'article'.
   In this case we can map the arguments to the right name.

     options:
       converters:
         pattern: 'node'
       map:
         pattern: 'article'
   

   Here we get a node object in $article.

+++ b/core/lib/Drupal/Core/ParamConverter/EntityAliasConverter.phpundefined
@@ -0,0 +1,80 @@
+    $types = array_keys(entity_get_info());
...
+        $entity = entity_get_controller($dealiased_var)->load(array($value));

I think it should now be possible to inject the entity manager and get the entity controller through that...

the entity_get_info() can afaik not yet be injected, requires changes to the manager, which are planned.

+++ b/core/modules/system/tests/modules/paramconverter_test/lib/Drupal/paramconverter_test/TestControllers.phpundefined
@@ -0,0 +1,24 @@
+  public function test_user_node_foo($user, $node, $foo) {
+    return "user: " . (is_object($user)?$user->label():$user)
+         . ", node: " . (is_object($node)?$node->label():$node)
+         . ", foo: " . (is_object($foo)?$foo->label():$foo);

Coding style :)

+++ b/core/modules/system/tests/modules/paramconverter_test/paramconverter_test.moduleundefined
@@ -0,0 +1,3 @@
+/* Intentionally blank file. */

Should be done as a @file docblock. It's kinda weird but we require that every file has one.

What does ~ mean?

~ means NULL in YAML. Part of the YAML spec. Not sure why we'd declare anything anywhere with a NULL value (instead of omitting the key/property altogether), but if that's API-wise legit/needed in the first place, then ~ is perfectly fine.

The idea behind node: ~ is to explicitly prevent an argument from being upcast. EntityConverter would see "node", find a matching entity and perform upcasting on it. But if the EntityAliasConverter running before EntityConverter finds this option, it will mark node as upcast without actually touching it. This definitely need documentation.

Btw I'm not really happy with the name EntityAliasConverter. Maybe we should call it ExplicitEntityConverter or something. Any suggestions?

Whew, lots of homework. :)

There are two failures in CollapsedDrupal\language\Tests\LanguageUILanguageNegotiationTest.

I have no idea how this could be related to upcasting. Just re-running the test to make sure this was no random failure.

#98: 1798214-upcasting.patch queued for re-testing.

The fail of LanguageUILanguageNegotiationTest it strange.

If I take both converters out of the DIC it passes. If only one of them is active the test fails, although it's not even running code from the converters. *shrug*

You added a patch which is testable by our state-of-the-art continous integration test environment. Are you sure you don't want to change the status to "needs review"?

[X] Yes [ ] No

The problem with the UI language negotiation test is that that particular language negotiation method calls current_path() to check whether it's an admin path. The current_path() method checks to see if we're in request scope, in which case it returns the system_path attribute of the request, however with this upcasting stuff happening, a situation arises where we are within request scope but the system_path attribute has not been set on the request yet, and the EntityManager makes a call to language() for caching purposes - this then invokes the language negotiation method which eventually ends up making the current_path() call which retrieves the value NULL. If this explanation is too long and rambling, just see the interdiff :-)

I am afraid that you might hate me for this but I strongly believe that this is not the right way of doing this. I know we really need to find a solution for this quickly but at the same time I am really worried about this piece of code because it's such an essential part of the API and one of the things that developers will have to touch a lot. DX and stability is crucial here...

+++ b/core/lib/Drupal/Core/CoreBundle.phpundefined
@@ -198,6 +200,15 @@ public function build(ContainerBuilder $container) {
+    $container->register('paramconverter_manager', 'Drupal\Core\ParamConverter\ParamConverterManager')
+      ->addTag('route_enhancer', array('priority' => 0));
+    $container->register('paramconverter.entity_explicit', 'Drupal\Core\ParamConverter\ExplicitEntityConverter')
+      ->addArgument(new Reference('plugin.manager.entity'))
+      ->addTag('paramconverter', array('priority' => 10));
+    $container->register('paramconverter.entity', 'Drupal\Core\ParamConverter\EntityConverter')
+      ->addArgument(new Reference('plugin.manager.entity'))
+      ->addTag('paramconverter', array('priority' => 0));

What bugs me most is the whole concept of priorities. We haven have two layers of these it seems... once for the enhancers, and then once more in the param converter manager. That doesn't look stable at all. We don't want to get to a place where our developers have to worry about the priority of their converters and the naming of their {slugs}.

+++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.phpundefined
@@ -0,0 +1,68 @@
+  /**
+   * Tries to upcast every variable to a entity type of the same name.
+   *
+   * If there is no entity type with the name of the variable it simply skips it.
+   * It will not process variables which are marked as converted. It will mark
+   * any variable it processes as converted.
+   *
+   * @param array $defaults
+   * @param \Symfony\Component\HttpFoundation\Request $request
+   *   The current request.
+   *
+   * @return array The modified defaults.
+   */
+  public function process($defaults, $request) {
+    $variables = $defaults[RouteObjectInterface::ROUTE_OBJECT]->compile()->getVariables();
+
+    $types = array_keys($this->entityManager->getDefinitions());
+
+    foreach ($variables as $var) {
+      // Only upcast if there is a type with the name of the variable
+      // and the variable hasn't been upcast yet.
+      if (in_array($var, $types) && ! in_array($var, $defaults['_converted'])) {
+        $value = $defaults[$var];
+
+        $entities = $this->entityManager->getStorageController($var)->load(array($value));
+
+        // Make sure $entities is null, if upcasting fails.
+        $entity = $entities ? reset($entities) : null;
+        $defaults[$var] = $entity;
+        $defaults['_converted'][] = $var;
+      }
+    }
+    return $defaults;
+  }

I am also worried by the guessing going on here... That, together with priorities simply screams 'race condition'.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.phpundefined
@@ -0,0 +1,78 @@
+  public function enhance(array $defaults, Request $request) {
+    // This array will collect the names of all variables which have been
+    // altered by a converter.
+    // This serves two purposes:
+    //  1. It might prevent converters later in the pipeline to process
+    //     a variable again.
+    //  2. To check if upcasting was successfull after each converter had
+    //     a go. See below.
+    $defaults['_converted'] = array();
+
+    foreach ($this->converters as $converter) {
+      $defaults = $converter->process($defaults, $request);
+    }
+
+    // Check if all upcasting yielded a result.
+    // If an upcast value is NULL do a 404.
+    foreach ($defaults['_converted'] as $variable) {
+      if ($defaults[$variable] === null) {
+        throw new NotFoundHttpException();
+      }
+    }
+
+    return $defaults;
+  }

I find this also a bit strange.. Having to coupe with this $defaults array and managing and populating that in every converter properly doesn't look right to me either. There shouldn't be a need for something like that. Writing a new, custom converter should really just be as easy as returning as receiving a value and returning an upcast object/whatever from it. We certainly don't want make our converters responsible about that metadata.

Okay.. So I don't want to spoil the party without offering an alternative:
I just had a short conversation with fago about this issue and we concluded that we should probably consider going with TypedData all the way. That would mean we would run upcasting through the TypedDataManager... That way we don't only cover entity types out of the box but basically cover everything that can be expressed as typed data plugin (which should be more or less everything that we have in Drupal at some point plus simple types like lists, strings, booleans, etc.). We would then be able to run our upcasting through the createInstance() logic of the typed data plugin manager. Unless I am missing something here we would not need any custom logic for loading stuff...

So, when defining a route we would basically only declare the typed data type of a {slug} in the route options and be done with it...

I am not a TypedData expert but according to fago there are also concepts for loading things based on conditions or to load lists of things easily.

I am happy to elaborate more on this or even provide a patch if this sounds appealing. However, there are still a few TypedData issues that have to be solved before we would be able to fully leverage that awesomeness (e.g. #1868004: Improve the TypedData API usage of EntityNG).

Regarding loading - nope you don't need to handle this, you just need to call getValue() on a typed data object. I'm not into this issue, but from my distant-point-of-view relying upon typed data would make sense here.

This is how entity loading via typed data works right now:

   $data_definition = array(
      'type' => 'entity',
      'constraints' => array(
        'entity type' => 'node',
      ),
    );
    $node = $typed_data_manager->create($data_definition, 1)->getValue();

Once #1845546: Implement validation for the TypedData API is in, we could even think about leveraging validation here. Not sure this makes sense here performance wise though.

With #1868004: Improve the TypedData API usage of EntityNG it would work just with type "node", but I'd suggest supporting whole data definitions anyhow as this gives us more flexibility and would also support lists or custom "computed property" classes, such that you can easily plug-in in your own "load XY" logic.

Okay... So what we would have then is a single ParamConverter service into which we would inject the TypedDataManager. In that ParamConverter we would basically read the typed data definition array(s) from the route options and iterate over them to upcast the respective {slug}'s accordingly. All that said ParamConverter have to do would be to pass those arrays to the TypedDataManager->create() method one by one.

"What if my {slug} does not have a TypedData plugin and is therefore not know by the typed data API?" Well, write one... :)

So, I am 100000000++ for outsourcing this data handling and loading to TypedData API. In fact, I am so intrigued by this idea that I am going to write a patch in the afternoon.

1) There are some code style issues in the patch, and variable names could be cleaned up for better readability.

2) Code style issues aside, I think this patch could be simplified. We could easily combine the explicit and implicit entity converters into a unified entity converter that does both in a single pass. In other words, merge the two classes into one class and do both from a single process() function. It seems unnecessary, wasteful, and more complex to take two separate passes.

3) I'd also like to discuss the term 'converter'. I'm not sure it offers the best UX. At least in the case of entities, or anything else that is stored in the database, it is a lot more like 'loading' (vs 'converting'). If we used the term 'loader' it would be more consistent with Drupal 7 and optimized for the 80% case, which is loading things from the database. I also don't like the word 'converter' in the YAML files. In the YAML file, we're really mapping a token to a type, rather than converting a token to a type.

Thanks for the hard work on this so far! :)

Spent some time reviewing this patch in some more depth. I think we're on the right track, but as @Dries already mentioned, I also think there's a lot of room for simplification. Substantial room, even.

Aside from the architectural/technical issues that I'll note below, the code/patch could really use some love with regard to its coding style and documentation. Let's bear in mind that the routing system always was and remains to be a very complex part of Drupal's architecture.

Therefore, I do not feel comfortable with introducing a substantial base layer that claims to be able to handle - pretty much - THE most important aspect of the routing system, without substantial documentation, which makes it crystal clear how the individual parts are supposed to be used (and how they're NOT; i.e., what's a legit param converter, and what's not), and how the individual pieces are wired together in the end.

As I'll note below, I also think that the actual parameters that parameter converters receive (duh) are too obscure right now. You can replace $defaults with array $options, and doing so translates into how other developers are interpreting this API. $variables and $options are somehow extracted and derived out of $defaults, which turns $defaults into something that's way beyond Array Of Doom™. From an API and architectural perspective, that simply looks wrong and we need to provide a proper API for upcasting/loading arguments.

The other detail I'm concerned about is that the decision of whether an argument has been converted already, or not, is left to parameter converters. In terms of architecture, those converters should not be aware of that. They simply take an input and convert it into an output, if applicable. It is not their business to validate whether it is even valid to convert the input in any way. That logic must be performed by the wrapping/routing layer already.

Overall, I think we've made good progress here in the meantime. But I also think that there is still a lot of work to do for this patch. I do not consider it ready yet, mostly from an architectural perspective.

+++ b/core/includes/path.inc
@@ -88,7 +88,10 @@ function current_path() {
   // fallback code below, once the path alias logic has been figured out in
   // http://drupal.org/node/1269742.
   if (drupal_container()->isScopeActive('request')) {
-    return drupal_container()->get('request')->attributes->get('system_path');
+    $path = drupal_container()->get('request')->attributes->get('system_path');
+    if ($path !== NULL) {
+      return $path;
+    }
   }
   // If we are outside the request scope, fall back to using the path stored in
   // _current_path().

I do not understand why we would want to fall back to the non-request-aware path, if there IS a request scope.

This would imply that we're unable to resolve a path, even though there is a request. I do not think that this situation should be possible.

If there is a request, then we must be able to resolve a path.

+++ b/core/lib/Drupal/Core/CoreBundle.php
@@ -198,6 +200,15 @@ public function build(ContainerBuilder $container) {
+    $container->register('paramconverter_manager', 'Drupal\Core\ParamConverter\ParamConverterManager')
+      ->addTag('route_enhancer', array('priority' => 0));
+    $container->register('paramconverter.entity_explicit', 'Drupal\Core\ParamConverter\ExplicitEntityConverter')
+      ->addArgument(new Reference('plugin.manager.entity'))
+      ->addTag('paramconverter', array('priority' => 10));
+    $container->register('paramconverter.entity', 'Drupal\Core\ParamConverter\EntityConverter')
+      ->addArgument(new Reference('plugin.manager.entity'))
+      ->addTag('paramconverter', array('priority' => 0));

1) Using tags for this registration looks very odd. Are we re-inventing the EventDispatcher with a custom-flavored ContainerBuilder tagging mechanism?

2) Without looking into the other changes of this patch (which probably resembles the 99% experience of all developers who will have to deal with this), I am not able to make sense of "ExplicitEntityConverter" vs. "EntityConverter". Both are in the same namespace, and one of both just happens to be registered before the other, so what gives?

3) A priority of 10 does not leave sufficient room for all of contrib to sneak their way in. Should be bumped to 100.

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterParamConvertersPass.php
@@ -0,0 +1,47 @@
+    $manager = $container->getDefinition('paramconverter_manager');

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterRouteEnhancersPass.php
@@ -0,0 +1,38 @@
+    $router = $container->getDefinition('router.dynamic');

I am able to relate to the idea that architectural purity might be able to see a difference between "dynamic routes" and "converters for parameters of dynamic routes", but honestly, that's architectural purity only. The separation makes absolutely no sense for Drupal, and I cannot see why we need two different classes/services that are responsible for doing one and the same job.

A "dynamic router" has to be able to convert parameters. Otherwise, it is simply not dynamic. By design. And dictated by simple, human, non-technical logic.

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterParamConvertersPass.php
@@ -0,0 +1,47 @@
+      $services[$priority][] = new Reference($id);
...
+    foreach($services as $bucket) {
+      foreach($bucket as $service) {

s/$bucket/$priority/

+++ b/core/lib/Drupal/Core/ParamConverter/ExplicitEntityConverter.php
@@ -0,0 +1,95 @@
+   * It will not process variables which are marked as converted. It will mark
+   * any variable it processes as converted.
...
+        $defaults['_converted'][] = $var;

This track record should not be necessary.

The routing system should already know which parameters it upcasted already.

Also, the phpDoc states the direct *opposite* of what the code is doing.

+++ b/core/lib/Drupal/Core/ParamConverter/ExplicitEntityConverter.php
@@ -0,0 +1,95 @@
+  public function process($defaults, $request) {
+    $variables = $defaults[RouteObjectInterface::ROUTE_OBJECT]->compile()->getVariables();
+    $options = $defaults[RouteObjectInterface::ROUTE_OBJECT]->getOptions();

1) Wow.

2) I wouldn't have expected that it is possible to call methods on an array key of $defaults (!) - denoted by a constant of another class.

3) A compile() method that is effectively executed at runtime, on every single request, looks bogus. Anything that needs to be compiled should be precompiled already, or be cached appropriately.

4) Why aren't $variables and $options injected into param converter classes? The code looks too generic, so it doesn't look like a responsibility for each of them to figure that out.

+++ b/core/lib/Drupal/Core/ParamConverter/ExplicitEntityConverter.php
@@ -0,0 +1,95 @@
+    if (! isset($options['converters'])) {
...
+      } else {

Minor: Various (but heavy) code style issues here and elsewhere.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterInterface.php
@@ -0,0 +1,24 @@
+interface ParamConverterInterface {
+
+  /**
+   * Allows to alter the defaults of the current request.
+   *
+   * @param array $defaults
+   *
+   * @return array The modified defaults. Each enhancer MUST return the
+   *   $defaults but may add, remove or alter values.
+   */
+  public function process($defaults, $request);

Looking at this interface (and the implementations), I have the following problems:

1) Incomplete documentation.

2) I can make sense of $request (even though it is not type-hinted), and for now I assume that it is what I assume it is.

3) I do not know what $defaults is supposed to mean, what it contains, and where it comes from.

4) I do not understand why I should accept "defaults" and, alas, return $defaults. If parameter converters are supposed to alter existing data, then the data should be taken by reference instead of being returned.

5) In general, however, the code gives little to no idea what is supposed to be done by classes that implement this interface/method. The actual implementations/examples did not help to understand it, since they are too complex/specific to begin with.

5) To the architectural point of pass-based converters rather than mapping individual arguments to a converter: We've tried it both ways now. You don't like either one of them.

Can you clarify to which part(s) of my review this maps?

It's not clear to me — it's possible that you're interpreting more into #115 than I actually mentioned? (which is perfectly possible, as we're coming from two completely different perspectives; i.e., neck-deep into the [new] code + neck-deep into the [old] code :))

I also want to stress on the fact that we definitely need this, in order to move forward. I'm also aware that we're doctoring on this for a long time already. However: As I've tried to clarify, this issue/patch forms the architectural basis for pretty much the entire functionality that Drupal delivers (since by design, Drupal is based on dynamic routes and stuff). As such, it's not that kind of arbitrary class + service that we can simply slap into system — the moment we add this, an armada of contributors will start to make sense of it and try to convert existing routes to it (give or take the lack of an access controller; that's a detail which doesn't matter at all). And like any other code that is changed, some other core contributors will run into neck-deep details of the dynamic router and param converter systems (because their stuff suddenly breaks, even though it did not before), and they will need to be able to understand what's going on.

So, overall, I think we're (relatively) close, could do with some simplification wherever possible [less manager classes?], better input/output parameters and variables, a more controlled flow of what needs and what gets converted, definitely some good chunk of documentation, and we should finally be able to move forward. :)

As I read the last few comments, I get the impression that there's a major missconception of what this patch (#107) does and what not.

So here's in short what it does with some symfonic prologue: (@Crell: Please correct me if I'm wrong, I'm explaining it to myself right now...)

The heart of the new style routing is a service registered as "router.dynamic", which is a Symfony\Cmf\Component\Routing\DynamicRouter.

The method DynamicRouter::matchRequest(Request $request) is where it happens: You hand in a Request, you get back a route object. Bingo: Routing done.

Unfortunately that's not entirely true: You get back an "array of doom" called $defaults, holding the route object, the parameters, the machine name of the route and the controller responsible for processing it. Before this $defaults array is returned, we get the opportunity to alter these defaults. They are passed alongside the incoming request to the route enhancers. Route enhancers have to implement RouterEnhancerInterface which looks like this:

enhance(array $defaults, Request $request);

<scope of this patch>

• We provide infrastructure to give bundles the possibility to register route enhancers in a specific order. See Drupal\Core\DependencyInjection\Compiler\RegisterRouteEnhancersPass.
• We implement a service 'paramconverter_manager' providing parameter upcasting and register it to router.dynamic.

  The 'paramconverter_manager' allows bundles to register parameter converters to run in specific order. See Drupal\Core\DependencyInjection\Compiler\RegisterParamConvertersPass.
  ParamConverters have to implement ParamConverterInterface which looks like this

  process($defaults, $request);
  

  Yes, that's basically mimicking RouterEnhancerInterface. It was introduced to have a clear distinction between RouteEnhancers and ParamConverters to avoid confusion. But in the end it's operating on the same data. (And yes, it's missing type hints, I'll fix that!)

• We provide two upcasting services running in this order:
  1. Upcast every parameter two an entity type explicitly denoted in the route options/YAML (that's EplicitEntityConverter).
  2. Upcast every parameter to an entity type matching its name (that's EntityConverter).

</scope of this patch>

I have no idea if anyone will ever want to run a converter between these two. But if so, this architecture won't stop them from doing it.

So, Dries, this is why it was made like this. Surely it's possible to merge these two converters. If you are willing to trade this (admittedly excessive) flexibility for one object and a method call, let me know.

Quoting #115:

Therefore, I do not feel comfortable with introducing a substantial base layer that claims to be able to handle - pretty much - THE most important aspect of the routing system, without substantial documentation, which makes it

Sun, would you please point out where you found the "substancial base layer [...] to handle [...] THE most importing aspect of the routing system" in this patch? I think that's too much honor. It's just upcasting. Routing works perfectly fine without it.

+++ b/core/lib/Drupal/Core/ParamConverter/ExplicitEntityConverter.php
@@ -0,0 +1,95 @@
+   * It will not process variables which are marked as converted. It will mark
+   * any variable it processes as converted.
...
+        $defaults['_converted'][] = $var;

Also, the phpDoc states the direct *opposite* of what the code is doing.

Uhm, looks like excessive omission can be misleading. Look at this:

+   * It will not process variables which are marked as converted. It will mark
+   * any variable it processes as converted.
...
+    foreach ($variables as $var) {
... 
+      // Only upcast if there is a type with the name of the dealiased variable
+      // and the variable hasn't been upcast yet.
+      if (in_array($dealiased_var, $types) && ! in_array($var, $defaults['_converted'])) {
... do upcasting ...
+        $defaults['_converted'][] = $var;
+      }
+    }

I am not able to make sense of "ExplicitEntityConverter" vs. "EntityConverter". Both are in the same namespace, and one of both just happens to be registered before the other, so what gives?

The distinction should be clear from the explanation above. Just to make sure that the options set in YAML take precedence over the "guessing" of the entity type by name, ExplicitEntityConverter has to run before EntityConverter.
There is a test with a ridiculously made up edge case to showcase this:

+  options:
+    converters:
+      user: 'node'

+      } else {

Minor: Various (but heavy) code style issues here and elsewhere.

Now that I looked over the code again and again to argue against the last comments, I found some of the code style issues myself. (Although I don't consider an inline else as heavy issue, but that's probably because I'm an old yaph. :P)

There will definitely be a reroll, but before this we should agree on naming.

I'm not sure if "Loader" is a good choice: ParamLoaderManager sounds like something that provides parameters after loading whatsoever. Here we provide something which is loaded by taking the parameters as argument.

What came to my mind was the term "Inflation": Converting parameters to "fully blown" entities.

So what about ParamInflatorManager, ParamInflatorInterface, EntityInflator, ExplicitEntityInflator?

So, Dries, this is why it was made like this. Surely it's possible to merge these two converters. If you are willing to trade this (admittedly excessive) flexibility for one object and a method call, let me know.

Yes, to me it felt like we are over-engineering this, hence my suggestion to have a single EntityConverter. Why would someone want to plug in another converter in between these two specific converters? If they want to change the behavior, they could also register another Converter before and/or after this one, or even replace this Converter with a completely new one. It just feels like we're going a bit overboard with offering flexibility, at the cost of the developer's experience.

I merged the two converters and took care of the coding style issues.

Also, Crell proposed eliminating the request from ParamConverterInterface to make it easier for access checkers to use this outside of the route enhancing/request pipeline.

I opened #1906810: Require type hints for automatic entity upcasting as a follow-up.

I just had a chat with @goechsler. Here is the (simplified/slightly modified backscroll)... Note: We agree to move ANY of this (if we even agree on it / consider it) to a follow-up. Architecturally, even though I don't agree to some parts as described in the backscroll and previously in this issue we should still move forward. My concerns are just nuances and can be fixed later as this already works they way it is.

goechsler: Should we filter all those _route and _route_object keys out of defaults?
fubhy: If you ask me, converters should not have to worry about that stuff at all.
fubhy: They should not have to write to an array. Instead our enhancer that runs the converters should do that for them.
goechsler: Well, that's what im proposing. I want to clean the defaults array and just hand the variables to the enhancers.
goechsler: But I introduced one of those underscore variables myself. "_converted" - I'm not happy with it anymore :/
fubhy: That one confused me too :)
fubhy: Okay, but what does the converter get? If you ask me, the converter should only get whatever is in it's scope
goechsler: I use this _converted array to keep track of every variable that was touched by a converter.
fubhy: Okay, so instead of _converted it should be sufficient if the converter simply returns an object. If it does, you know it has been converted. If the return value is NULL, well.. then it hasn't been converted
goechsler: So we would just check, if any of the variables coming back from the converters is NULL and do 404, if so
fubhy: If i read the code correctly we are currently iterating over all converters in any case
goechsler: True. But right now, there's only on converter. I merged the explicit and the guessing one
fubhy: Instead, I would iterate over the variables and then invoke the converters for single variables one by one.
fubhy: so that the return value from a converter is always an upcast object for a particular variable or NULL
fubhy: once a converter returns something that is not NULL, we break out of the loop...
fubhy: If none of the converters had anything for a particular {slug} ==> HttpNotFoundException
goechsler: why should I call the converter n times, if I could do it all at once?
fubhy: Why does it matter? Either you loop over the variables in the converter or in the manager.
fubhy: But I find it cleaner if we do that once in the manager and then the converter only has to worry about upcasting a single variable: Separation of concerns
fubhy: A converter upcasts a single {slug} while the manager worries about managing that conversion process by keeping a list of the available converters and variables and invoking the converters one by one, one variable at a time.
fubhy: simply speaking...
fubhy: that way you would also get rid of $defaults in the converters
goechsler: what about the dependent parameters? I you don't have all the variables arround in the converter, it's hard to do that
fubhy: the converter really should just get the $route objects (to read from the options) and the current state of the $variables array (including previously upcast request arguments).
fubhy: Dependent parameters would work through something like constraints (@see #1906810: Require type hints for automatic entity upcasting) ... and yes, we can forward the variables. Forwarding the variables is not the problem, but the converter should only have to worry about one at a time if you ask me.
fubhy: But thats just my opinion. To me, the manager should be responsible for distributing the work while the converter should only have to worry about fullfiling a single upcasting job. We could greatly simplify the converters that way
goechsler: Well we agreed on the architecture like it is. But I see your point./blockquote:

Here's ParamConverterInterface with a renamed parameter and I was able to save a few bytes of whitespace, yay! For it is sunday, I added an interdiff.

Since we agreed to solve the remaining concerns in the follow-ups (I've yet to set up another follow-up for simplifying the converters field of responsibility @see comment #125) I am okay with settings this back to RTBC. Here comes my last nitpicky review for now.

+++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.phpundefined
@@ -0,0 +1,108 @@
+/**
+ * This class allows the upcasting of entity ids to the respective entity
+ * object.
+ */
+class EntityConverter implements ParamConverterInterface {
+  /**
+   * Entity manager which perform the upcasting in the end.
+   *
+   * @var \Drupal\Core\Entity\EntityManager

We are missing an empty line there. Also: "Entity manager which _performs_ the upcasting in the end." Note the missing "s".

+++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.phpundefined
@@ -0,0 +1,108 @@
+   * @return array
+   *   The modified defaults.

The modified variables now, eh? :)

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.phpundefined
@@ -0,0 +1,84 @@
+class ParamConverterManager implements RouteEnhancerInterface {
+
+  protected $converters;
+

The $converters could use a docblock.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.phpundefined
@@ -0,0 +1,84 @@
+   * Add a converter to the paramconverter service.

"Add_s_ a converter to the paramconverter service."

This only addresses the nitpicks from #127.

I have not looked at Drupal\openid\Tests\Upgrade\OpenIDAuthmapUpgradePathTest yet.

I just wondered, if this might eventually start to shine, if we polish it any longer...

This looks great overall. I noticed a lot of nitpicky stuff, some of which is below but I didn't include everything, also one question about the enhance() method on the ParamConverterManager.

+++ b/core/lib/Drupal/Core/CoreBundle.phpundefined
@@ -287,6 +295,9 @@ public function build(ContainerBuilder $container) {
+    // Add a compiler pass for upcasting of entity route parameters.
+    $container->addCompilerPass(new RegisterParamConvertersPass());
+    $container->addCompilerPass(new RegisterRouteEnhancersPass());

This isn't really for registering of entity route parameters at all.

+++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.phpundefined
@@ -0,0 +1,106 @@
+  /**
+   * Tries to upcast every variable to an entity type.
+   *
+   * If there is a type denoted in the route options it will try to upcast to
+   * it, if there is no definition in the options it will try to upcast to an
+   * entity type of that name. If the chosen enity type does not exists it will
+   * leave the variable untouched.
+   * If the entity type exist, but there is no entity with the given id it will
+   * convert the variable to NULL.
+   *
+   * Example:
+   *
+   * pattern: '/a/{user}/some/{foo}/and/{bar}/'
+   * options:
+   *   converters:
+   *     foo: 'node'
+   *
+   * The value for {user} will be converted to a user entity and the value
+   * for {foo} to a node entity, but it will not touch the value for {bar}.
+   *
+   * It will not process variables which are marked as converted. It will mark
+   * any variable it processes as converted.
+   *
+   * @param array &$variables
+   *   Array of values to convert to their corresponding objects, if applicable.
+   * @param \Symfony\Component\Routing\Route $route
+   *   The route object.
+   * @param array &$converted
+   *   Array collecting the names of all variables which have been
+   *   altered by a converter.
+   */
+  public function process(array &$variables, Route $route, array &$converted) {

I really like this. It feels a lot nicer than a service per entity type, and it's easy to follow.

+++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.phpundefined
@@ -0,0 +1,106 @@
+   * If the entity type exist, but there is no entity with the given id it will

s/exist/exists/

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterInterface.phpundefined
@@ -0,0 +1,29 @@
+  /**
+   * Allows to convert variables to their corresponding objects.

I don't really understand this summary, really a lot of the phpdoc from the entity converter could be moved to here if it was slightly more generic.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.phpundefined
@@ -0,0 +1,89 @@
+
+/**
+ * Provides a service which allows to enhance (say alter) the arguments coming
+ * from the URL.
+ *
+ * A typical use case for this would be upcasting a node id to a node entity.
+ *
+ * This class will not enhance any of the arguments itself, but allow other
+ * services to register to do so.

Summary isn't one line.

The enhance method gets a request object, but that isn't used anywhere. Does this mean then that when we're upcasting router objects for menu links (which is going to have to happen for access checks) then we'll need to mock the request object each time? It'd be good to avoid that, and seems a shame to be doing it even if our default implementation doesn't even require it.

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.phpundefined
@@ -0,0 +1,89 @@
+    // 1. It might prevent converters later in the pipeline to process
+    //    a variable again.
+    // 2. To check if upcasting was successfull after each converter had

"prevent ... to process", then successful is mis-spelled later. I'm not doing a full comment review nitpick atm but there's a lot of grammar issues in the commments generally.

+++ b/core/modules/system/lib/Drupal/system/Tests/ParamConverter/UpcastingTest.phpundefined
@@ -0,0 +1,77 @@
+   *
+   * All of these requests end up being proccessed by a controller with this
+   * the signature: f($user, $node, $foo) returning either values or labels

"with this the signature.". Also while it'd be fun to have a function called f() that seems like an unnecessary abbreviation here.

finally got to reviewing this - yay!

i'm really happy with what i see here. parameter upcasting as a step in route enhancement, with its own layer of indirection to support multiple approaches to upcasting, but with a sane default for entities. yay. nicely enough, i also see exactly what i need to do in order to set up the route enhancer for displays :)

as catch said, a number of nits on docs. i didn't detail those, either. however:

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.php
@@ -0,0 +1,89 @@
+    $converters = array();
+
+    $route = $defaults[RouteObjectInterface::ROUTE_OBJECT];
+
+    foreach ($this->converters as $converter) {
+      $converter->process($defaults, $route, $converters);

nit: it's weird that the name of this variable containing upcasted/converted variables is $converters. over in EntityConverter it's called $converted, and that's much better. let's change it to that here.

I don't want to un-RTBC this issue, because I'm fine with remaining work being done in a follow up. However, posting my own DX improvement suggestions here as an interdiff on top of #131, in case committers kick this issue back for more DX polish. It doesn't address all of #133, but does indirectly address some of it and #134.

This blocks so many things I've gone ahead and committed it. However let's leave it as a critical task until there's a follow-up with the feedback post #131 'cos it's not quite shiny yet ;)

It's in! :)

Now, back to polishing. Wax on, wax off ...

Sorry, incomplete patch. Let's try this.

Does this mean then that when we're upcasting router objects for menu links (which is going to have to happen for access checks) then we'll need to mock the request object each time? It'd be good to avoid that,

How about moving the code to an other method and glueing it to enhance()?

  public function enhance(array $defaults, Request $request) {
    $route = $defaults[RouteObjectInterface::ROUTE_OBJECT];

    return $this->convert($defaults, $route);
  }

  public function convert(array $variables, Route $route) {
    // do stuff

Can we create a follow-up issue for that stuff and do the change notification first?

Some other issues of #115 were also not addressed yet. The two I care about most are:

1) current_path() falls back to the non-request code when in request scope, which doesn't make sense to me.

2) The documentation on how paramconverter implementations work should be moved entirely from the EntityConverter implementation into the interface.

This has been waiting for a change notice for over 3 weeks now. Let's please get this knocked out.

I changed the original change notice for the routing system a bit: http://drupal.org/node/1800686/revisions/view/2555920/2587906

Looks like catch may be best to review this one to make sure it's shiny. :)

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.phpundefined
@@ -61,29 +58,45 @@ public function addConverter(ParamConverterInterface $converter) {
+   * @param array &$variables
+   *   Array of values to convert to their corresponding objects, if applicable.
+   * @param \Symfony\Component\Routing\Route $route
+   *   The route object.
+   *
+   * @return array
+   *   The modified variables.
+   */

Variables is passed by reference in the phpdoc, not in the method signature, and then it's returned anyway. Let's pick one.

Also, while we are at it let's change this in a way that allows ParamConverters to only interfere with {slug}'s. Currently it can basically inject anything into $defaults which renders ParamConverters rather pointless because they are more or less able to do the same as full-fledged RouteEnhancers (the only difference basically being the obligatory HttpNotFoundException() that's automatically invoked if upcasting fails).

Something along the lines of this:

EDIT: That was nonsense.. Will post a patch for that.

Ugh, let's really file followup issues instead of repurposing the original issue into followups. It took me several minutes to figure out what the status was here. @g.oechsler or @fubhy or someone else, could you please file the followup as requested in #136 and move these patches there? I don't think the current patch is major, but actually I really have no idea.

Okay, sure!

#1943846: Improve ParamConverterManager and developer experience for ParamConverters

Thanks @fubhy! :)

Er wait. I think this is still CNW as of #148.

@webchick, the followup issue includes the patch that is CNW as far as I can see. There was already a commit in this issue back in #136. (Not changing the status in case I am wrong.)

Oops my mistake. I thought it was just for fubhy's concern in #149. Cool then. :)

I've been looking at implementing this for edit module, but since there are custom loading arguments needed there (type/id across entities for example), we need the fullest API used there. The code comments in the custom example does not sound very promising...

   * I'm pretty sure the _access syntax is wrong in this case. And have no idea how to pass arguments to the access callback.
   * This is pretty much a copy/paste of RouteTestSubscriber, and it probably doesn't work fully.

It would be great to get some eyes on this example, since it is crucial for the conversion of Drupal and the contrib modules and *especially* for security.

The edit module uses a requirements key (http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...) and the access class validates and upcasts the path arguments (http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...). This was made up before this solution landed obviously, but I'm not seeing how we could apply the new approach based on the docs.

@Gabor: here's an example of how you can integrate {entity_type} and {entity} into the existing EntityConverter. To do {field_name} and {langcode}, I suggest waiting on #1906810: Require type hints for automatic entity upcasting, unless there's some pressing need to get to it sooner. If there is that need, you'd need to create your own class that implements ParamConverterInterface and register it as a service with the 'paramconverter' tag. You may also be interested in #1943846: Improve ParamConverterManager and developer experience for ParamConverters if you run into DX difficulties with that.

And have no idea how to pass arguments to the access callback.

The access check service's access() method receives a $request. Everything it needs should be available in $request->attributes.

Thanks @effulgentsia for the tips, I've opened #1979566: Use request upcasting functionality in edit module with a slightly improved patch :) Adding test coverage as well.

Adding links to help with context or learning about upcasting. They're

- https://www.drupal.org/docs/8/api/routing-system/parameter-upcasting-in-...
- https://www.drupal.org/docs/8/api/routing-system/how-a-node-id-gets-conv...