Issue #2185831 by tim.plunkett: Split up ParamConverterManager and stop...

Issue #2185831 by tim.plunkett: Split up ParamConverterManager and stop throwing NotFoundHttpException.

core/core.services.yml

@@ -342,8 +342,6 @@ services:
     class: Drupal\Core\ParamConverter\ParamConverterManager
     calls:
       - [setContainer, ['@service_container']]
-    tags:
-      - { name: route_enhancer }
   paramconverter_subscriber:
     class: Drupal\Core\EventSubscriber\ParamConverterSubscriber
     tags:
@@ -373,6 +371,12 @@ services:
     class: Drupal\Core\EventSubscriber\AjaxResponseSubscriber
     tags:
       - { name: event_subscriber }
+  route_enhancer.param_conversion:
+    class: Drupal\Core\Routing\Enhancer\ParamConversionEnhancer
+    arguments: ['@paramconverter_manager']
+    tags:
+      - { name: route_enhancer }
+      - { name: event_subscriber }
   route_enhancer.authentication:
     class: Drupal\Core\Routing\Enhancer\AuthenticationEnhancer
     calls:

core/includes/menu.inc

@@ -9,13 +9,13 @@
 use Drupal\Component\Utility\String;
 use Drupal\Core\Cache\Cache;
 use Drupal\Core\Language\Language;
+use Drupal\Core\ParamConverter\ParamNotConvertedException;
 use Drupal\Core\Routing\RequestHelper;
 use Drupal\Core\Template\Attribute;
 use Drupal\menu_link\Entity\MenuLink;
 use Drupal\menu_link\MenuLinkStorageController;
 use Symfony\Cmf\Component\Routing\RouteObjectInterface;
 use Symfony\Component\HttpFoundation\Request;
-use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
 use Symfony\Component\Routing\Route;
 
 /**
@@ -934,7 +934,7 @@ function menu_item_route_access(Route $route, $href, &$map, Request $request = N
   try {
     $request->attributes->add(\Drupal::service('router')->matchRequest($request));
   }
-  catch (NotFoundHttpException $e) {
+  catch (ParamNotConvertedException $e) {
     return FALSE;
   }
 

core/lib/Drupal/Core/Access/AccessManager.php

@@ -7,7 +7,8 @@
 
 namespace Drupal\Core\Access;
 
-use Drupal\Core\ParamConverter\ParamConverterManager;
+use Drupal\Core\ParamConverter\ParamConverterManagerInterface;
+use Drupal\Core\ParamConverter\ParamNotConvertedException;
 use Drupal\Core\Routing\Access\AccessInterface as RoutingAccessInterface;
 use Drupal\Core\Routing\RequestHelper;
 use Drupal\Core\Routing\RouteProviderInterface;
@@ -17,7 +18,6 @@
 use Symfony\Component\Routing\Route;
 use Symfony\Component\DependencyInjection\ContainerAware;
 use Symfony\Component\HttpFoundation\Request;
-use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
 use Symfony\Component\Routing\Exception\RouteNotFoundException;
 use Symfony\Cmf\Component\Routing\RouteObjectInterface;
 
@@ -73,7 +73,7 @@ class AccessManager extends ContainerAware {
   /**
    * The paramconverter manager.
    *
-   * @var \Drupal\Core\ParamConverter\ParamConverterManager
+   * @var \Drupal\Core\ParamConverter\ParamConverterManagerInterface
    */
   protected $paramConverterManager;
 
