Issue #2158571 by tstoeckler, dawehner, kgoel, tim.plunkett, catch,...

Issue #2158571 by tstoeckler, dawehner, kgoel, tim.plunkett, catch, effulgentsia: Routes added in RouteSubscribers cannot be altered.

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

@@ -36,7 +36,7 @@ public function __construct(ModuleHandlerInterface $module_handler) {
   /**
    * {@inheritdoc}
    */
-  protected function alterRoutes(RouteCollection $collection, $module) {
+  protected function alterRoutes(RouteCollection $collection) {
     foreach ($collection as $name => $route) {
       if ($route->hasRequirement('_module_dependencies')) {
         $modules = $route->getRequirement('_module_dependencies');

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

@@ -21,7 +21,7 @@ class SpecialAttributesRouteSubscriber extends RouteSubscriberBase {
   /**
    * {@inheritdoc}
    */
-  protected function alterRoutes(RouteCollection $collection, $module) {
+  protected function alterRoutes(RouteCollection $collection) {
     $special_variables = array(
       'system_path',
       '_maintenance',
@@ -57,7 +57,7 @@ protected function alterRoutes(RouteCollection $collection, $module) {
    */
   public function onAlterRoutes(RouteBuildEvent $event) {
     $collection = $event->getRouteCollection();
-    return $this->alterRoutes($collection, $event->getProvider());
+    return $this->alterRoutes($collection);
   }
 
 }

core/lib/Drupal/Core/Extension/ModuleHandler.php

@@ -929,9 +929,6 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
       watchdog('system', '%module module uninstalled.', array('%module' => $module), WATCHDOG_INFO);
 
       $schema_store->delete($module);
-
-      // Make sure any route data is also removed for this module.
-      \Drupal::service('router.dumper')->dump(array('provider' => $module));
     }
     drupal_get_installed_schema_version(NULL, TRUE);
 

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

@@ -87,80 +87,66 @@ public function addRoutes(RouteCollection $routes) {
    *   An array of options.
    */
   public function dump(array $options = array()) {
-    $options += array(
-      'provider' => '',
-    );
-
-    // If there are no new routes, just delete any previously existing of this
-    // provider.
-    if (empty($this->routes) || !count($this->routes)) {
-      $this->connection->delete($this->tableName)
-        ->condition('provider', $options['provider'])
-        ->execute();
-    }
     // Convert all of the routes into database records.
-    else {
-      // Accumulate the menu masks on top of any we found before.
-      $masks = array_flip($this->state->get('routing.menu_masks.' . $this->tableName, array()));
-      $insert = $this->connection->insert($this->tableName)->fields(array(
-        'name',
-        'provider',
-        'fit',
-        'path',
-        'pattern_outline',
-        'number_parts',
-        'route',
-      ));
-      $names = array();
-      foreach ($this->routes as $name => $route) {
-        $route->setOption('compiler_class', '\Drupal\Core\Routing\RouteCompiler');
-        $compiled = $route->compile();
-        // The fit value is a binary number which has 1 at every fixed path
-        // position and 0 where there is a wildcard. We keep track of all such
-        // patterns that exist so that we can minimize the the number of path
-        // patterns we need to check in the RouteProvider.
-        $masks[$compiled->getFit()] = 1;
-        $names[] = $name;
-        $values = array(
-          'name' => $name,
-          'provider' => $options['provider'],
-          'fit' => $compiled->getFit(),
-          'path' => $compiled->getPath(),
-          'pattern_outline' => $compiled->getPatternOutline(),
-          'number_parts' => $compiled->getNumParts(),
-          'route' => serialize($route),
-        );
-        $insert->values($values);
-      }
-
-      // Delete any old records of this provider first, then insert the new ones.
-      // That avoids stale data. The transaction makes it atomic to avoid
-      // unstable router states due to random failures.
-      $transaction = $this->connection->startTransaction();
-      try {
-        // Previously existing routes might have been moved to a new provider,
-        // so ensure that none of the names to insert exists. Also delete any
-        // old records of this provider (which may no longer exist).
-        $delete = $this->connection->delete($this->tableName);
-        $or = $delete->orConditionGroup()
-          ->condition('provider', $options['provider'])
-          ->condition('name', $names);
-        $delete->condition($or);
-        $delete->execute();
+    // Accumulate the menu masks on top of any we found before.
+    $masks = array_flip($this->state->get('routing.menu_masks.' . $this->tableName, array()));
+    // Delete any old records first, then insert the new ones. That avoids
+    // stale data. The transaction makes it atomic to avoid unstable router
+    // states due to random failures.
+    $transaction = $this->connection->startTransaction();
+    try {
+      // We don't use truncate, because it is not guaranteed to be transaction
+      // safe.
+      $this->connection->delete($this->tableName)->execute();
+
+      // Split the routes into chunks to avoid big INSERT queries.
+      $route_chunks = array_chunk($this->routes->all(), 50, TRUE);
+      foreach ($route_chunks as $routes) {
+        $insert = $this->connection->insert($this->tableName)->fields(array(
+          'name',
+          'fit',
+          'path',
+          'pattern_outline',
+          'number_parts',
+          'route',
+        ));
+        $names = array();
+        foreach ($routes as $name => $route) {
+          /** @var \Symfony\Component\Routing\Route $route */
+          $route->setOption('compiler_class', '\Drupal\Core\Routing\RouteCompiler');
+          $compiled = $route->compile();
+          // The fit value is a binary number which has 1 at every fixed path
+          // position and 0 where there is a wildcard. We keep track of all such
+          // patterns that exist so that we can minimize the the number of path
+          // patterns we need to check in the RouteProvider.
+          $masks[$compiled->getFit()] = 1;
+          $names[] = $name;
+          $values = array(
+            'name' => $name,
+            'fit' => $compiled->getFit(),
+            'path' => $compiled->getPath(),
+            'pattern_outline' => $compiled->getPatternOutline(),
+            'number_parts' => $compiled->getNumParts(),
+            'route' => serialize($route),
+          );
+          $insert->values($values);
+        }
 
         // Insert all new routes.
         $insert->execute();
-      } catch (\Exception $e) {
-        $transaction->rollback();
-        watchdog_exception('Routing', $e);
-        throw $e;
       }
-      // Sort the masks so they are in order of descending fit.
-      $masks = array_keys($masks);
-      rsort($masks);
-      $this->state->set('routing.menu_masks.' . $this->tableName, $masks);
+
+
+    } catch (\Exception $e) {
+      $transaction->rollback();
+      watchdog_exception('Routing', $e);
+      throw $e;
     }
-    // The dumper is reused for multiple providers, so reset the queued routes.
+    // Sort the masks so they are in order of descending fit.
+    $masks = array_keys($masks);
+    rsort($masks);
+    $this->state->set('routing.menu_masks.' . $this->tableName, $masks);
+
     $this->routes = NULL;
   }
 

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

@@ -22,19 +22,14 @@ class RouteBuildEvent extends Event {
    */
   protected $routeCollection;
 
-  /**
-   * The provider of this route collection.
-   *
-   * @var string
-   */
-  protected $provider;
-
   /**
    * Constructs a RouteBuildEvent object.
+   *
+   * @param \Symfony\Component\Routing\RouteCollection $route_collection
+   *   The route collection.
    */
-  public function __construct(RouteCollection $route_collection, $provider) {
+  public function __construct(RouteCollection $route_collection) {
     $this->routeCollection = $route_collection;
-    $this->provider = $provider;
   }
 
   /**
@@ -44,11 +39,4 @@ public function getRouteCollection() {
     return $this->routeCollection;
   }
 
-  /**
-   * Gets the provider for this route collection.
-   */
-  public function getProvider() {
-    return $this->provider;
-  }
-
 }

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

@@ -20,16 +20,13 @@
 
 /**
  * Managing class for rebuilding the router table.
- *
- * Because this class makes use of the modules system, it cannot currently
- * be unit tested.
  */
 class RouteBuilder implements RouteBuilderInterface {
 
   /**
    * The dumper to which we should send collected routes.
    *
-   * @var \Symfony\Component\Routing\Matcher\Dumper\MatcherDumperInterface
+   * @var \Drupal\Core\Routing\MatcherDumperInterface
    */
   protected $dumper;
 
@@ -68,6 +65,13 @@ class RouteBuilder implements RouteBuilderInterface {
    */
   protected $controllerResolver;
 
+  /**
+   * The route collection during the rebuild.
+   *
+   * @var \Symfony\Component\Routing\RouteCollection
+   */
+  protected $routeCollection;
+
   /**
    * Constructs the RouteBuilder using the passed MatcherDumperInterface.
    *
@@ -81,6 +85,8 @@ class RouteBuilder implements RouteBuilderInterface {
    *   The module handler.
    * @param \Drupal\Core\Controller\ControllerResolverInterface $controller_resolver
    *   The controller resolver.
+   * @param \Drupal\Core\KeyValueStore\StateInterface $state
+   *   The state.
    */
   public function __construct(MatcherDumperInterface $dumper, LockBackendInterface $lock, EventDispatcherInterface $dispatcher, ModuleHandlerInterface $module_handler, ControllerResolverInterface $controller_resolver, StateInterface $state = NULL) {
     $this->dumper = $dumper;
@@ -103,9 +109,9 @@ public function rebuild() {
       return FALSE;
     }
 
-    foreach ($this->getRouteDefinitions() as $provider => $routes) {
-      $collection = new RouteCollection();
-
+    $collection = new RouteCollection();
+    $this->routeCollection = $collection;
+    foreach ($this->getRouteDefinitions() as $routes) {
       // The top-level 'routes_callback' is a list of methods in controller
       // syntax, see \Drupal\Core\Controller\ControllerResolver. These methods
       // should return a set of \Symfony\Component\Routing\Route objects, either
@@ -142,24 +148,36 @@ public function rebuild() {
         $collection->add($name, $route);
       }
 
-      $this->dispatcher->dispatch(RoutingEvents::ALTER, new RouteBuildEvent($collection, $provider));
-      $this->dumper->addRoutes($collection);
-      $this->dumper->dump(array('provider' => $provider));
     }
 
-    // Now allow modules to register additional, dynamic routes.
-    // @todo Either remove this alter or the per-provider alter.
-    $collection = new RouteCollection();
-    $this->dispatcher->dispatch(RoutingEvents::ALTER, new RouteBuildEvent($collection, 'dynamic_routes'));
+    // DYNAMIC is supposed to be used to add new routes based upon all the
+    // static defined ones.
+    $this->dispatcher->dispatch(RoutingEvents::DYNAMIC, new RouteBuildEvent($collection));
+
+    // ALTER is the final step to alter all the existing routes. We cannot stop
+    // people from adding new routes here, but we define two separate steps to
+    // make it clear.
+    $this->dispatcher->dispatch(RoutingEvents::ALTER, new RouteBuildEvent($collection));
+
     $this->dumper->addRoutes($collection);
-    $this->dumper->dump(array('provider' => 'dynamic_routes'));
+    $this->dumper->dump();
 
     $this->state->delete(static::REBUILD_NEEDED);
     $this->lock->release('router_rebuild');
     $this->dispatcher->dispatch(RoutingEvents::FINISHED, new Event());
+
+    $this->routeCollection = NULL;
+
     return TRUE;
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  public function getCollectionDuringRebuild() {
+    return $this->routeCollection ?: FALSE;
+  }
+
   /**
    * {@inheritdoc}
    */

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

@@ -19,6 +19,18 @@ interface RouteBuilderInterface {
    */
   public function rebuild();
 
+  /**
+   * Returns the route collection during the rebuild.
+   *
+   * Don't use this function unless you really have to! Better pass along the
+   * collection for yourself during the rebuild.
+   *
+   * Every use of this function is a design flaw of your code.
+   *
+   * @return \Symfony\Component\Routing\RouteCollection|FALSE
+   */
+  public function getCollectionDuringRebuild();
+
   /**
    * Rebuilds the route info and dumps to dumper if necessary.
    *

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

@@ -45,4 +45,8 @@ public function setRebuildNeeded() {
     $this->rebuildNeeded = TRUE;
   }
 
+  public function getCollectionDuringRebuild() {
+    return FALSE;
+  }
+
 }

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

@@ -21,11 +21,8 @@ abstract class RouteSubscriberBase implements EventSubscriberInterface {
    *
    * @param \Symfony\Component\Routing\RouteCollection $collection
    *   The route collection for adding routes.
-   * @param string $provider
-   *   The provider these routes belong to. For dynamically added routes, the
-   *   provider name will be 'dynamic_routes'.
    */
-  abstract protected function alterRoutes(RouteCollection $collection, $provider);
+  abstract protected function alterRoutes(RouteCollection $collection);
 
   /**
    * {@inheritdoc}
@@ -43,7 +40,7 @@ public static function getSubscribedEvents() {
    */
   public function onAlterRoutes(RouteBuildEvent $event) {
     $collection = $event->getRouteCollection();
-    $this->alterRoutes($collection, $event->getProvider());
+    $this->alterRoutes($collection);
   }
 
 }

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

@@ -12,14 +12,17 @@
  */
 final class RoutingEvents {
 
+  /**
+   * THE DYNAMIC event is fired on a route collection to allow new routes.
+   *
+   * This event is used to add new routes based upon existing routes.
+   */
+  const DYNAMIC = 'routing.route_dynamic';
+
   /**
    * The ALTER event is fired on a route collection to allow changes to routes.
    *
    * This event is used to process new routes before they get saved.
-   *
-   * @see \Drupal\Core\Routing\RouteBuildEvent
-   *
-   * @var string
    */
   const ALTER = 'routing.route_alter';
 

core/modules/config_translation/config_translation.module

@@ -121,6 +121,9 @@ function config_translation_config_translation_info(&$info) {
         $base_route = $route_provider->getRouteByName('field_ui.instance_edit_' . $entity_type_id);
       }
       catch (RouteNotFoundException $e) {
+        if ($collection = \Drupal::service('router.builder')->getCollectionDuringRebuild()) {
+          $base_route = $collection->get('field_ui.instance_edit_' . $entity_type_id);
+        }
         // Ignore non-existent routes.
       }
 

core/modules/config_translation/lib/Drupal/config_translation/ConfigMapperInterface.php

@@ -9,6 +9,7 @@
 
 use Drupal\Core\Language\Language;
 use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\Routing\RouteCollection;
 
 /**
  * Defines an interface for configuration mapper.
@@ -23,6 +24,14 @@ interface ConfigMapperInterface {
    */
   public function getTitle();
 
+  /**
+   * Sets the route collection.
+   *
+   * @param \Symfony\Component\Routing\RouteCollection $collection
+   *   The route collection.
+   */
+  public function setRouteCollection(RouteCollection $collection);
+
   /**
    * Returns the name of the base route the mapper is attached to.
    *

core/modules/config_translation/lib/Drupal/config_translation/ConfigMapperManager.php

@@ -21,6 +21,7 @@
 use Drupal\Core\Plugin\Discovery\ContainerDerivativeDiscoveryDecorator;
 use Drupal\Core\Plugin\Factory\ContainerFactory;
 use Drupal\Core\TypedData\TypedDataInterface;
+use Symfony\Component\Routing\RouteCollection;
 
 /**
  * Manages plugins for configuration translation mappers.
@@ -102,10 +103,13 @@ public function __construct(CacheBackendInterface $cache_backend, LanguageManage
   /**
    * {@inheritdoc}
    */
-  public function getMappers() {
+  public function getMappers(RouteCollection $collection = NULL) {
     $mappers = array();
     foreach($this->getDefinitions() as $id => $definition) {
       $mappers[$id] = $this->createInstance($id);
+      if ($collection) {
+        $mappers[$id]->setRouteCollection($collection);
+      }
     }
 
     return $mappers;

core/modules/config_translation/lib/Drupal/config_translation/ConfigMapperManagerInterface.php

@@ -8,6 +8,7 @@
 namespace Drupal\config_translation;
 
 use Drupal\Component\Plugin\PluginManagerInterface;
+use Symfony\Component\Routing\RouteCollection;
 
 /**
  * Provides a common interface for config mapper managers.
@@ -17,10 +18,13 @@ interface ConfigMapperManagerInterface extends PluginManagerInterface {
   /**
    * Returns an array of all mappers.
    *
+   * @param \Symfony\Component\Routing\RouteCollection $collection
+   *   The route collection used to initialize the mappers.
+   *
    * @return \Drupal\config_translation\ConfigMapperInterface[]
    *   An array of all mappers.
    */
-  public function getMappers();
+  public function getMappers(RouteCollection $collection = NULL);
 
   /**
    * Returns TRUE if the configuration data has translatable items.

core/modules/config_translation/lib/Drupal/config_translation/ConfigNamesMapper.php

@@ -17,6 +17,7 @@
 use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\Routing\Route;
+use Symfony\Component\Routing\RouteCollection;
 
 /**
  * Configuration mapper base implementation.
@@ -51,6 +52,13 @@ class ConfigNamesMapper extends PluginBase implements ConfigMapperInterface, Con
    */
   protected $baseRoute;
 
+  /**
+   * The available routes.
+   *
+   * @var \Symfony\Component\Routing\RouteCollection
+   */
+  protected $routeCollection;
+
   /**
    * The language code of the language this mapper, if any.
    *
@@ -91,14 +99,13 @@ class ConfigNamesMapper extends PluginBase implements ConfigMapperInterface, Con
   public function __construct($plugin_id, $plugin_definition, ConfigFactoryInterface $config_factory, LocaleConfigManager $locale_config_manager, ConfigMapperManagerInterface $config_mapper_manager, RouteProviderInterface $route_provider, TranslationInterface $translation_manager) {
     $this->pluginId = $plugin_id;
     $this->pluginDefinition = $plugin_definition;
+    $this->routeProvider = $route_provider;
 
     $this->configFactory = $config_factory;
     $this->localeConfigManager = $locale_config_manager;
     $this->configMapperManager = $config_mapper_manager;
 
     $this->setTranslationManager($translation_manager);
-
-    $this->baseRoute = $route_provider->getRouteByName($this->getBaseRouteName());
   }
 
   /**
@@ -118,6 +125,13 @@ public static function create(ContainerInterface $container, array $configuratio
     );
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  public function setRouteCollection(RouteCollection $collection) {
+    $this->routeCollection = $collection;
+  }
+
   /**
    * {@inheritdoc}
    */
@@ -145,7 +159,12 @@ public function getBaseRouteParameters() {
    * {@inheritdoc}
    */
   public function getBaseRoute() {
-    return $this->baseRoute;
+    if ($this->routeCollection) {
+      return $this->routeCollection->get($this->getBaseRouteName());
+    }
+    else {
+      return $this->routeProvider->getRouteByName($this->getBaseRouteName());
+    }
   }
 
   /**

core/modules/config_translation/lib/Drupal/config_translation/Routing/RouteSubscriber.php

@@ -9,6 +9,7 @@
 
 use Drupal\Core\Routing\RouteSubscriberBase;
 use Drupal\config_translation\ConfigMapperManagerInterface;
+use Drupal\Core\Routing\RoutingEvents;
 use Symfony\Component\Routing\RouteCollection;
 
 /**
@@ -36,15 +37,9 @@ public function __construct(ConfigMapperManagerInterface $mapper_manager) {
   /**
    * {@inheritdoc}
    */
-  protected function alterRoutes(RouteCollection $collection, $provider) {
-    // @todo \Drupal\config_translation\ConfigNamesMapper uses the route
-    //   provider directly, which is unsafe during rebuild. This currently only
-    //   works by coincidence; fix in https://drupal.org/node/2158571.
-    if ($provider != 'dynamic_routes') {
-      return;
-    }
+  protected function alterRoutes(RouteCollection $collection) {
+    $mappers = $this->mapperManager->getMappers($collection);
 
-    $mappers = $this->mapperManager->getMappers();
     foreach ($mappers as $mapper) {
       $collection->add($mapper->getOverviewRouteName(), $mapper->getOverviewRoute());
       $collection->add($mapper->getAddRouteName(), $mapper->getAddRoute());
@@ -53,4 +48,13 @@ protected function alterRoutes(RouteCollection $collection, $provider) {
     }
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  public static function getSubscribedEvents() {
+    // Come after field_ui.
+    $events[RoutingEvents::ALTER] = array('onAlterRoutes', -110);
+    return $events;
+  }
+
 }

core/modules/config_translation/tests/Drupal/config_translation/Tests/ConfigEntityMapperTest.php

@@ -66,7 +66,7 @@ public function setUp() {
     $this->routeProvider = $this->getMock('Drupal\Core\Routing\RouteProviderInterface');
 
     $this->routeProvider
-      ->expects($this->once())
+      ->expects($this->any())
       ->method('getRouteByName')
       ->with('language.edit')
       ->will($this->returnValue(new Route('/admin/config/regional/language/edit/{language_entity}')));

core/modules/config_translation/tests/Drupal/config_translation/Tests/ConfigNamesMapperTest.php

@@ -97,7 +97,7 @@ public function setUp() {
     $this->baseRoute = new Route('/admin/config/system/site-information');
 
     $this->routeProvider
-      ->expects($this->once())
+      ->expects($this->any())
       ->method('getRouteByName')
       ->with('system.site_information_settings')
       ->will($this->returnValue($this->baseRoute));

core/modules/content_translation/lib/Drupal/content_translation/Routing/ContentTranslationRouteSubscriber.php

@@ -38,7 +38,7 @@ public fun