Problem/Motivation

Setting collation in the table schema for MySQL tables has no effect. Per the original report:

I would very much like to see it fixed in D7 ASAP, because it's causing major problems for my module, S3 File System. I define a table with utf8_bin collation, since I need case-sensitive primary keys, but the collation setting gets ignored, and I have to run db_query("ALTER TABLE {s3fs_file} CONVERT TO CHARACTER SET utf8 COLLATE utf8_bin"); manually to fix it.

This bug also makes it clear that the tests for the database creation code are incomplete.

Proposed resolution

Apply any explicit collation settings in the table schema before checking for collation settings in the connection info.

Remaining tasks

Write tests (MySQL-specific, presumably - do the other core db engines purport to support table-specific collations?).

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

TBD

Original issue summary

There's a typo in DatabaseSchema_mysql::createTableSql() which makes the 'collation' setting in schema definitions get silently ignored. The schema info variable is called $table, but it's referenced in the collation code as $info. Since it's wrapped in a !empty() check, PHP happily confirms that $info['collation'] is always empty, because $info is undefined.

The fix is simply to rename $info to $table, which I've done in the attached patches (this bug exists in D8 and D7).

I would very much like to see it fixed in D7 ASAP, because it's causing major problems for my module, S3 File System. I define a table with utf8_bin collation, since I need case-sensitive primary keys, but the collation setting gets ignored, and I have to run db_query("ALTER TABLE {s3fs_file} CONVERT TO CHARACTER SET utf8 COLLATE utf8_bin"); manually to fix it.

This bug also makes it clear that the tests for the database creation code are incomplete. Unfortunately, I know nothing about drupal testing, so I can't write a patch for that.

CommentFileSizeAuthor
#32 D8-mysql_collation_fix-2193059-32.patch1.12 KBanya_m
#17 D8-mysql_collation_fix-2193059-17.patch1.09 KBkars-t
#17 D7-mysql_collation_fix-2193059-17-do-not-test.patch1.02 KBkars-t
#11 D7-mysql_collation_fix-2193059-11.patch1.02 KBcoredumperror
#11 D8-mysql_collation_fix-2193059-11.patch1.09 KBcoredumperror
#10 D7-mysql_collation_fix-2193059-10.patch1.02 KBcoredumperror
#10 D8-mysql_collation_fix-2193059-10.patch1.09 KBcoredumperror
D8-mysql_collation_fix.patch717 bytescoredumperror
D7-mysql_collation_fix.patch642 bytescoredumperror

Issue fork drupal-2193059

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:

About issue forks

Comments

The last submitted patch, D7-mysql_collation_fix.patch, failed testing.

coredumperror’s picture

coredumperror commented

D7-mysql_collation_fix.patch queued for re-testing.

The last submitted patch, D7-mysql_collation_fix.patch, failed testing.

coredumperror’s picture

coredumperror commented

The test bot is complaining about the D7 patch failing to apply because it's attempting to apply it against 8.x-dev.

Should I open a separate issue for the D7 version of this bug?

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Needs review » Reviewed & tested by the community

I started to add a test, but quickly realized it would require adding new methods to our Schema classes (mysql stores this information in information_schema.table), but we can't just code that directly into the test since that would break on non-mysql drivers.

Setting to RTBC since this fixes an obvious typo, and need not be blocked by the creation of a test.

Re #4 once this is committed to 8, and the version is set to 7.x, then your tests should run.

coredumperror’s picture

coredumperror commented

Thank you! I am very much looking forward to this fix making into the official Drupal release.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

I'm not sure this is the correct fix. The collation should be set on the connection options. in D8 $info is set by the first line DatabaseSchema::createTableSql() see https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Database%...

This means that the collation needs to be set in the settings.php and is set for all tables. At the moment the implementation does not support per table or per column collation settings.

coredumperror’s picture

coredumperror commented

Wow, I completely missed the definition of the $info variable up there at the top of the function. I vaguely recall assuming this was a copy-paste typo based on some other implementation of createTableSql(), and it appears I may have been mistaken.

Still, the docs state that you can set the collation in the schema definition, which this function, as it currently stands, does not actually allow.

> At the moment the implementation does not support per table or per column collation settings.

