Problem/Motivation

In #2986452: Database reserved keywords need to be quoted as per the ANSI standard we fixed working with reserved keywords for queries. For the class Drupal\Core\Database\Schema there is nothing done. In #3023794: MySQL >= 8.0.2 Bug: Exception during module activation because of reserved keyword "GROUPS" we have a problem with using sites with the "group" module on Drupal 8 with MySQL 8. The problem should be fixed for Drupal 9, only for queries not for the class Drupal\Core\Database\Schema.
In #3033043: SQlite index names not quoted we have the problem: "An example of such a module is restrict_ip (https://www.drupal.org/project/restrict_ip) which defines an index named 'type-path'. The index name is perfectly valid on both MySQL and SQlite."

Proposed resolution

Add testing for working with reserved keywords in combination with the class Drupal\Core\Database\Schema. Add fixes where necessary. All functionality that the Schema class offers needs testing with reserved keywords. The needed test functionality is:

  1. Create a table in the new schema;
  2. Add, change, and delete a primary key;
  3. Add, change and delete a unique key;
  4. Add, change and delete a non unique key;
  5. Add, change and delete a column/field;
  6. Do a table search;
  7. Rename a table;
  8. Insert data in the table;
  9. Update the data in the table;
  10. Merge the data in the table;
  11. Upsert data in the table;
  12. Delete data from the table;
  13. Truncate the data in the table;
  14. Drop the table;

Remaining tasks

TBD

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

TBD

CommentFileSizeAuthor
#68 3130579_68.patch75.75 KBGhost of Drupal Past
#67 3130579_68.patch75.47 KBGhost of Drupal Past
#66 3130579_66.patch75.54 KBGhost of Drupal Past
#65 3130579_65.patch75.82 KBGhost of Drupal Past
#62 3130579_62.patch75.54 KBGhost of Drupal Past
#60 3130579_60.patch75.27 KBGhost of Drupal Past
#58 3130579_58.patch74.4 KBGhost of Drupal Past
#48 3130579-48.patch21.28 KBpradhumanjain2311
#44 3130579-44.patch21.2 KBpradhumanjain2311
#40 3130579-38.patch30.23 KBdragan_bp
#36 3130579-36.patch21.21 KBSutharsan
#33 3130579-33.patch43.51 KBSuresh Prabhu Parkala
#32 3130579-32.patch21.18 KBartem_sylchuk
#27 interdiff-3130579-24-27.txt584 bytesmarkdorison
#27 3130579-27.patch21.64 KBmarkdorison
#24 3130579-24.patch21.64 KBmarkdorison
#16 3130579-sql-reserved-words-16.patch21.64 KBjcisio
#11 3130579-11.patch21.71 KBdaffie
#11 interdiff-3130579-2-11.txt15.2 KBdaffie
#3 3130579-2-test-only-should-fail.patch5.03 KBdaffie
#2 3130579-2.patch9.69 KBdaffie

Issue fork drupal-3130579

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

daffie created an issue. See original summary.

Status: Needs review » Needs work

The last submitted patch, 3: 3130579-2-test-only-should-fail.patch, failed testing. View results

johnwebdev’s picture

Status: Needs work » Reviewed & tested by the community

Nice! Great test 👍

daffie’s picture

Issue summary: View changes
Related issues: +#3033043: SQlite index names not quoted

For the committer: Please give user @jsst commit credits for his work on #3033043: SQlite index names not quoted.

daffie’s picture

