Issue #1943846 by fubhy, effulgentsia, tim.plunkett: Improve...

Issue #1943846 by fubhy, effulgentsia, tim.plunkett: Improve ParamConverterManager and developer experience for ParamConverters.

core/core.services.yml

@@ -293,13 +293,25 @@ services:
       - { name: route_filter }
   paramconverter_manager:
     class: Drupal\Core\ParamConverter\ParamConverterManager
+    calls:
+      - [setContainer, ['@service_container']]
     tags:
       - { name: route_enhancer }
+  paramconverter_subscriber:
+    class: Drupal\Core\EventSubscriber\ParamConverterSubscriber
+    tags:
+      - { name: event_subscriber }
+    arguments: ['@paramconverter_manager']
   paramconverter.entity:
     class: Drupal\Core\ParamConverter\EntityConverter
     tags:
       - { name: paramconverter }
     arguments: ['@plugin.manager.entity']
+  route_subscriber.entity:
+    class: Drupal\Core\EventSubscriber\EntityRouteAlterSubscriber
+    tags:
+      - { name: event_subscriber }
+    arguments: ['@plugin.manager.entity']
   reverse_proxy_subscriber:
     class: Drupal\Core\EventSubscriber\ReverseProxySubscriber
     tags:

core/lib/Drupal/Core/CoreServiceProvider.php

@@ -55,7 +55,7 @@ public function register(ContainerBuilder $container) {
     // Add a compiler pass for registering event subscribers.
     $container->addCompilerPass(new RegisterKernelListenersPass(), PassConfig::TYPE_AFTER_REMOVING);
     $container->addCompilerPass(new RegisterAccessChecksPass());
-    // Add a compiler pass for upcasting of entity route parameters.
+    // Add a compiler pass for upcasting route parameters.
     $container->addCompilerPass(new RegisterParamConvertersPass());
     $container->addCompilerPass(new RegisterRouteEnhancersPass());
     // Add a compiler pass for registering services needing destruction.

core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterParamConvertersPass.php

@@ -23,26 +23,14 @@ class RegisterParamConvertersPass implements CompilerPassInterface {
    *   The container to process.
    */
   public function process(ContainerBuilder $container) {
-
     if (!$container->hasDefinition('paramconverter_manager')) {
       return;
     }
 
     $manager = $container->getDefinition('paramconverter_manager');
-
-    $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));
     }
   }
 }

core/lib/Drupal/Core/EventSubscriber/EntityRouteAlterSubscriber.php

