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?

Files: 
CommentFileSizeAuthor
#37 date-format-case-1012620-37.patch4.41 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 39,753 pass(es).
[ View ]
#37 date-format-case-1012620-37-interdiff.txt1.36 KBBerdir
#36 date-format-case-1012620-36.patch4.21 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 39,731 pass(es).
[ View ]
#36 date-format-case-1012620-36-interdiff.txt2.06 KBBerdir
#31 date-format-case-1012620-31.patch4.12 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 39,708 pass(es).
[ View ]
#31 date-format-case-1012620-31-test-only.patch3.23 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 39,658 pass(es), 6 fail(s), and 2 exception(s).
[ View ]
#27 date-format-case-1012620-27-tests-only.patch2 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 39,665 pass(es), 5 fail(s), and 1 exception(s).
[ View ]
#27 date-format-case-1012620-27.patch2.43 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 39,671 pass(es).
[ View ]
#24 date-format-case-test-coverage-1012620-24.patch1.47 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 49,345 pass(es).
[ View ]
#18 drupal-unique_key_on_date-1012620-18-testsonly-shouldfail.patch1.18 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 48,578 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#18 drupal-unique_key_on_date-1012620-18.patch1.63 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 48,555 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#16 101260-14.patch1.64 KBkbasarab
PASSED: [[SimpleTest]]: [MySQL] 39,899 pass(es).
[ View ]
#11 test_only.patch1.09 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 35,361 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#10 add-binary-to-date-format-1012620-10.patch1.54 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch add-binary-to-date-format-1012620-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Subscribing...

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.

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.

That does indeed sound like an easy and better solution.

Version:7.x-dev» 8.x-dev
Issue tags:+needs backport to D7

Priority:Normal» Major

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.

Status:Active» Needs review
StatusFileSize
new1.54 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch add-binary-to-date-format-1012620-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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 :)

StatusFileSize
new1.09 KB
FAILED: [[SimpleTest]]: [MySQL] 35,361 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

Test only patch that will fail.

Status:Needs review» Needs work

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

Status:Needs work» Needs review

Back to needs review.

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.

StatusFileSize
new1.64 KB
PASSED: [[SimpleTest]]: [MySQL] 39,899 pass(es).
[ View ]

Apparently it didn't attach...

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.

StatusFileSize
new1.63 KB
FAILED: [[SimpleTest]]: [MySQL] 48,555 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.18 KB
FAILED: [[SimpleTest]]: [MySQL] 48,578 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

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.

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.

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.

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new1.47 KB
PASSED: [[SimpleTest]]: [MySQL] 49,345 pass(es).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

Test looks good.

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.

Status:Needs work» Needs review
StatusFileSize
new2.43 KB
PASSED: [[SimpleTest]]: [MySQL] 39,671 pass(es).
[ View ]
new2 KB
FAILED: [[SimpleTest]]: [MySQL] 39,665 pass(es), 5 fail(s), and 1 exception(s).
[ View ]

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

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

Status:Needs review» Needs work

That sounds like needs work.

Status:Needs work» Needs review
StatusFileSize
new3.23 KB
FAILED: [[SimpleTest]]: [MySQL] 39,658 pass(es), 6 fail(s), and 2 exception(s).
[ View ]
new4.12 KB
PASSED: [[SimpleTest]]: [MySQL] 39,708 pass(es).
[ View ]

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.

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).

Status:Needs work» Needs review

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

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.

StatusFileSize
new2.06 KB
new4.21 KB
PASSED: [[SimpleTest]]: [MySQL] 39,731 pass(es).
[ View ]

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

StatusFileSize
new1.36 KB
new4.41 KB
PASSED: [[SimpleTest]]: [MySQL] 39,753 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Thanks, looks done to me!

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.