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:
- Create a table in the new schema;
- Add, change, and delete a primary key;
- Add, change and delete a unique key;
- Add, change and delete a non unique key;
- Add, change and delete a column/field;
- Do a table search;
- Rename a table;
- Insert data in the table;
- Update the data in the table;
- Merge the data in the table;
- Upsert data in the table;
- Delete data from the table;
- Truncate the data in the table;
- Drop the table;
Remaining tasks
TBD
User interface changes
None
API changes
None
Data model changes
None
Release notes snippet
TBD
Comment | File | Size | Author |
---|---|---|---|
#68 | 3130579_68.patch | 75.75 KB | Ghost of Drupal Past |
Issue fork drupal-3130579
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:
Comments
Comment #2
daffie CreditAttribution: daffie commentedComment #3
daffie CreditAttribution: daffie commentedComment #5
johnwebdev CreditAttribution: johnwebdev commentedNice! Great test 👍
Comment #6
daffie CreditAttribution: daffie commentedFor the committer: Please give user @jsst commit credits for his work on #3033043: SQlite index names not quoted.
Comment #7
daffie CreditAttribution: daffie commentedComment #8
catchWhy does this use backticks rather than double quotes?
Comment #9
alexpottNice 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
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?
A touch less code with
$fields = array_map([$this->connection, 'escapeField'], $fields);
I think we also need to quote $info['table'] _ $key here too. Just in case. I'm not sure about $info['schema']
Here too - not sure about $info['schema'].
Comment #10
daffie CreditAttribution: daffie commented@catch said on Slack: "I think it's RC-eligible but better before than during."
@xjm said on Slack: "Yeah seems major to me"
Comment #11
daffie CreditAttribution: daffie commented@catch and @alexpott: Thank you for the review.
For #8 and #9.1: Changed all backticks and double quotes to block quotes.
For #9.2: Fixed.
For #9.3: Fixed.
For #9.4: Fixed.
Comment #12
johnwebdev CreditAttribution: johnwebdev commentedI 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.
Comment #13
TR CreditAttribution: TR commentedComment #14
daffie CreditAttribution: daffie commentedComment #15
daffie CreditAttribution: daffie commentedClosed #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.
Comment #16
jcisio CreditAttribution: jcisio as a volunteer and at Axess Open Web Services commentedReroll.
Comment #17
jcisio CreditAttribution: jcisio as a volunteer and at Axess Open Web Services commentedI 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?
Comment #18
TR CreditAttribution: TR commentedYes, 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.
Comment #19
daffie CreditAttribution: daffie commented@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).
Comment #20
jcisio CreditAttribution: jcisio as a volunteer and at Axess Open Web Services commentedBecause #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?
Comment #21
kenton.r CreditAttribution: kenton.r commentedI 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?"
Comment #23
mondrakeNeeds 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.Comment #24
markdorisonRe-rolled patch from #16. Suggestions from #23 not yet addressed.
Comment #25
mondrakeComment #26
jcisio CreditAttribution: jcisio as a volunteer and at Axess Open Web Services commented#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.
Comment #27
markdorisonFixed PHPCS issue.
Comment #28
alexpottI 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.
Comment #30
heddnAnother example of reserved name: #3221969: Exception got while installing (with sqlite DB)
Comment #32
artem_sylchukHere 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
Comment #33
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedRe-roll for the latest 9.4.x. Please have a look
Comment #34
daffie CreditAttribution: daffie commentedSuresh, there are al lot of style guide violations and unnecessay changes in your patch.
Comment #36
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedRe-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.
Comment #40
dragan_bp CreditAttribution: dragan_bp as a volunteer commentedHere's a fix for #33 on 9.4.x. Removed unnecessary changes
Comment #41
daffie CreditAttribution: daffie commented@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.
Comment #42
daffie CreditAttribution: daffie commentedComment #43
daffie CreditAttribution: daffie commentedComment #44
pradhumanjain2311 CreditAttribution: pradhumanjain2311 at OpenSense Labs for DrupalFit commentedTry to Fix Patch #40 for 10.1.x.
Comment #45
daffie CreditAttribution: daffie commentedComment #46
pradhumanjain2311 CreditAttribution: pradhumanjain2311 at OpenSense Labs for DrupalFit commented@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
Comment #47
daffie CreditAttribution: daffie commentedThe 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.
Comment #48
pradhumanjain2311 CreditAttribution: pradhumanjain2311 at OpenSense Labs for DrupalFit commented@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.
Comment #49
daffie CreditAttribution: daffie commentedStill waiting for the testbot results for PostgreSQL, SQLite and MariaDB.
Something that needs to be fixed:
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.
Comment #51
daffie CreditAttribution: daffie at Finalist commentedGiven @jsst commit credits for his work on #3033043: SQlite index names not quoted.
Comment #52
mondrakeChanged to MR workflow and fixed driver specific test.
Comment #53
daffie CreditAttribution: daffie at Finalist commentedMy 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!
Comment #54
alexpottCommitted 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.
Comment #57
xjmI think this is still backportable... maybe this status will get a backport rolled?
Comment #58
Ghost of Drupal Pastgit cherry-pick 62e6052
core/tests/Drupal/KernelTests/Core/Database/DriverSpecificSchemaTestBase.php
andcore/modules/pgsql/tests/src/Kernel/pgsql/SchemaTest.php
php -l
both. Observe no syntax errors.git diff origin/9.5.x
both. Observe new methods added and nothing more.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.
Comment #60
Ghost of Drupal PastI copied the
checkSchemaComment
method from10.1.x:core/modules/mysql/tests/src/Kernel/mysql/SchemaTest.php
tocore/modules/mysql/tests/src/Kernel/mysql/SchemaTest.php
. Still going blind.Comment #62
Ghost of Drupal PastA
use
statement is hopefully all that's needed now.Comment #64
catchGood to have a patch for people to apply, but 9.5.x is security-only as of a month ago.
Comment #65
Ghost of Drupal PastStill, it's worth getting this to green.
Comment #66
Ghost of Drupal PastComment #67
Ghost of Drupal PastComment #68
Ghost of Drupal PastComment #69
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commented