+<?php
+
+/**
+ * @file
+ * Contains Drupal\Core\EventSubscriber\EntityRouteAlterSubscriber.
+ */
+
+namespace Drupal\Core\EventSubscriber;
+
+use Drupal\Core\Entity\EntityManager;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Drupal\Core\Routing\RoutingEvents;
+use Drupal\Core\Routing\RouteBuildEvent;
+
+/**
+ * Registers the 'type' of route parameter names that match an entity type.
+ *
+ * @todo Matching on parameter *name* is not ideal, because it breaks
+ *   encapsulation: parameter names are local to the controller and route, and
+ *   controllers and routes can't be expected to know what all possible entity
+ *   types might exist across all modules in order to pick names that don't
+ *   conflict. Instead, the 'type' should be determined from introspecting what
+ *   kind of PHP variable (e.g., a type hinted interface) the controller
+ *   requires: https://drupal.org/node/2041907.
+ */
+class EntityRouteAlterSubscriber implements EventSubscriberInterface {
+
+  /**
+   * Entity manager.
+   *
+   * @var \Drupal\Core\Entity\EntityManager
+   */
+  protected $entityManager;
+
+  /**
+   * Constructs a new EntityRouteAlterSubscriber.
+   *
+   * @param \Drupal\Core\Entity\EntityManager $entity_manager
+   *   The entity manager.
+   */
+  public function __construct(EntityManager $entity_manager) {
+    $this->entityManager = $entity_manager;
+  }
+
+  /**
+   * Applies parameter converters to route parameters.
+   *
+   * @param \Drupal\Core\Routing\RouteBuildEvent $event
+   *   The event to process.
+   */
+  public function onRoutingRouteAlterSetType(RouteBuildEvent $event) {
+    $entity_types = array_keys($this->entityManager->getDefinitions());
+    foreach ($event->getRouteCollection() as $route) {
+      $parameter_definitions = $route->getOption('parameters') ?: array();
+      // For all route parameter names that match an entity type, add the 'type'
+      // to the parameter definition if it's not already explicitly provided.
+      foreach (array_intersect($route->compile()->getVariables(), $entity_types) as $parameter_name) {
+        if (!isset($parameter_definitions[$parameter_name])) {
+          $parameter_definitions[$parameter_name] = array();
+        }
+        $parameter_definitions[$parameter_name] += array(
+          'type' => 'entity:' . $parameter_name,
+        );
+      }
+      if (!empty($parameter_definitions)) {
+        $route->setOption('parameters', $parameter_definitions);
+      }
+    }
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  static function getSubscribedEvents() {
+    $events[RoutingEvents::ALTER][] = array('onRoutingRouteAlterSetType', 100);
+    return $events;
+  }
+}

core/lib/Drupal/Core/EventSubscriber/ParamConverterSubscriber.php

+<?php
+
+/**
+ * @file
+ * Contains Drupal\Core\EventSubscriber\ParamConverterSubscriber.
+ */
+
+namespace Drupal\Core\EventSubscriber;
+
+use Drupal\Core\ParamConverter\ParamConverterManager;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Drupal\Core\Routing\RoutingEvents;
+use Drupal\Core\Routing\RouteBuildEvent;
+
+/**
+ * Event subscriber for registering parameter converters with routes.
+ */
+class ParamConverterSubscriber implements EventSubscriberInterface {
+
+  /**
+   * The parameter converter manager.
+   *
+   * @var \Drupal\Core\ParamConverter\ParamConverterManager
+   */
+  protected $paramConverterManager;
+
+  /**
+   * Constructs a new ParamConverterSubscriber.
+   *
+   * @param \Drupal\Core\ParamConverter\ParamConverterManager $param_converter_manager
+   *   The parameter converter manager that will be responsible for upcasting
+   *   request attributes.
+   */
+  public function __construct(ParamConverterManager $param_converter_manager) {
+    $this->paramConverterManager = $param_converter_manager;
+  }
+
+  /**
+   * Applies parameter converters to route parameters.
+   *
+   * @param \Drupal\Core\Routing\RouteBuildEvent $event
+   *   The event to process.
+   */
+  public function onRoutingRouteAlterSetParameterConverters(RouteBuildEvent $event) {
+    $this->paramConverterManager->setRouteParameterConverters($event->getRouteCollection());
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  static function getSubscribedEvents() {
+    $events[RoutingEvents::ALTER][] = array('onRoutingRouteAlterSetParameterConverters', 10);
+    return $events;
+  }
+}

core/lib/Drupal/Core/ParamConverter/EntityConverter.php

@@ -12,8 +12,7 @@
 use Drupal\Core\Entity\EntityManager;
 
 /**
- * This class allows the upcasting of entity ids to the respective entity
- * object.
+ * Parameter converter for upcasting entity ids to full objects.
  */
 class EntityConverter implements ParamConverterInterface {
 
@@ -30,74 +29,29 @@ class EntityConverter implements ParamConverterInterface {
    * @param \Drupal\Core\Entity\EntityManager $entityManager
    *   The entity manager.
    */
-  public function __construct(EntityManager $entityManager) {
-    $this->entityManager = $entityManager;
+  public function __construct(EntityManager $entity_manager) {
+    $this->entityManager = $entity_manager;
   }
 
   /**
-   * 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.
+   * {@inheritdoc}
    */
-  public function process(array &$variables, Route $route, array &$converted) {
-    $variable_names = $route->compile()->getVariables();
-
-    $options = $route->getOptions();
-    $configuredTypes = isset($options['converters']) ? $options['converters'] : array();
-
-    $entityTypes = array_keys($this->entityManager->getDefinitions());
-
-    foreach ($variable_names as $name) {
-      // Do not process this variable if it's already marked as converted.
-      if (in_array($name, $converted)) {
-        continue;
-      }
-
-      // Obtain entity type to convert to from the route configuration or just
-      // use the variable name as default.
-      if (array_key_exists($name, $configuredTypes)) {
-        $type = $configuredTypes[$name];
-      }
-      else {
-        $type = $name;
-      }
-
-      if (in_array($type, $entityTypes)) {
-        $value = $variables[$name];
-
-        $storageController = $this->entityManager->getStorageController($type);
-        $entity = $storageController->load($value);
-        $variables[$name] = $entity;
+  public function convert($value, $definition, $name, array $defaults, Request $request) {
+    $entity_type = substr($definition['type'], strlen('entity:'));
+    if ($storage = $this->entityManager->getStorageController($entity_type)) {
+      return $storage->load($value);
+    }
+  }
 
-        // Mark this variable as converted.
-        $converted[] = $name;
-      }
+  /**
+   * {@inheritdoc}
+   */
+  public function applies($definition, $name, Route $route) {
+    if (!empty($definition['type']) && strpos($definition['type'], 'entity:') === 0) {
+      $entity_type = substr($definition['type'], strlen('entity:'));
+      return (bool) $this->entityManager->getDefinition($entity_type);
     }
+    return FALSE;
   }
+
 }

core/lib/Drupal/Core/ParamConverter/ParamConverterInterface.php

@@ -7,6 +7,7 @@
 
 namespace Drupal\Core\ParamConverter;
 
+use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\Routing\Route;
 
 /**
@@ -17,13 +18,36 @@ interface ParamConverterInterface {
   /**
    * Allows to convert variables to their corresponding objects.
    *
-   * @param array &$variables
-   *   Array of values to convert to their corresponding objects, if applicable.
+   * @param mixed $value
+   *   The raw value.
+   * @param mixed $definition
+   *   The parameter definition provided in the route options.
+   * @param string $name
+   *   The name of the parameter.
+   * @param array $defaults
+   *   The route defaults array.
+   * @param \Symfony\Component\HttpFoundation\Request $request
+   *   The request object.
+   *
+   * @return mixed|null
+   *   The converted parameter value.
+   */
+  public function convert($value, $definition, $name, array $defaults, Request $request);
+
+  /**
+   * Determines if the converter applies to a specific route and variable.
+   *
+   * @param mixed $definition
+   *   The parameter definition provided in the route options.
+   * @param string $name
+   *   The name of the parameter.
    * @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.
+   *   The route to consider attaching to.
+   *
+   * @return bool
+   *   TRUE if the converter applies to the passed route and parameter, FALSE
+   *   otherwise.
    */
-  public function process(array &$variables, Route $route, array &$converted);
+  public function applies($definition, $name, Route $route);
+
 }

core/lib/Drupal/Core/ParamConverter/ParamConverterManager.php

@@ -11,79 +11,175 @@
 use Symfony\Cmf\Component\Routing\Enhancer\RouteEnhancerInterface;
 use Symfony\Cmf\Component\Routing\RouteObjectInterface;
 use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
+use Symfony\Component\Routing\RouteCollection;
 use Symfony\Component\HttpFoundation\Request;
 
-use Drupal\Core\ParamConverter\ParamConverterInterface;
-
 /**
- * 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.
+ * Manages converter services for converting request parameters to full objects.
  *
- * This class will not enhance any of the arguments itself, but allow other
- * services to register to do so.
+ * A typical use case for this would be upcasting (converting) a node id to a
+ * node entity.
  */
-class ParamConverterManager implements RouteEnhancerInterface {
+class ParamConverterManager extends ContainerAware implements RouteEnhancerInterface {
+
+  /**
+   * An array of registered converter service ids.
+   *
+   * @var array
+   */
+  protected $converterIds = array();
+
+  /**
+   * Array of registered converter service ids sorted by their priority.
+   *
+   * @var array
+   */
+  protected $sortedConverterIds;
 
   /**
-   * Converters managed by the ParamConverterManager.
+   * Array of loaded converter services keyed by their ids.
    *
    * @var array
    */
-  protected $converters;
+  protected $converters = array();
 
   /**
-   * Adds a converter to the paramconverter service.
+   * Registers a parameter converter with the manager.
    *
-   * @see \Drupal\Core\DependencyInjection\Compiler\RegisterParamConvertersPass
+   * @param string $converter
+   *   The parameter converter service id to register.
+   * @param int $priority
+   *   (optional) The priority of the converter. Defaults to 0.
    *
-   * @param \Drupal\Core\ParamConverter\ParamConverterInterface $converter
-   *   The converter to add.
+   * @return \Drupal\Core\ParamConverter\ParamConverterManager
+   *   The called object for chaining.
    */
-  public function addConverter(ParamConverterInterface $converter) {
-    $this->converters[] = $converter;
+  public function addConverter($converter, $priority = 0) {
+    if (empty($this->converterIds[$priority])) {
+      $this->converterIds[$priority] = array();
+    }
+    $this->converterIds[$priority][] = $converter;
+    unset($this->sortedConverterIds);
     return $this;
   }
 
   /**
-   * Implements \Symfony\Cmf\Component\Routing\Enhancer\ŖouteEnhancerIterface.
+   * Sorts the converter service ids and flattens them.
+   *
+   * @return array
+   *   The sorted parameter converter service ids.
+   */
+  public function getConverterIds() {
+    if (!isset($this->sortedConverterIds)) {
+      krsort($this->converterIds);
+      $this->sortedConverterIds = array();
+      foreach ($this->converterIds as $resolvers) {
+        $this->sortedConverterIds = array_merge($this->sortedConverterIds, $resolvers);
+      }
+    }
+    return $this->sortedConverterIds;
+  }
+
+  /**
+   * Lazy-loads converter services.
+   *
+   * @param string $converter
+   *   The service id of converter service to load.
+   *
+   * @return \Drupal\Core\ParamConverter\ParamConverterInterface
+   *   The loaded converter service identified by the given service id.
+   *
+   * @throws \InvalidArgumentException
+   *   If the given service id is not a registered converter.
+   */
+  public 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($converter);
+  }
+
+  /**
+   * Saves a list of applicable converters to each route.
    *
-   * Iterates over all registered converters and allows them to alter the
-   * defaults.
+   * @param \Symfony\Component\Routing\RouteCollection $routes
+   *   A collection of routes to apply converters to.
+   */
+  public function setRouteParameterConverters(RouteCollection $routes) {
+    foreach ($routes->all() as $route) {
+      if (!$parameters = $route->getOption('parameters')) {
+        // Continue with the next route if no parameters have been defined.
+        continue;
+      }
+
+      // Loop over all defined parameters and look up the right converter.
+      foreach ($parameters as $name => &$definition) {
+        if (isset($definition['converter'])) {
+          // Skip parameters that already have a manually set converter.
+          continue;
+        }
+
+        foreach ($this->getConverterIds() as $converter) {
+          if ($this->getConverter($converter)->applies($definition, $name, $route)) {
+            $definition['converter'] = $converter;
+            break;
+          }
+        }
+      }
+
+      // Override the parameters array.
+      $route->setOption('parameters', $parameters);
+    }
+  }
+
+  /**
+   * Invokes the registered converter for each defined parameter on a route.
    *
    * @param array $defaults
-   *   The getRouteDefaults array.
+   *   The route defaults array.
    * @param \Symfony\Component\HttpFoundation\Request $request
    *   The current request.
    *
+   * @throws \Symfony\Component\HttpKernel\Exception\NotFoundHttpException
+   *   If one of the assigned converters returned NULL because the given
+   *   variable could not be converted.
+   *
    * @return array
    *   The modified defaults.
    */
   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.
-    $converters = array();
-
     $route = $defaults[RouteObjectInterface::ROUTE_OBJECT];
 
-    foreach ($this->converters as $converter) {
-      $converter->process($defaults, $route, $converters);
+    // Skip this enhancer if there are no parameter definitions.
+    if (!$parameters = $route->getOption('parameters')) {
+      return $defaults;
     }
 
-    // Check if all upcasting yielded a result.
-    // If an upcast value is NULL do a 404.
-    foreach ($converters as $variable) {
-      if ($defaults[$variable] === NULL) {
+    // Invoke the registered converter for each parameter.
+    foreach ($parameters as $name => $definition) {
+      if (!isset($defaults[$name])) {
+        // Do not try to convert anything that is already set to NULL.
+        continue;
+      }
+
+      if (!isset($definition['converter'])) {
+        // Continue if no converter has been specified.
+        continue;
+      }
+
+      // If a converter returns NULL it means that the parameter could not be
+      // converted in which case we throw a 404.
+      $defaults[$name] = $this->getConverter($definition['converter'])->convert($defaults[$name], $definition, $name, $defaults, $request);
+      if (!isset($defaults[$name])) {
         throw new NotFoundHttpException();
       }
     }
 
     return $defaults;
   }
+
 }
+

core/modules/system/lib/Drupal/system/Tests/ParamConverter/UpcastingTest.php

@@ -50,15 +50,13 @@ public function testUpcasting() {
     $this->assertRaw("user: {$user->label()}, node: {$node->label()}, foo: $foo", 'user and node upcast by entity name');
 
     // paramconverter_test/test_node_user_user/{node}/{foo}/{user}
-    // converters:
-    //   foo: 'user'
-    $this->drupalGet('paramconverter_test/test_node_user_user/' . $node->id() . '/' . $user->id() . "/" . $user->id());
+    // options.parameters.foo.type = entity:user
+    $this->drupalGet("paramconverter_test/test_node_user_user/{$node->nid}/" . $user->id() . "/" . $user->id());
     $this->assertRaw("user: {$user->label()}, node: {$node->label()}, foo: {$user->label()}", 'foo converted to user as well');
 
     // paramconverter_test/test_node_node_foo/{user}/{node}/{foo}
-    // converters:
-    //   user: 'node'
-    $this->drupalGet('paramconverter_test/test_node_node_foo/' . $node->id() . '/' . $node->id() . "/$foo");
+    // options.parameters.user.type = entity:node
+    $this->drupalGet("paramconverter_test/test_node_node_foo/{$node->nid}/{$node->nid}/$foo");
     $this->assertRaw("user: {$node->label()}, node: {$node->label()}, foo: $foo", 'user is upcast to node (rather than to user)');
   }
 
@@ -69,9 +67,8 @@ public function testSameTypes() {
     $node = $this->drupalCreateNode(array('title' => $this->randomName(8)));
     $parent = $this->drupalCreateNode(array('title' => $this->randomName(8)));
     // paramconverter_test/node/{node}/set/parent/{parent}
-    // converters:
-    //   parent: 'node'
-    $this->drupalGet("paramconverter_test/node/" . $node->id() . "/set/parent/" . $parent->id());
+    // options.parameters.parent.type = entity:node
+    $this->drupalGet("paramconverter_test/node/" . $node->nid . "/set/parent/" . $parent->nid);
     $this->assertRaw("Setting '" . $parent->title . "' as parent of '" . $node->title . "'.")