Another issue discovered by Privatemsg, while working on #994348: Display different date/time formats (based on the age of the message) at /messages :)

Because the default collation for mysql is usually something like utf8_general_ci (ci = case insensitive), it is not possible to define date formats like "d/m/Y" *and* "d/m/y" because it fails with a duplicate key error.

Not sure how to fix, my suggestion would be to replace the UNIQUE index with a normal index.

Or maybe use db_merge() instead of insert to at least avoid these nasty errors. Especially since they could also happen if two modules define the same date format.

Or both, because it could be confusing if you define "d/m/y" and "d/m/Y" and only the first one ever shows up.

PS: This is not Mysql bug or something like that like the other collation issue, it is by definition. That is why all these collations have a _ci suffix..

Thoughts?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

BenK’s picture

Subscribing...

jhodgdon’s picture

Good catch! PHP date strings are definitely case sensitive, so it's important to be able to put upper/lower variations of them in the formats database.

Replacing the unique index with a regular index seems like a good start. But I think you'd need to be careful about this, because the unique index is probably also attempting to enforce real uniqueness of the formats. So you'd also need to do something else in the code to make sure that no real php-identical string duplicate formats were added to the table. Or at least verify that that is already being done.

bfroehle’s picture

As noticed by Berdir, The addition of a 'binary' schema option in #966210-7: DB Case Sensitivity: system_update_7061() fails on inserting files with same name but different case would present an easy solution for this issue.

jhodgdon’s picture

That does indeed sound like an easy and better solution.

bfroehle’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
bfroehle’s picture

bfroehle’s picture

bfroehle’s picture

Priority: Normal » Major
Berdir’s picture

Issue tags: +Needs tests, +Novice

We have support for BINARY now, which means that a patch here should be trivial.

I'll get to it in the next days but if someone wants to do, you can find an example in #966210: DB Case Sensitivity: system_update_7061() fails on inserting files with same name but different case and the privatemsg patch in #994348-6: Display different date/time formats (based on the age of the message) at /messages contains code that actually triggers this error. That hook could be implemented in a test module (maybe we already have a test implementation for this somewhere) and then visit the admin page to see if both formats are there.

Berdir’s picture

Status: Active » Needs review
FileSize
1.54 KB

Ok, attached a patch.

According to #966210-88: DB Case Sensitivity: system_update_7061() fails on inserting files with same name but different case, this does not need a update function but the 7.x backport will need one.

To reproduce:

- Add a custom date format, e.g. d/m/Y
- Add another one that only differs in the case, e.g. d/m/y. (after a re-install, there is no update function)

Before patch: Validation error teilling you that the format already exists
After patch: Works :)

Berdir’s picture

FileSize
1.09 KB

Test only patch that will fail.

Status: Needs review » Needs work

The last submitted patch, test_only.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Back to needs review.

xjm’s picture

kbasarab’s picture

Reroll of patch in #10.

Locally the tests in this patch are passing but there are 4 others failing. When trying on HEAD the same 4 tests fail prior to this patch.

kbasarab’s picture

FileSize
1.64 KB

Apparently it didn't attach...

YesCT’s picture

Assigned: Unassigned » YesCT

I'll work on this to get a tests only and whole patch that applies, and get the bot to recheck them.

YesCT’s picture

in irc @berdir says... about the date format issue, note that it will be irrelevant for 8.x as soon as date formats are converted to CMI. still needs to be fixed in 7.x, though

well. here it is anyway.

YesCT’s picture

it's the same as the 14 patch (from #16)

Status: Needs review » Needs work
Issue tags: -Needs tests, -Novice, -Needs backport to D7

The last submitted patch, drupal-unique_key_on_date-1012620-18.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Needs tests, +Novice, +Needs backport to D7

The last submitted patch, drupal-unique_key_on_date-1012620-18.patch, failed testing.

YesCT’s picture

Assigned: YesCT » Unassigned

well. this was non-trivial. :)

Have to investigate the test fail for the patch...
and backport it to 7 using the advice from #10

un-assigning it from myself since I'm done for the day today.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.47 KB

Right, the multilingual cmi patch landed in 8.x, so the test now obviously passes. The confirmation message is different now, I guess that's why the patch above failed.

