Move {router} out of system.install and create the table lazy

I don't know what the point is without an issue summary.

1. +++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
   @@ -97,7 +101,7 @@ public function dump(array $options = array()) {
   -      $this->connection->delete($this->tableName)->execute();
   +      $this->safeDatabaseExecute($this->connection->delete($this->tableName))->execute();
   

   This feels very weird. I don't have a more specific word for it off hand. :-) Rather, this operation should be encapsulated into ensureTableIsReady() or something, which will create it if it doesn't exist, truncate it if it does, etc.

2. +++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
   @@ -161,4 +165,110 @@ public function getRoutes() {
   +    catch (\Exception $e) {
   +      // If there was an exception, try to create the table.
   +      if ($this->ensureTableExists()) {
   +        return $query->execute();
   +      }
   +      // Some other failure that we can not recover from.
   +      throw $e;
   +    }
   

   There's a very specific exception thrown for missing table. Just catch that, then you don't need the re-throw.

Rerolled. Review is not addressed but this has a higher chance of passing as it now passes the actual query objects to safeDatabaseExecute and doesn't want to execute the result either.

#2371709: [PP-x] Move the on-demand-table creation into the database API

1. +++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
   @@ -162,4 +167,111 @@ public function getRoutes() {
   +  protected function safeDatabaseExecute(Query $query) {
   

   The method name here is seriously weird. "safe" execute is right up there with "safe mode" or "mysql_no_really_for_reals_escape_string" as a name that is a code smell as it lacks context. :-)

   As noted in an earlier comment, the model here is weird. This is off the critical path, so ensureTable() would typically add one very simple query to a rare operation that is already several hundred queries; ie, it wouldn't be noticeable.

   If we want to stick to the catch-exception approach, then we need a better name here that is more self-descriptive. I am also tempted to say that the table definition should be passed in as well, as we were discussing in the DB issue, but in context that may not be necessary as a one-off.

2. +++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
   @@ -162,4 +167,111 @@ public function getRoutes() {
   +  protected static function schemaDefinition() {
   

   There's no value to this being static, so don't make it static.

1. +++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
   @@ -97,7 +101,13 @@ public function dump(array $options = array()) {
   +      catch (\Exception $e) {
   +        $this->ensureTableExists();
   +      }
   

   Should this catch be more specific than a Pokemon exception? Just database exceptions, for instance?

   (The same applies elsewhere in the patch.)

2. +++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
   @@ -162,4 +172,85 @@ public function getRoutes() {
   +        $this->connection->schema()->createTable($this->tableName, static::schemaDefinition());
   

   Since schemaDefinition() is no longer static, we can't call it this way. It should error. Just $this->schemaDefinition().

Other than those fairly small nits, this looks good.

OK, split the difference. Thanks, dawehner!

Since schemaDefinition() is no longer static, we can't call it this way. It should error.

You wish... :-)

There's no 'table doesn't exist exception' that can be reliably used across sqlite, mysql, postgres - we tried that in the original issue that added this pattern.

Patch looks great. Committed/pushed to 8.1.x, thanks!

This change broke Rules tests in interesting ways and we need to have Drupal version switches now in some tests, because our tests run on both branches.

A change notice would have been nice when we make test breaking changes like this.