Issue #3126658 by alexpott, daffie, Beakerboy: Support enclosing reserved words with brackets

core/lib/Drupal/Core/Command/DbDumpCommand.php

@@ -265,7 +265,7 @@ protected function getTableIndexes(Connection $connection, $table, &$definition)
    */
   protected function getTableCollation(Connection $connection, $table, &$definition) {
     // Remove identifier quotes from the table name. See
-    // \Drupal\Core\Database\Driver\mysql\Connection::identifierQuote().
+    // \Drupal\Core\Database\Driver\mysql\Connection::$identifierQuotes.
     $table = trim($connection->prefixTables('{' . $table . '}'), '"');
     $query = $connection->query("SHOW TABLE STATUS WHERE NAME = :table_name", [':table_name' => $table]);
     $data = $query->fetchAssoc();

core/lib/Drupal/Core/Database/Connection.php

@@ -2,6 +2,7 @@
 
 namespace Drupal\Core\Database;
 
+use Drupal\Component\Assertion\Inspector;
 use Drupal\Core\Database\Query\Condition;
 
 /**
@@ -179,6 +180,17 @@ abstract class Connection {
    */
   protected $rootTransactionEndCallbacks = [];
 
+  /**
+   * The identifier quote characters for the database type.
+   *
+   * An array containing the start and end identifier quote characters for the
+   * database type. The ANSI SQL standard identifier quote character is a double
+   * quotation mark.
+   *
+   * @var string[]
+   */
+  protected $identifierQuotes;
+
   /**
    * Constructs a Connection object.
    *
@@ -191,6 +203,11 @@ abstract class Connection {
    *   - Other driver-specific options.
    */
   public function __construct(\PDO $connection, array $connection_options) {
+    if ($this->identifierQuotes === NULL) {
+      @trigger_error('In drupal:10.0.0 not setting the $identifierQuotes property in the concrete Connection class will result in an RuntimeException. See https://www.drupal.org/node/2986894', E_USER_DEPRECATED);
+      $this->identifierQuotes = ['', ''];
+    }
+    assert(count($this->identifierQuotes) === 2 && Inspector::assertAllStrings($this->identifierQuotes), '\Drupal\Core\Database\Connection::$identifierQuotes must contain 2 string values');
     // The 'transactions' option is deprecated.
     if (isset($connection_options['transactions'])) {
       @trigger_error('Passing a \'transactions\' connection option to ' . __METHOD__ . ' is deprecated in drupal:9.1.0 and is removed in drupal:10.0.0. All database drivers must support transactions. See https://www.drupal.org/node/2278745', E_USER_DEPRECATED);
@@ -329,7 +346,7 @@ protected function setPrefix($prefix) {
       $this->prefixes = ['default' => $prefix];
     }
 
-    $identifier_quote = $this->identifierQuote();
+    [$start_quote, $end_quote] = $this->identifierQuotes;
     // Set up variables for use in prefixTables(). Replace table-specific
     // prefixes first.
     $this->prefixSearch = [];
@@ -339,8 +356,8 @@ protected function setPrefix($prefix) {
         $this->prefixSearch[] = '{' . $key . '}';
         // $val can point to another database like 'database.users'. In this
         // instance we need to quote the identifiers correctly.
-        $val = str_replace('.', $identifier_quote . '.' . $identifier_quote, $val);
-        $this->prefixReplace[] = $identifier_quote . $val . $key . $identifier_quote;
+        $val = str_replace('.', $end_quote . '.' . $start_quote, $val);
+        $this->prefixReplace[] = $start_quote . $val . $key . $end_quote;
       }
     }
     // Then replace remaining tables with the default prefix.
@@ -348,9 +365,9 @@ protected function setPrefix($prefix) {
     // $this->prefixes['default'] can point to another database like
     // 'other_db.'. In this instance we need to quote the identifiers correctly.
     // For example, "other_db"."PREFIX_table_name".
-    $this->prefixReplace[] = $identifier_quote . str_replace('.', $identifier_quote . '.' . $identifier_quote, $this->prefixes['default']);
+    $this->prefixReplace[] = $start_quote . str_replace('.', $end_quote . '.' . $start_quote, $this->prefixes['default']);
     $this->prefixSearch[] = '}';
-    $this->prefixReplace[] = $identifier_quote;
+    $this->prefixReplace[] = $end_quote;
 
     // Set up a map of prefixed => un-prefixed tables.
     foreach ($this->prefixes as $table_name => $prefix) {
@@ -360,20 +377,6 @@ protected function setPrefix($prefix) {
     }
   }
 
-  /**
-   * Returns the identifier quote character for the database type.
-   *
-   * The ANSI SQL standard identifier quote character is a double quotation
-   * mark.
-   *
-   * @return string
-   *   The identifier quote character for the database type.
-   */
-  protected function identifierQuote() {
-    @trigger_error('In drupal:10.0.0 this method will be abstract and contrib and custom drivers will have to implement it. See https://www.drupal.org/node/2986894', E_USER_DEPRECATED);
-    return '';
-  }
-
   /**
    * Appends a database prefix to all tables in a query.
    *
@@ -413,7 +416,7 @@ public function prefixTables($sql) {
    *   This method should only be called by database API code.
    */
   public function quoteIdentifiers($sql) {
-    return str_replace(['[', ']'], $this->identifierQuote(), $sql);
+    return str_replace(['[', ']'], $this->identifierQuotes, $sql);
   }
 
   /**
@@ -579,7 +582,7 @@ public function makeSequenceName($table, $field) {
     $sequence_name = $this->prefixTables('{' . $table . '}_' . $field . '_seq');
     // Remove identifier quotes as we are constructing a new name from a
     // prefixed and quoted table name.
-    return str_replace($this->identifierQuote(), '', $sequence_name);
+    return str_replace($this->identifierQuotes, '', $sequence_name);
   }
 
   /**
@@ -1063,7 +1066,8 @@ public function condition($conjunction) {
    */
   public function escapeDatabase($database) {
     $database = preg_replace('/[^A-Za-z0-9_]+/', '', $database);
-    return $this->identifierQuote() . $database . $this->identifierQuote();
+    [$start_quote, $end_quote] = $this->identifierQuotes;
+    return $start_quote . $database . $end_quote;
   }
 
   /**
@@ -1106,10 +1110,10 @@ public function escapeTable($table) {
   public function escapeField($field) {
     if (!isset($this->escapedFields[$field])) {
       $escaped = preg_replace('/[^A-Za-z0-9_.]+/', '', $field);
-      $identifier_quote = $this->identifierQuote();
+      [$start_quote, $end_quote] = $this->identifierQuotes;
       // Sometimes fields have the format table_alias.field. In such cases
       // both identifiers should be quoted, for example, "table_alias"."field".
-      $this->escapedFields[$field] = $identifier_quote . str_replace('.', $identifier_quote . '.' . $identifier_quote, $escaped) . $identifier_quote;
+      $this->escapedFields[$field] = $start_quote . str_replace('.', $end_quote . '.' . $start_quote, $escaped) . $end_quote;
     }
     return $this->escapedFields[$field];
   }
@@ -1130,7 +1134,8 @@ public function escapeField($field) {
    */
   public function escapeAlias($field) {
     if (!isset($this->escapedAliases[$field])) {
-      $this->escapedAliases[$field] = $this->identifierQuote() . preg_replace('/[^A-Za-z0-9_]+/', '', $field) . $this->identifierQuote();
+      [$start_quote, $end_quote] = $this->identifierQuotes;
+      $this->escapedAliases[$field] = $start_quote . preg_replace('/[^A-Za-z0-9_]+/', '', $field) . $end_quote;
     }
     return $this->escapedAliases[$field];
   }

core/lib/Drupal/Core/Database/Driver/mysql/Connection.php

@@ -63,6 +63,11 @@ class Connection extends DatabaseConnection {
    */
   const MIN_MAX_ALLOWED_PACKET = 1024;
 
+  /**
+   * {@inheritdoc}
+   */
+  protected $identifierQuotes = ['"', '"'];
+
   /**
    * {@inheritdoc}
    */
@@ -213,15 +218,6 @@ public function queryTemporary($query, array $args = [], array $options = []) {
     return $tablename;
   }
 
-  /**
-   * {@inheritdoc}
-   */
-  protected function identifierQuote() {
-    // The database is using the ANSI option on set up so use ANSI quotes and
-    // not MySQL's custom backtick quote.
-    return '"';
-  }
-
   public function driver() {
     return 'mysql';
   }

core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php

@@ -54,6 +54,11 @@ class Connection extends DatabaseConnection {
    */
   protected $transactionalDDLSupport = TRUE;
 
+  /**
+   * {@inheritdoc}
+   */
+  protected $identifierQuotes = ['"', '"'];
+
   /**
    * Constructs a connection object.
    */
@@ -197,13 +202,6 @@ public function queryTemporary($query, array $args = [], array $options = []) {
     return $tablename;
   }
 
-  /**
-   * {@inheritdoc}
-   */
-  protected function identifierQuote() {
-    return '"';
-  }
-
   public function driver() {
     return 'pgsql';
   }

core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php

@@ -60,6 +60,11 @@ class Connection extends DatabaseConnection {
    */
   protected $transactionalDDLSupport = TRUE;
 
+  /**
+   * {@inheritdoc}
+   */
+  protected $identifierQuotes = ['"', '"'];
+
   /**
    * Constructs a \Drupal\Core\Database\Driver\sqlite\Connection object.
    */
@@ -375,13 +380,6 @@ public function databaseType() {
     return 'sqlite';
   }
 
-  /**
-   * {@inheritdoc}
-   */
-  protected function identifierQuote() {
-    return '"';
-  }
-
   /**
    * Overrides \Drupal\Core\Database\Connection::createDatabase().
    *

core/tests/Drupal/Tests/Core/Database/ConditionTest.php

@@ -5,6 +5,7 @@
 use Drupal\Core\Database\Connection;
 use Drupal\Core\Database\Query\Condition;
 use Drupal\Core\Database\Query\PlaceholderInterface;
+use Drupal\Tests\Core\Database\Stub\StubConnection;
 use Drupal\Tests\Core\Database\Stub\StubPDO;
 use Drupal\Tests\UnitTestCase;
 use Prophecy\Argument;
@@ -195,14 +196,7 @@ class_alias('MockCondition', $mocked_namespace);
 
     $mockPdo = $this->createMock(StubPDO::class);
 
-    $connection = $this->getMockBuilder(Connection::class)
-      ->setConstructorArgs([$mockPdo, $options])
-      ->setMethods(['identifierQuote'])
-      ->getMockForAbstractClass();
-    // @todo In drupal:10.0.0 this function will be abstract and the mock
-    // builder will automatically create it. This can be
-    // can be removed at that time.
-    $connection->method('identifierQuote')->willReturn(NULL);
+    $connection = new StubConnection($mockPdo, $options);
     $condition = $connection->condition('AND');
     $this->assertSame('MockCondition', get_class($condition));
   }

core/tests/Drupal/Tests/Core/Database/ConnectionTest.php

@@ -78,7 +78,7 @@ public function providerTestPrefixTables() {
         'SELECT * FROM test_table',
         'test_',
         'SELECT * FROM {table}',
-        '',
+        ['', ''],
       ],
       [
         'SELECT * FROM "first_table" JOIN "second"."thingie"',
@@ -88,6 +88,16 @@ public function providerTestPrefixTables() {
         ],
         'SELECT * FROM {table} JOIN {thingie}',
       ],
+      [
+        'SELECT * FROM [first_table] JOIN [second].[thingie]',
+        [
+          'table' => 'first_',
+          'thingie' => 'second.',
+        ],
+        'SELECT * FROM {table} JOIN {thingie}',
+        ['[', ']'],
+      ],
+
     ];
   }
 
@@ -96,7 +106,7 @@ public function providerTestPrefixTables() {
    *
    * @dataProvider providerTestPrefixTables
    */
-  public function testPrefixTables($expected, $prefix_info, $query, $quote_identifier = '"') {
+  public function testPrefixTables($expected, $prefix_info, $query, array $quote_identifier = ['"', '"']) {
     $mock_pdo = $this->createMock('Drupal\Tests\Core\Database\Stub\StubPDO');
     $connection = new StubConnection($mock_pdo, ['prefix' => $prefix_info], $quote_identifier);
     $this->assertEquals($expected, $connection->prefixTables($query));
@@ -275,7 +285,8 @@ public function providerEscapeTables() {
     return [
       ['nocase', 'nocase'],
       ['camelCase', 'camelCase'],
-      ['backtick', '`backtick`', '`'],
+      ['backtick', '`backtick`', ['`', '`']],
+      ['brackets', '[brackets]', ['[', ']']],
       ['camelCase', '"camelCase"'],
       ['camelCase', 'camel/Case'],
       // Sometimes, table names are following the pattern database.schema.table.
@@ -290,7 +301,7 @@ public function providerEscapeTables() {
    * @covers ::escapeTable
    * @dataProvider providerEscapeTables
    */
-  public function testEscapeTable($expected, $name, $identifier_quote = '"') {
+  public function testEscapeTable($expected, $name, array $identifier_quote = ['"', '"']) {
     $mock_pdo = $this->createMock(StubPDO::class);
     $connection = new StubConnection($mock_pdo, [], $identifier_quote);
 
@@ -307,9 +318,10 @@ public function testEscapeTable($expected, $name, $identifier_quote = '"') {
    */
   public function providerEscapeAlias() {
     return [
-      ['!nocase!', 'nocase', '!'],
-      ['`backtick`', 'backtick', '`'],
-      ['nocase', 'nocase', ''],
+      ['!nocase!', 'nocase', ['!', '!']],
+      ['`backtick`', 'backtick', ['`', '`']],
+      ['nocase', 'nocase', ['', '']],
+      ['[brackets]', 'brackets', ['[', ']']],
       ['"camelCase"', '"camelCase"'],
       ['"camelCase"', 'camelCase'],
       ['"camelCase"', 'camel.Case'],
@@ -320,7 +332,7 @@ public function providerEscapeAlias() {
    * @covers ::escapeAlias
    * @dataProvider providerEscapeAlias
    */
-  public function testEscapeAlias($expected, $name, $identifier_quote = '"') {
+  public function testEscapeAlias($expected, $name, array $identifier_quote = ['"', '"']) {
     $mock_pdo = $this->createMock(StubPDO::class);
     $connection = new StubConnection($mock_pdo, [], $identifier_quote);
 
@@ -337,15 +349,16 @@ public function testEscapeAlias($expected, $name, $identifier_quote = '"') {
    */
   public function providerEscapeFields() {
     return [
-      ['/title/', 'title', '/'],
-      ['`backtick`', 'backtick', '`'],
-      ['test.title', 'test.title', ''],
+      ['/title/', 'title', ['/', '/']],
+      ['`backtick`', 'backtick', ['`', '`']],
+      ['test.title', 'test.title', ['', '']],
       ['"isDefaultRevision"', 'isDefaultRevision'],
       ['"isDefaultRevision"', '"isDefaultRevision"'],
       ['"entity_test"."isDefaultRevision"', 'entity_test.isDefaultRevision'],
       ['"entity_test"."isDefaultRevision"', '"entity_test"."isDefaultRevision"'],
       ['"entityTest"."isDefaultRevision"', '"entityTest"."isDefaultRevision"'],
       ['"entityTest"."isDefaultRevision"', 'entityTest.isDefaultRevision'],
+      ['[entityTest].[isDefaultRevision]', 'entityTest.isDefaultRevision', ['[', ']']],
     ];
   }
 
@@ -353,7 +366,7 @@ public function providerEscapeFields() {
    * @covers ::escapeField
    * @dataProvider providerEscapeFields
    */
-  public function testEscapeField($expected, $name, $identifier_quote = '"') {
+  public function testEscapeField($expected, $name, array $identifier_quote = ['"', '"']) {
     $mock_pdo = $this->createMock(StubPDO::class);
     $connection = new StubConnection($mock_pdo, [], $identifier_quote);
 
@@ -370,10 +383,11 @@ public function testEscapeField($expected, $name, $identifier_quote = '"') {
    */
   public function providerEscapeDatabase() {
     return [
-      ['/name/', 'name', '/'],
-      ['`backtick`', 'backtick', '`'],
-      ['testname', 'test.name', ''],
+      ['/name/', 'name', ['/', '/']],
+      ['`backtick`', 'backtick', ['`', '`']],
+      ['testname', 'test.name', ['', '']],
       ['"name"', 'name'],
+      ['[name]', 'name', ['[', ']']],
     ];
   }
 
@@ -381,11 +395,41 @@ public function providerEscapeDatabase() {
    * @covers ::escapeDatabase
    * @dataProvider providerEscapeDatabase
    */
-  public function testEscapeDatabase($expected, $name, $identifier_quote = '"') {
+  public function testEscapeDatabase($expected, $name, array $identifier_quote = ['"', '"']) {
     $mock_pdo = $this->createMock(StubPDO::class);
     $connection = new StubConnection($mock_pdo, [], $identifier_quote);
 
     $this->assertEquals($expected, $connection->escapeDatabase($name));
   }
 
+  /**
+   * @covers ::__construct
+   * @expectedDeprecation In drupal:10.0.0 not setting the $identifierQuotes property in the concrete Connection class will result in an RuntimeException. See https://www.drupal.org/node/2986894
+   * @group legacy
+   */
+  public function testIdentifierQuotesDeprecation() {
+    $mock_pdo = $this->createMock(StubPDO::class);
+    new StubConnection($mock_pdo, [], NULL);
+  }
+
+  /**
+   * @covers ::__construct
+   */
+  public function testIdentifierQuotesAssertCount() {
+    $this->expectException(\AssertionError::class);
+    $this->expectExceptionMessage('\Drupal\Core\Database\Connection::$identifierQuotes must contain 2 string values');
+    $mock_pdo = $this->createMock(StubPDO::class);
+    new StubConnection($mock_pdo, [], ['"']);
+  }
+
+  /**
+   * @covers ::__construct
+   */
+  public function testIdentifierQuotesAssertString() {
+    $this->expectException(\AssertionError::class);
+    $this->expectExceptionMessage('\Drupal\Core\Database\Connection::$identifierQuotes must contain 2 string values');
+    $mock_pdo = $this->createMock(StubPDO::class);
+    new StubConnection($mock_pdo, [], [0, '1']);
+  }
+
 }

core/tests/Drupal/Tests/Core/Database/OrderByTest.php

@@ -25,9 +25,6 @@ class OrderByTest extends UnitTestCase {
   protected function setUp() {
     $connection = $this->getMockBuilder('Drupal\Core\Database\Connection')
       ->disableOriginalConstructor()
-      // Prevent deprecation message being triggered by
-      // Connection::identifierQuote().
-      ->setMethods(['identifierQuote'])
       ->getMockForAbstractClass();
     $this->query = new Select($connection, 'test', NULL);
   }

core/tests/Drupal/Tests/Core/Database/Stub/StubConnection.php

@@ -20,13 +20,6 @@ class StubConnection extends Connection {
    */
   public $driver = 'stub';
 
-  /**
-   * The identifier quote character. Can be set in the constructor for testing.
-   *
-   * @var string
-   */
-  protected $identifierQuote = '';
-
   /**
    * Constructs a Connection object.
    *
@@ -34,21 +27,14 @@ class StubConnection extends Connection {
    *   An object of the PDO class representing a database connection.
    * @param array $connection_options
    *   An array of options for the connection.
-   * @param string $identifier_quote
-   *   The identifier quote character. Defaults to an empty string.
+   * @param string[]|null $identifier_quotes
+   *   The identifier quote characters. Defaults to an empty strings.
    */
-  public function __construct(\PDO $connection, array $connection_options, $identifier_quote = '') {
-    $this->identifierQuote = $identifier_quote;
+  public function __construct(\PDO $connection, array $connection_options, $identifier_quotes = ['', '']) {
+    $this->identifierQuotes = $identifier_quotes;
     parent::__construct($connection, $connection_options);
   }
 
-  /**
-   * {@inheritdoc}
-   */
-  protected function identifierQuote() {
-    return $this->identifierQuote;
-  }
-
   /**
    * {@inheritdoc}
    */