The bug here is now obviously fixed, as it is just a string in a yaml file but I suggest we still add test coverage and then backport the previous patch, including upgrade path.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Test looks good.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs work

Committed/pushed the test to 8.x, moving to 7.x.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.43 KB
2 KB

Ok, here is a backport to 7.x including the fix.

xjm’s picture

Don't we need a hook_update_N() (preferably with a 7.0 -> 7.x upgrade path test)?

xjm’s picture

webchick’s picture

Status: Needs review » Needs work

That sounds like needs work.

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.23 KB
4.12 KB

Upgrade path + tests added.

Copied over the relevant parts from the test into one of the upgrade path test, confirmed that it fails without the update function.

Note that I did not copy everything from the actual test, the additional stuff tests of the API functions really work, this part is enough to expose the bug.

Status: Needs review » Needs work

The last submitted patch, date-format-case-1012620-31-test-only.patch, failed testing.

Berdir’s picture

Strange random test failure:

exception 'DatabaseSchemaObjectExistsException' with message 'Table variable already exists.' in /var/lib/drupaltestbot/sites/default/files/checkout/includes/database/schema.inc:657
Stack trace:
#0 /var/lib/drupaltestbot/sites/default/files/checkout/includes/database/database.inc(2717): DatabaseSchema->createTable('variable', Array)
#1 /var/lib/drupaltestbot/sites/default/files/checkout/includes/common.inc(6869): db_create_table('variable', Array)
#2 /var/lib/drupaltestbot/sites/default/files/checkout/modules/system/system.install(497): drupal_install_schema('system')
#3 [internal function]: system_install()
#4 /var/lib/drupaltestbot/sites/default/files/checkout/includes/module.inc(833): call_user_func_array('system_install', Array)
#5 /var/lib/drupaltestbot/sites/default/files/checkout/includes/install.inc(724): module_invoke('system', 'install')
#6 /var/lib/drupaltestbot/sites/default/files/checkout/modules/simpletest/drupal_web_test_case.php(1444): drupal_install_system()
#7 /var/lib/drupaltestbot/sites/default/files/checkout/modules/simpletest/tests/file.test(240): DrupalWebTestCase->setUp('file_test')
#8 /var/lib/drupaltestbot/sites/default/files/checkout/modules/simpletest/drupal_web_test_case.php(500): FileHookTestCase->setUp()
#9 /var/lib/drupaltestbot/sites/default/files/checkout/scripts/run-tests.sh(366): DrupalTestCase->run()
#10 /var/lib/drupaltestbot/sites/default/files/checkout/scripts/run-tests.sh(22): simpletest_script_run_one_test('1', 'FileMoveTest')
#11 {main}FATAL FileNameMungingTest: test runner returned a non-zero error code (1).
Berdir’s picture

Status: Needs work » Needs review

#31: date-format-case-1012620-31.patch queued for re-testing.

xjm’s picture

This looks pretty good to me. I suggested to @Berdir that it might be good to have a second assertion in the upgrade test just to confirm that a previously existing date format still works.

The comments are a little awkward and have some typos; here's how I'd suggest changing them:

+++ b/modules/simpletest/tests/common.testundefined
@@ -2298,6 +2298,12 @@ class FormatDateUnitTest extends DrupalWebTestCase {
+    // Add a new date format which just differs in the case.

+++ b/modules/simpletest/tests/upgrade/upgrade.testundefined
@@ -566,6 +566,17 @@ class BasicMinimalUpdatePath extends UpdatePathTestCase {
+    // Add a new date format which just differs in the case.

I'd change these to:
// Add a new date format that is identical except for its case.
(If that's correct in context).

+++ b/modules/simpletest/tests/upgrade/upgrade.testundefined
@@ -566,6 +566,17 @@ class BasicMinimalUpdatePath extends UpdatePathTestCase {
+    // Confirm that a date format that just differ in the case can be added.

I'd change this to:
// Confirm that date formats are case-sensitive.

Berdir’s picture

Added an assert and updated the comments. Removed the second comment from upgrade.test, which I think is unecessary.

Berdir’s picture

Ok, also took care of the unique key while changing that field.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, looks done to me!

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)
Issue tags: -Novice, -Needs backport to D7, -Needs upgrade path tests

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