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?
Comment | File | Size | Author |
---|---|---|---|
#37 | date-format-case-1012620-37.patch | 4.41 KB | Berdir |
#37 | date-format-case-1012620-37-interdiff.txt | 1.36 KB | Berdir |
#36 | date-format-case-1012620-36.patch | 4.21 KB | Berdir |
#36 | date-format-case-1012620-36-interdiff.txt | 2.06 KB | Berdir |
#31 | date-format-case-1012620-31.patch | 4.12 KB | Berdir |
Comments
Comment #1
BenK CreditAttribution: BenK commentedSubscribing...
Comment #2
jhodgdonGood 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.
Comment #3
bfroehle CreditAttribution: bfroehle commentedAs 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.
Comment #4
jhodgdonThat does indeed sound like an easy and better solution.
Comment #5
bfroehle CreditAttribution: bfroehle commentedComment #6
bfroehle CreditAttribution: bfroehle commentedBlocked on #1237252: DB Case Sensitivity: Allow BINARY attribute in MySQL.
Comment #7
bfroehle CreditAttribution: bfroehle commented#1006826: Case sensitive custom date formats 'd-m-y' and 'd-M-Y': PDOException: duplicate entry 'd-m-y-custom' is a duplicate of this.
Comment #8
bfroehle CreditAttribution: bfroehle commentedComment #9
BerdirWe 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.
Comment #10
BerdirOk, 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 :)
Comment #11
BerdirTest only patch that will fail.
Comment #13
BerdirBack to needs review.
Comment #14
xjm#10: add-binary-to-date-format-1012620-10.patch queued for re-testing.
Comment #15
kbasarab CreditAttribution: kbasarab commentedReroll 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.
Comment #16
kbasarab CreditAttribution: kbasarab commentedApparently it didn't attach...
Comment #17
YesCT CreditAttribution: YesCT commentedI'll work on this to get a tests only and whole patch that applies, and get the bot to recheck them.
Comment #18
YesCT CreditAttribution: YesCT commentedin 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.
Comment #19
YesCT CreditAttribution: YesCT commentedit's the same as the 14 patch (from #16)
Comment #21
YesCT CreditAttribution: YesCT commented#18: drupal-unique_key_on_date-1012620-18.patch queued for re-testing.
Comment #23
YesCT CreditAttribution: YesCT commentedwell. 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.
Comment #24
BerdirRight, 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.
Comment #25
catchTest looks good.
Comment #26
catchCommitted/pushed the test to 8.x, moving to 7.x.
Comment #27
BerdirOk, here is a backport to 7.x including the fix.
Comment #28
xjmDon't we need a
hook_update_N()
(preferably with a 7.0 -> 7.x upgrade path test)?Comment #29
xjmComment #30
webchickThat sounds like needs work.
Comment #31
BerdirUpgrade 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.
Comment #33
BerdirStrange random test failure:
Comment #34
Berdir#31: date-format-case-1012620-31.patch queued for re-testing.
Comment #35
xjmThis 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:
I'd change these to:
// Add a new date format that is identical except for its case.
(If that's correct in context).
I'd change this to:
// Confirm that date formats are case-sensitive.
Comment #36
BerdirAdded an assert and updated the comments. Removed the second comment from upgrade.test, which I think is unecessary.
Comment #37
BerdirOk, also took care of the unique key while changing that field.
Comment #38
xjmThanks, looks done to me!
Comment #39
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/297665a