Issue summary: View changes
catch’s picture

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -699,7 +699,7 @@ public function fieldExists($table, $column) {
     //   that has that bug fixed.
     try {
-      $this->connection->queryRange("SELECT $column FROM {" . $table . "}", 0, 1);
+      $this->connection->queryRange("SELECT `" . $column . "` FROM {" . $table . "}", 0, 1);
       return TRUE;
     }

Why does this use backticks rather than double quotes?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Nice finds. And yep we definitely need to go through core SQL and look for places that need to use square brackets. The Schema class is

  1. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
    @@ -699,7 +699,7 @@ public function fieldExists($table, $column) {
    -      $this->connection->queryRange("SELECT $column FROM {" . $table . "}", 0, 1);
    +      $this->connection->queryRange("SELECT `" . $column . "` FROM {" . $table . "}", 0, 1);
    

    We could use square bracket here. That said I can see the argument for using " directly here. I'd like to avoid yet another delimiter - ie the backtick. Ah I see we already have backticks in \Drupal\Core\Database\Driver\mysql\Schema::createFieldSql() etc... fun. Ho hum. Does this matter - I guess not. In MySQL backticks and double quotes are equivalent for us as we set ANSI as the sql mode in \Drupal\Core\Database\Driver\mysql\Connection::open(). In #2986452: Database reserved keywords need to be quoted as per the ANSI standard I went for double quotes in \Drupal\Core\Database\Driver\mysql\Connection::$identifierQuotes because then all core dbs had the same identifier quote which seemed nice to me.

    We also should file an issue to see if we need this override - ie. is the bug here is still relevant to us?

  2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
    @@ -794,6 +794,9 @@ public function addUniqueKey($table, $name, $fields) {
    +    foreach ($fields as &$field) {
    +      $field = $this->connection->escapeField($field);
    +    }
    

    A touch less code with $fields = array_map([$this->connection, 'escapeField'], $fields);

  3. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
    @@ -69,12 +69,12 @@ protected function createIndexSql($tablename, $schema) {
    -        $sql[] = 'CREATE UNIQUE INDEX ' . $info['schema'] . '.' . $info['table'] . '_' . $key . ' ON ' . $info['table'] . ' (' . $this->createKeySql($fields) . ")\n";
    +        $sql[] = 'CREATE UNIQUE INDEX ' . $info['schema'] . '.' . $info['table'] . '_' . $key . ' ON "' . $info['table'] . '" (' . $this->createKeySql($fields) . ")\n";
    ...
    -        $sql[] = 'CREATE INDEX ' . $info['schema'] . '.' . $info['table'] . '_' . $key . ' ON ' . $info['table'] . ' (' . $this->createKeySql($fields) . ")\n";
    +        $sql[] = 'CREATE INDEX ' . $info['schema'] . '.' . $info['table'] . '_' . $key . ' ON "' . $info['table'] . '" (' . $this->createKeySql($fields) . ")\n";
    

    I think we also need to quote $info['table'] _ $key here too. Just in case. I'm not sure about $info['schema']

  4. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
    @@ -488,7 +488,7 @@ protected function introspectSchema($table) {
    -    $result = $this->connection->query('PRAGMA ' . $info['schema'] . '.table_info(' . $info['table'] . ')');
    +    $result = $this->connection->query('PRAGMA ' . $info['schema'] . '.table_info("' . $info['table'] . '")');
    
    @@ -525,7 +525,7 @@ protected function introspectSchema($table) {
    -    $result = $this->connection->query('PRAGMA ' . $info['schema'] . '.index_list(' . $info['table'] . ')');
    +    $result = $this->connection->query('PRAGMA ' . $info['schema'] . '.index_list("' . $info['table'] . '")');
    

    Here too - not sure about $info['schema'].

daffie’s picture

Priority: Normal » Major

@catch said on Slack: "I think it's RC-eligible but better before than during."

@xjm said on Slack: "Yeah seems major to me"

johnwebdev’s picture

I think we should document this as a Drupal standard, update documentation (i.e. https://www.drupal.org/docs/8/api/database-api)
and fix occurrences in core (search for $database->query).

As follow ups.

TR’s picture

Issue tags: +MySQL 8
daffie’s picture

Issue tags: +Bug Smash Initiative
daffie’s picture

Closed #2884129: When creating an entity whose base table corresponds to an SQL reserved word, storage fails as a duplicate of this issue. Could we give @tetranz commit credits on this issue for his work on the duplicate issue.

jcisio’s picture

I wanted to add a test that alters table but it turns out that that test is already there.

Patch works for 9.0 but not 8.9 because the introduction of square bracket in 9.0. Should it be backported to 8.9 using the backstick?

TR’s picture

Yes, this ABSOLUTELY needs to be fixed in D8.

D8 claims to be compatible with MySql 8, but it's not ... and that's because of this issue.

Both #3023794: MySQL >= 8.0.2 Bug: Exception during module activation because of reserved keyword "GROUPS" and #3150212: votingapi_update_8303() fails with MySql 8 point out major/critical issues caused by this bug in D8.

With the former, working D8 sites with the Group module installed fail catastrophically and are unrecoverable if they update to MySql 8. That's not something that can be fixed in Group. Hope you had a backup ... and hope you can downgrade to MySql 5.

With the latter, working sites with D8 and with the Voting API module can't be updated if MySql 8 is installed. Again, it doesn't appear that this is something that can be fixed in the Voting API, but it can be worked around by uninstalling MySql 8 and going back to MySql 5.

These are both D8 issues which can't be avoided, and which people are LIKELY to encounter because in order to upgrade from D8 to D9 you first need to upgrade your PHP and MySql, and in many cases that will mean upgrading your OS/distribution to one that supports these newer PHP and MySql versions. Then BAM - your site is wrecked. If you checked first to see if Drupal supports MySql 8 you would have seen the official documentation that says it DOES so you still would have gone ahead with this upgrade. If you didn't check, well that's not your fault either because it's unreasonable to anticipate that Drupal 8 will break with a two-year-old "new" version of MySql.

So this is something that absolutely, definitely needs to be fixed in D8, not just in D9.

daffie’s picture

@TR: As I have explained in #3023794-9: MySQL >= 8.0.2 Bug: Exception during module activation because of reserved keyword "GROUPS", the chance of getting support for quoting of reserved keywords in Drupal 8 is very small. If you want to change this, my suggestion is to take it up on Slack with one of the Drupal core product maintainers (@webchick, @Gábor Hojtsy or @yoroy) or the Drupal core release managers (@catch or @xjm).

jcisio’s picture

Because #2986452: Database reserved keywords need to be quoted as per the ANSI standard will not be backported to D8, and D8 is expected to be supported in the next 18 months, could we "backport" this patch but using backticks instead of square brackets?

kenton.r’s picture

I would like to see this back ported. I have encountered the incompatibility of the Group with MySQL 8 on D8. Is there a possibility for or answer for the above question: "could we "backport" this patch but using backticks instead of square brackets?"

Version: 9.0.x-dev » 9.1.x-dev

Drupal 9.0.10 was released on December 3, 2020 and is the final full bugfix release for the Drupal 9.0.x series. Drupal 9.0.x will not receive any further development aside from security fixes. Sites should update to Drupal 9.1.0 to continue receiving regular bugfixes.

Drupal-9-only bug reports should be targeted for the 9.1.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.2.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mondrake’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs a reroll.

I do not think that using the new square brackets syntax for identifiers within database driver code is very useful. That syntax is meaningful when you want to have abstract SQL statements that need to be portable across different DBMSes, so that each DBMS can use the proper quote characters. But when in the database driver code, you do not need to care about that, and moreover DDL statements are seldom portable. In short - just use the quote characters specific for the DBMS at hand.

In the patch, the ` for MySql can be replaced with ", which is now the identifier used also by the MySql Connection class for escaping. PgSql already uses " in the schema statements. SQLite seems to miss the use of any quoting characters, and again here it could be " as per the one used by SQLite Connection class.

markdorison’s picture

Re-rolled patch from #16. Suggestions from #23 not yet addressed.

mondrake’s picture

Version: 9.1.x-dev » 9.2.x-dev
jcisio’s picture

#21 #23 I'd prefer not using square bracket in database drivers, patch will be small and backport for D8 will be straightforward. But we'd need @alexpott's feedback per #9.1.

markdorison’s picture

Fixed PHPCS issue.

alexpott’s picture

I could go either way here re #23. On one hand these classes are DB specific so using their own identifiers makes sense. However, the other way of thinking is that using square brackets here keeps things consistent and easier for people doing cross-database work and also makes it easier to copy and paste between drivers where they need the same thing - or potentially move stuff to traits or up the class hierarchy.

I find the arguments to use square brackets outweigh the reason to not.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

heddn’s picture

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

artem_sylchuk’s picture

Here is a quick re-roll for the recent 9.3.x branch, that has #2797141: Remove the methods tableExists() and fieldExists() from Drupal\Core\Database\Driver\mysql\Schema merged.
However for 9.4.x it will require another major re-roll because of #3129043: Move core database drivers to modules of their own, see https://www.drupal.org/node/3129492 for details

Suresh Prabhu Parkala’s picture

Status: Needs work » Needs review
FileSize
43.51 KB

Re-roll for the latest 9.4.x. Please have a look

daffie’s picture

Status: Needs review » Needs work

Suresh, there are al lot of style guide violations and unnecessay changes in your patch.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Sutharsan’s picture

FileSize
21.21 KB

Re-rolled the #27 patch for Drupal 9.5.x.

Note that two back ticks (in new code) are left in place in \Drupal\mysql\Driver\Database\mysql\Schema::changeField. As this is a re-roll I did not check for new code that also needs square brackets.

    if ($spec['type'] === 'serial') {
      $max = $this->connection->query('SELECT MAX(`' . $field_new . '`) FROM {' . $table . '}')->fetchField();
      $this->connection->query("ALTER TABLE {" . $table . "} AUTO_INCREMENT = " . ($max + 1));
    }

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

dragan_bp made their first commit to this issue’s fork.

dragan_bp’s picture

Here's a fix for #33 on 9.4.x. Removed unnecessary changes

daffie’s picture

Issue summary: View changes
Issue tags: +Needs reroll

@dragan_bp: Great to see that you are working on this issue. I shall help by reviewing your work.

The first goal is to get this fixed in 10.1.x. Later we maybe able to backport this to earlier Drupal versions.
The issue summery has a list of tests that are needed. It is a long list, only they are all very simple tests. It is less work than it looks.

daffie’s picture

Issue summary: View changes
daffie’s picture

Issue summary: View changes
pradhumanjain2311’s picture

Status: Needs work » Needs review
FileSize
21.2 KB

Try to Fix Patch #40 for 10.1.x.

daffie’s picture

Status: Needs review » Needs work
pradhumanjain2311’s picture

@daffie the patch was applied cleanly on my local. Would you please suggest me why this error occurs
error: core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php: No such file or directory

daffie’s picture

The file "core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php" has been renamed to "core/tests/Drupal/KernelTests/Core/Database/DriverSpecificSchemaTestBase.php". The test is now a "driver specific" test. In the test we only have test that are the same for database drivers. Every database driver override this test and adds tests that are specific for their own database driver. The MySQL specific tests go into "core/modules/mysql/tests/src/Kernel/mysql/SchemaTest.php"

@pradhumanjainOSL Did you use the 10.1.x branch? Did you do a "git pull" before working on the patch? You can also contact me on Drupal Slack. My username there is the same as it is here.

pradhumanjain2311’s picture

@daffie,
Thanks for your guidence.
Actually i forgot to take a pull. Apologies for that.
Now i made changes in "core/tests/Drupal/KernelTests/Core/Database/DriverSpecificSchemaTestBase.php".
Please review it.

daffie’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll

Still waiting for the testbot results for PostgreSQL, SQLite and MariaDB.

Something that needs to be fixed:

+++ b/core/tests/Drupal/KernelTests/Core/Database/DriverSpecificSchemaTestBase.php
@@ -1224,4 +1224,113 @@ public function testDefaultAfterAlter(): void {
+    if ($this->connection->databaseType() == 'pgsql') {
+      $ensure_identifiers_length = new \ReflectionMethod(get_class($this->schema), 'ensureIdentifiersLength');
+      $ensure_identifiers_length->setAccessible(TRUE);
+      $unique_key_introspect_name = $ensure_identifiers_length->invoke($this->schema, $table_name_new, $unique_key_name, 'key');
+    }
...
+    if ($this->connection->databaseType() == 'pgsql') {
+      $index_introspect_name = $ensure_identifiers_length->invoke($this->schema, $table_name_new, $index_name, 'idx');
+    }

These parts are specific for PostgreSQL. They need to be removed from this class and added to the override in the pgsql module. It would be helpful to cut put the single big test in a number of smaller ones. Than the override only has to override one or two smaller tests instead of overriding the whole big test.

@pradhumanjainOSL It looks good.

daffie credited jsst.

daffie’s picture

Issue summary: View changes

Given @jsst commit credits for his work on #3033043: SQlite index names not quoted.

daffie’s picture

Status: Needs work » Reviewed & tested by the community

My one remark from comment #49 has been addressed.
The bug has been fixed.
Testing has been added.
All code changes look good to me.
For me it is RTBC.

@mondrake: Thank very much for working on this issue!

alexpott’s picture

Version: 10.1.x-dev » 9.5.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed 9363b0e7a1 to 10.1.x and 62e6052d75 to 10.0.x. Thanks!

I think this is totally eligible for backport to 9.5.x - will need a reroll though.

  • alexpott committed 9363b0e on 10.1.x
    Issue #3130579 by daffie, markdorison, pradhumanjainOSL, dragan_bp,...

  • alexpott committed 62e6052 on 10.0.x
    Issue #3130579 by daffie, markdorison, pradhumanjainOSL, dragan_bp,...
xjm’s picture

Title: Make Drupal\Core\Database\Schema work with reserved keywords for naming » [9.5.x backport] Make Drupal\Core\Database\Schema work with reserved keywords for naming
Status: Patch (to be ported) » Needs work

I think this is still backportable... maybe this status will get a backport rolled?

Ghost of Drupal Past’s picture

  1. git cherry-pick 62e6052
  2. remove conflict markers from core/tests/Drupal/KernelTests/Core/Database/DriverSpecificSchemaTestBase.php and core/modules/pgsql/tests/src/Kernel/pgsql/SchemaTest.php
  3. php -l both. Observe no syntax errors.
  4. git diff origin/9.5.x both. Observe new methods added and nothing more.
  5. Submit patch

I have done nothing else and I have no idea what's in the patch. #3036725: Update requirements docs with a note about MySQL 8 issues that may affect some sites led me here.

Status: Needs review » Needs work

The last submitted patch, 58: 3130579_58.patch, failed testing. View results

Ghost of Drupal Past’s picture

I copied the checkSchemaComment method from 10.1.x:core/modules/mysql/tests/src/Kernel/mysql/SchemaTest.php to core/modules/mysql/tests/src/Kernel/mysql/SchemaTest.php. Still going blind.

Status: Needs review » Needs work

The last submitted patch, 60: 3130579_60.patch, failed testing. View results

Ghost of Drupal Past’s picture

Status: Needs work » Needs review
FileSize
75.54 KB

A use statement is hopefully all that's needed now.

Status: Needs review » Needs work

The last submitted patch, 62: 3130579_62.patch, failed testing. View results

catch’s picture

Title: [9.5.x backport] Make Drupal\Core\Database\Schema work with reserved keywords for naming » Make Drupal\Core\Database\Schema work with reserved keywords for naming
Status: Needs work » Fixed

Good to have a patch for people to apply, but 9.5.x is security-only as of a month ago.

Ghost of Drupal Past’s picture

FileSize
75.82 KB

Still, it's worth getting this to green.

Ghost of Drupal Past’s picture

FileSize
75.54 KB
Ghost of Drupal Past’s picture

FileSize
75.47 KB
Ghost of Drupal Past’s picture

Bhanu951’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.