Why not? What bad things could happen if $info were changed to $table here? Myself and all of other users of S3 File System have been using a single utf8_bin collated column in our s3fs_files tables for months, with no issues (I found a workaround).

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

So - we need to first check the collation on the schema definition and then fallback to collation set in the connection properties. So we can't just change $info to $table since that would be breaking existing functionality that other sites rely on.

coredumperror’s picture

coredumperror commented
StatusFileSize
new D8-mysql_collation_fix-2193059-10.patch1.09 KB
new D7-mysql_collation_fix-2193059-10.patch1.02 KB

Ah, right. OK, here are modified patches that should do just that.

coredumperror’s picture

coredumperror commented
StatusFileSize
new D8-mysql_collation_fix-2193059-11.patch1.09 KB
new D7-mysql_collation_fix-2193059-11.patch1.02 KB

Oh damnit, I'm a moron. The typo I was complaining about which wasn't actually a typo... is a typo that I totally left in my updated patches. Wow I feel like a fool now.

Here's the un-typo'd patches.

kars-t’s picture

kars-t commented
Status: Needs work » Needs review
Issue tags: +Needs tests

I move the issue to "needs review" so the testbot can test it.

Test seem to be a problem here. I searched core if there are already collation tests. I only found "CaseSensitivityTest" which doesn't really seem to test if a table got a different collation. So it seems core here misses tests and this issue imho shouldn't get RTBC if there are no tests. And finally: If someone writes a test wouldn't that be mysql only?

Besides missing tests I believe the patches should fix the issue.

kars-t’s picture

kars-t commented
Title: Typo in DatabaseSchema_mysql::createTableSql() breaks table collation setting » DatabaseSchema_mysql::createTableSql() can't set table collation

I changed the issue title as stated in #9 the core behaviour is correct and the setting per table simply ignored.

Status: Needs review » Needs work

The last submitted patch, 11: D7-mysql_collation_fix-2193059-11.patch, failed testing.

Status: Needs work » Needs review

Kars-T queued 11: D7-mysql_collation_fix-2193059-11.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 11: D7-mysql_collation_fix-2193059-11.patch, failed testing.

kars-t’s picture

kars-t commented
StatusFileSize
new D7-mysql_collation_fix-2193059-17-do-not-test.patch1.02 KB
new D8-mysql_collation_fix-2193059-17.patch1.09 KB

Readding the patches while adding "do-not-test" to the D7 patch. By this the D8 patch can go green. @see https://www.drupal.org/node/332678#versions

kars-t’s picture

kars-t commented
Status: Needs work » Needs review

Forgot to change to "needs review".

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Issue tags: +Needs issue summary update

Issue summary needs to be updated to take into account the most recent patch (it was determined this is not a typo).

Small nit: 

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -109,10 +109,14 @@ protected function createTableSql($name, $table) {
+    else if (!empty($info['collation'])) {

Should be elseif.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Needs review » Needs work

This is at needs work for tests.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

Although, note the test may need to be MySQL-specific given #5.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs work » Needs review

Bump - this is still a problem: #2972251: "s3fs_file" table has no primary key.

Since it's been three years and a day since the last activity here, let's retest that last patch...

The last submitted patch, 17: D7-mysql_collation_fix-2193059-17-do-not-test.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 17: D8-mysql_collation_fix-2193059-17.patch, failed testing. View results

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Issue tags: +Needs reroll
anya_m’s picture

anya_m commented
Version: 8.6.x-dev » 8.7.x-dev
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new D8-mysql_collation_fix-2193059-32.patch1.12 KB

Rerolled patch for 8.7.x

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs issue summary update

Thanks for the reroll @anya_m!

Setting back to "Needs work" as we still need tests...

I've updated the issue summary.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

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

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

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

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

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.

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.

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.

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.

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

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

pameeela’s picture

pameeela commented
Status: Needs work » Needs review
Issue tags: +Bug Smash Initiative

Created an MR based on the latest patch.

Given that this is a pretty simple change, and tests will be difficult to add, I think it might qualify for the test exemption per the updated policy. Interestingly this was suggested 10 years ago in #5 but now the policy is official :)

smustgrave opened merge request !8217

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.