@@ -91,10 +91,10 @@ class AccessManager extends ContainerAware {
    *   The route provider.
    * @param \Symfony\Component\Routing\Generator\UrlGeneratorInterface $url_generator
    *   The url generator.
-   * @param \Drupal\Core\ParamConverter\ParamConverterManager $paramconverter_manager
+   * @param \Drupal\Core\ParamConverter\ParamConverterManagerInterface $paramconverter_manager
    *   The param converter manager.
    */
-  public function __construct(RouteProviderInterface $route_provider, UrlGeneratorInterface $url_generator, ParamConverterManager $paramconverter_manager) {
+  public function __construct(RouteProviderInterface $route_provider, UrlGeneratorInterface $url_generator, ParamConverterManagerInterface $paramconverter_manager) {
     $this->routeProvider = $route_provider;
     $this->urlGenerator = $url_generator;
     $this->paramConverterManager = $paramconverter_manager;
@@ -202,14 +202,14 @@ public function checkNamedRoute($route_name, array $parameters = array(), Accoun
         $defaults = $parameters + $route->getDefaults();
         $route_request = RequestHelper::duplicate($this->request, $this->urlGenerator->generate($route_name, $defaults));
         $defaults[RouteObjectInterface::ROUTE_OBJECT] = $route;
-        $route_request->attributes->add($this->paramConverterManager->enhance($defaults, $route_request));
+        $route_request->attributes->add($this->paramConverterManager->convert($defaults, $route_request));
       }
       return $this->check($route, $route_request, $account);
     }
     catch (RouteNotFoundException $e) {
       return FALSE;
     }
-    catch (NotFoundHttpException $e) {
+    catch (ParamNotConvertedException $e) {
       return FALSE;
     }
   }

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

@@ -7,7 +7,7 @@
 
 namespace Drupal\Core\EventSubscriber;
 
-use Drupal\Core\ParamConverter\ParamConverterManager;
+use Drupal\Core\ParamConverter\ParamConverterManagerInterface;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
 use Drupal\Core\Routing\RoutingEvents;
 use Drupal\Core\Routing\RouteBuildEvent;
@@ -20,18 +20,18 @@ class ParamConverterSubscriber implements EventSubscriberInterface {
   /**
    * The parameter converter manager.
    *
-   * @var \Drupal\Core\ParamConverter\ParamConverterManager
+   * @var \Drupal\Core\ParamConverter\ParamConverterManagerInterface
    */
   protected $paramConverterManager;
 
   /**
    * Constructs a new ParamConverterSubscriber.
    *
-   * @param \Drupal\Core\ParamConverter\ParamConverterManager $param_converter_manager
+   * @param \Drupal\Core\ParamConverter\ParamConverterManagerInterface $param_converter_manager
    *   The parameter converter manager that will be responsible for upcasting
    *   request attributes.
    */
-  public function __construct(ParamConverterManager $param_converter_manager) {
+  public function __construct(ParamConverterManagerInterface $param_converter_manager) {
     $this->paramConverterManager = $param_converter_manager;
   }
 

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

@@ -8,10 +8,7 @@
 namespace Drupal\Core\ParamConverter;
 
 use Symfony\Component\DependencyInjection\ContainerAware;
-use Symfony\Cmf\Component\Routing\Enhancer\RouteEnhancerInterface;
 use Symfony\Cmf\Component\Routing\RouteObjectInterface;
-use Symfony\Component\HttpFoundation\ParameterBag;
-use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
 use Symfony\Component\Routing\RouteCollection;
 use Symfony\Component\HttpFoundation\Request;
 
@@ -21,7 +18,7 @@
  * A typical use case for this would be upcasting (converting) a node id to a
  * node entity.
  */
-class ParamConverterManager extends ContainerAware implements RouteEnhancerInterface {
+class ParamConverterManager extends ContainerAware implements ParamConverterManagerInterface {
 
   /**
    * An array of registered converter service ids.
@@ -45,15 +42,7 @@ class ParamConverterManager extends ContainerAware implements RouteEnhancerInter
   protected $converters = array();
 
   /**
-   * Registers a parameter converter with the manager.
-   *
-   * @param string $converter
-   *   The parameter converter service id to register.
-   * @param int $priority
-   *   (optional) The priority of the converter. Defaults to 0.
-   *
-   * @return \Drupal\Core\ParamConverter\ParamConverterManager
-   *   The called object for chaining.
+   * {@inheritdoc}
    */
   public function addConverter($converter, $priority = 0) {
     if (empty($this->converterIds[$priority])) {
@@ -65,10 +54,7 @@ public function addConverter($converter, $priority = 0) {
   }
 
   /**
-   * Sorts the converter service ids and flattens them.
-   *
-   * @return array
-   *   The sorted parameter converter service ids.
+   * {@inheritdoc}
    */
   public function getConverterIds() {
     if (!isset($this->sortedConverterIds)) {
@@ -82,16 +68,7 @@ public function getConverterIds() {
   }
 
   /**
-   * 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.
+   * {@inheritdoc}
    */
   public function getConverter($converter) {
     if (isset($this->converters[$converter])) {
@@ -104,10 +81,7 @@ public function getConverter($converter) {
   }
 
   /**
-   * Saves a list of applicable converters to each route.
-   *
-   * @param \Symfony\Component\Routing\RouteCollection $routes
-   *   A collection of routes to apply converters to.
+   * {@inheritdoc}
    */
   public function setRouteParameterConverters(RouteCollection $routes) {
     foreach ($routes->all() as $route) {
@@ -137,33 +111,11 @@ public function setRouteParameterConverters(RouteCollection $routes) {
   }
 
   /**
-   * Invokes the registered converter for each defined parameter on a route.
-   *
-   * @param array $defaults
-   *   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.
+   * {@inheritdoc}
    */
-  public function enhance(array $defaults, Request $request) {
-    // Store a backup of the raw $defaults values corresponding to
-    // variables in the route path pattern.
+  public function convert(array $defaults, Request $request) {
+    /** @var $route \Symfony\Component\Routing\Route */
     $route = $defaults[RouteObjectInterface::ROUTE_OBJECT];
-    $variables = array_flip($route->compile()->getVariables());
-    // Foreach  will copy the values from the array it iterates. Even if they
-    // are references, use it to break them. This avoids any scenarios where raw
-    // variables also get replaced with converted values.
-    $raw_variables = array();
-    foreach (array_intersect_key($defaults, $variables) as $key => $value) {
-      $raw_variables[$key] = $value;
-    }
-    $defaults['_raw_variables'] = new ParameterBag($raw_variables);
 
     // Skip this enhancer if there are no parameter definitions.
     if (!$parameters = $route->getOption('parameters')) {
@@ -183,10 +135,10 @@ public function enhance(array $defaults, Request $request) {
       }
 
       // If a converter returns NULL it means that the parameter could not be
-      // converted in which case we throw a 404.
+      // converted.
       $defaults[$name] = $this->getConverter($definition['converter'])->convert($defaults[$name], $definition, $name, $defaults, $request);
       if (!isset($defaults[$name])) {
-        throw new NotFoundHttpException();
+        throw new ParamNotConvertedException(sprintf('The "%s" parameter was not converted for the path "%s" (route name: "%s")', $name, $route->getPath(), $defaults[RouteObjectInterface::ROUTE_NAME]));
       }
     }
 

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

+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Core\ParamConverter\ParamConverterManagerInterface.
+ */
+
+namespace Drupal\Core\ParamConverter;
+
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\Routing\RouteCollection;
+
+/**
+ * Provides an interface for a parameter converter manager.
+ */
+interface ParamConverterManagerInterface {
+
+  /**
+   * Registers a parameter converter with the manager.
+   *
+   * @param string $converter
+   *   The parameter converter service id to register.
+   * @param int $priority
+   *   (optional) The priority of the converter. Defaults to 0.
+   *
+   * @return $this
+   */
+  public function addConverter($converter, $priority = 0);
+
+  /**
+   * Sorts the converter service ids and flattens them.
+   *
+   * @return array
+   *   The sorted parameter converter service ids.
+   */
+    public function getConverterIds();
+
+  /**
+   * 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);
+
+  /**
+   * Saves a list of applicable converters to each route.
+   *
+   * @param \Symfony\Component\Routing\RouteCollection $routes
+   *   A collection of routes to apply converters to.
+   */
+  public function setRouteParameterConverters(RouteCollection $routes);
+
+  /**
+   * Invokes the registered converter for each defined parameter on a route.
+   *
+   * @param array $defaults
+   *   The route defaults array.
+   * @param \Symfony\Component\HttpFoundation\Request $request
+   *   The current request.
+   *
+   * @throws \Drupal\Core\ParamConverter\ParamNotConvertedException
+   *   If one of the assigned converters returned NULL because the given
+   *   variable could not be converted.
+   *
+   * @return array
+   *   The modified defaults.
+   */
+  public function convert(array $defaults, Request $request);
+
+}

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

+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Core\ParamConverter\ParamNotConvertedException.
+ */
+
+namespace Drupal\Core\ParamConverter;
+
+/**
+ * Provides an exception class for a request parameter that was not converted.
+ */
+class ParamNotConvertedException extends \Exception {
+
+}

core/lib/Drupal/Core/Routing/Enhancer/ParamConversionEnhancer.php

+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Core\Routing\Enhancer\ParamConversionEnhancer.
+ */
+
+namespace Drupal\Core\Routing\Enhancer;
+
+use Drupal\Core\ParamConverter\ParamConverterManagerInterface;
+use Drupal\Core\ParamConverter\ParamNotConvertedException;
+use Symfony\Cmf\Component\Routing\Enhancer\RouteEnhancerInterface;
+use Symfony\Cmf\Component\Routing\RouteObjectInterface;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Symfony\Component\HttpFoundation\ParameterBag;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent;
+use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
+use Symfony\Component\HttpKernel\KernelEvents;
+
+/**
+ * Provides a route enhancer that handles parameter conversion.
+ */
+class ParamConversionEnhancer implements RouteEnhancerInterface, EventSubscriberInterface {
+
+  /**
+   * The parameter conversion manager.
+   *
+   * @var \Drupal\Core\ParamConverter\ParamConverterManagerInterface
+   */
+  protected $paramConverterManager;
+
+  /**
+   * Constructs a new ParamConversionEnhancer.
+   *
+   * @param \Drupal\Core\ParamConverter\ParamConverterManagerInterface $param_converter_manager
+   *   The parameter conversion manager.
+   */
+  public function __construct(ParamConverterManagerInterface $param_converter_manager) {
+    $this->paramConverterManager = $param_converter_manager;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function enhance(array $defaults, Request $request) {
+    $defaults['_raw_variables'] = $this->copyRawVariables($defaults);
+    return $this->paramConverterManager->convert($defaults, $request);
+  }
+
+  /**
+   * Store a backup of the raw values that corresponding to the route pattern.
+   *
+   * @param array $defaults
+   *   The route defaults array.
+   *
+   * @return \Symfony\Component\HttpFoundation\ParameterBag
+   */
+  protected function copyRawVariables(array $defaults) {
+    /** @var $route \Symfony\Component\Routing\Route */
+    $route = $defaults[RouteObjectInterface::ROUTE_OBJECT];
+    $variables = array_flip($route->compile()->getVariables());
+    // Foreach will copy the values from the array it iterates. Even if they
+    // are references, use it to break them. This avoids any scenarios where raw
+    // variables also get replaced with converted values.
+    $raw_variables = array();
+    foreach (array_intersect_key($defaults, $variables) as $key => $value) {
+      $raw_variables[$key] = $value;
+    }
+    return new ParameterBag($raw_variables);
+  }
+
+  /**
+   * Catches failed parameter conversions and throw a 404 instead.
+   *
+   * @param \Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent $event
+   */
+  public function onException(GetResponseForExceptionEvent $event) {
+    $exception = $event->getException();
+    if ($exception instanceof ParamNotConvertedException) {
+      $event->setException(new NotFoundHttpException($exception->getMessage(), $exception));
+    }
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function getSubscribedEvents() {
+    $events[KernelEvents::EXCEPTION][] = array('onException', 0);
+    return $events;
+  }
+
+}

core/lib/Drupal/Core/Routing/UrlMatcher.php

@@ -30,7 +30,16 @@ public function finalMatch(RouteCollection $collection, Request $request) {
     $context = new RequestContext();
     $context->fromRequest($request);
     $this->setContext($context);
-    return $this->match('/' . $request->attributes->get('_system_path'));
+    if ($request->attributes->has('_system_path')) {
+      // _system_path never has leading or trailing slashes.
+      $path = '/' . $request->attributes->get('_system_path');
+    }
+    else {
+      // getPathInfo() always has leading slash, and might or might not have a
+      // trailing slash.
+      $path = rtrim($request->getPathInfo(), '/');
+    }
+    return $this->match($path);
   }
 
   /**

core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationController.php

@@ -12,12 +12,12 @@
 use Drupal\Core\Controller\ControllerBase;
 use Drupal\Core\Language\Language;
 use Drupal\Core\Language\LanguageManagerInterface;
+use Drupal\Core\ParamConverter\ParamNotConvertedException;
 use Drupal\Core\PathProcessor\InboundPathProcessorInterface;
 use Drupal\Core\Session\AccountInterface;
 use Symfony\Cmf\Component\Routing\RouteObjectInterface;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\HttpFoundation\Request;
-use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
 use Symfony\Component\Routing\Exception\ResourceNotFoundException;
 use Symfony\Component\Routing\Matcher\RequestMatcherInterface;
 
@@ -251,7 +251,7 @@ protected function getRequestForPath(Request $request, $path) {
       $route_request->attributes->add($this->router->matchRequest($route_request));
       return $route_request;
     }
-    catch (NotFoundHttpException $e) {
+    catch (ParamNotConvertedException $e) {
       return NULL;
     }
     catch (ResourceNotFoundException $e) {

core/modules/edit/lib/Drupal/edit/Access/EditEntityAccessCheck.php

@@ -44,7 +44,9 @@ public function access(Route $route, Request $request, AccountInterface $account
     // @todo Request argument validation and object loading should happen
     //   elsewhere in the request processing pipeline:
     //   http://drupal.org/node/1798214.
-    $this->validateAndUpcastRequestAttributes($request);
+    if (!$this->validateAndUpcastRequestAttributes($request)) {
+      return static::KILL;
+    }
 
     return $this->accessEditEntity($request->attributes->get('entity'), $account)  ? static::ALLOW : static::DENY;
   }
@@ -65,14 +67,16 @@ protected function validateAndUpcastRequestAttributes(Request $request) {
       $entity_id = $entity;
       $entity_type = $request->attributes->get('entity_type');
       if (!$entity_type || !$this->entityManager->getDefinition($entity_type)) {
-        throw new NotFoundHttpException();
+        return FALSE;
       }
       $entity = $this->entityManager->getStorageController($entity_type)->load($entity_id);
       if (!$entity) {
-        throw new NotFoundHttpException();
+        return FALSE;
       }
       $request->attributes->set('entity', $entity);
     }
+
+    return TRUE;
   }
 
 }

core/modules/edit/lib/Drupal/edit/Access/EditEntityFieldAccessCheck.php

@@ -12,7 +12,6 @@
 use Drupal\Core\Session\AccountInterface;
 use Symfony\Component\Routing\Route;
 use Symfony\Component\HttpFoundation\Request;
-use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
 use Drupal\Core\Entity\EntityInterface;
 
 /**
@@ -44,7 +43,9 @@ public function access(Route $route, Request $request, AccountInterface $account
     // @todo Request argument validation and object loading should happen
     //   elsewhere in the request processing pipeline:
     //   http://drupal.org/node/1798214.
-    $this->validateAndUpcastRequestAttributes($request);
+    if (!$this->validateAndUpcastRequestAttributes($request)) {
+      return static::KILL;
+    }
 
     return $this->accessEditEntityField($request->attributes->get('entity'), $request->attributes->get('field_name'))  ? static::ALLOW : static::DENY;
   }
@@ -65,11 +66,11 @@ protected function validateAndUpcastRequestAttributes(Request $request) {
       $entity_id = $entity;
       $entity_type = $request->attributes->get('entity_type');
       if (!$entity_type || !$this->entityManager->getDefinition($entity_type)) {
-        throw new NotFoundHttpException();
+        return FALSE;
       }
       $entity = $this->entityManager->getStorageController($entity_type)->load($entity_id);
       if (!$entity) {
-        throw new NotFoundHttpException();
+        return FALSE;
       }
       $request->attributes->set('entity', $entity);
     }
@@ -77,12 +78,14 @@ protected function validateAndUpcastRequestAttributes(Request $request) {
     // Validate the field name and language.
     $field_name = $request->attributes->get('field_name');
     if (!$field_name || !$entity->hasField($field_name)) {
-      throw new NotFoundHttpException();
+      return FALSE;
     }
     $langcode = $request->attributes->get('langcode');
     if (!$langcode || !$entity->hasTranslation($langcode)) {
-      throw new NotFoundHttpException();
+      return FALSE;
     }
+
+    return TRUE;
   }
 
 }

core/modules/edit/tests/Drupal/edit/Tests/Access/EditEntityAccessCheckTest.php

@@ -117,8 +117,6 @@ public function testAccess(EntityInterface $entity, $expected_result) {
 
   /**
    * Tests the access method with an undefined entity type.
-   *
-   * @expectedException \Symfony\Component\HttpKernel\Exception\NotFoundHttpException
    */
   public function testAccessWithUndefinedEntityType() {
     $route = new Route('/edit/form/test_entity/1/body/und/full', array(), array('_access_edit_entity' => 'TRUE'));
@@ -131,13 +129,11 @@ public function testAccessWithUndefinedEntityType() {
       ->will($this->returnValue(NULL));
 
     $account = $this->getMock('Drupal\Core\Session\AccountInterface');
-    $this->editAccessCheck->access($route, $request, $account);
+    $this->assertSame(AccessCheckInterface::KILL, $this->editAccessCheck->access($route, $request, $account));
   }
 
   /**
    * Tests the access method with a non existing entity.
-   *
-   * @expectedException \Symfony\Component\HttpKernel\Exception\NotFoundHttpException
    */
   public function testAccessWithNotExistingEntity() {
     $route = new Route('/edit/form/test_entity/1/body/und/full', array(), array('_access_edit_entity_field' => 'TRUE'));
@@ -156,7 +152,7 @@ public function testAccessWithNotExistingEntity() {
       ->will($this->returnValue(NULL));
 
     $account = $this->getMock('Drupal\Core\Session\AccountInterface');
-    $this->editAccessCheck->access($route, $request, $account);
+    $this->assertSame(AccessCheckInterface::KILL, $this->editAccessCheck->access($route, $request, $account));