See #966210-113: DB Case Sensitivity: system_update_7061() fails on inserting files with same name but different case. In system_update_7037() the 'binary' => TRUE flag was added to the {file_managed}.filename schema column without any explanation or reasoning. This makes it impossible to have a Views filter on that column (which we use in the Media module to help users filter by file name, which should be case insensitive by default. But with the latest D7 core, it is no longer that way and it has raised issues with several clients that their filtered results do not return expected results anymore.
We should add another update hook that reverts this change back to a non-binary column.
Comments
Comment #1
Dave ReidThis must be backported to D7.
Comment #2
Dave ReidAlso adding to the Media initiative sprint task list.
Comment #3
Dave ReidHere's the D7 patch.
Comment #4
drummMy motivation to work on that change is that Drupal.org needed it, we have lots of filenames that vary only by case. This tells me that in the long run, it actually wasn't a "regression," something in Drupal 6 or maybe earlier allowed this to happen.
Changing this must keep the old urls working. An update function will have to change the conflicting filenames and add some sort of redirect so the old URL works.
Comment #5
Dave ReidThe only problem I read in that issue was problems with the 'uri' column, not filename. File URLs are based on the uri column, not filename.
@drumm could you maybe explain the exact problem or reasoning why we needed to change the 'filename' column?
Comment #6
Dave ReidTo be clear: I am completely ok and understand why {file_managed}.uri was converted to binary. That makes total sense to me. What does not make sense is why {file_managed}.filename was converted.
Comment #7
iamEAP CreditAttribution: iamEAP commentedIn what cases would you want the file name and Uri to be different?
Comment #8
Dave ReidYou don't. And changing the field to be binary or not does not change the contents at all. It only changes that you cannot by default do case-insensitive comparison.
Comment #9
Dave ReidAlso, instances where we have to rename a file if a file with the same URI already exists, Drupal core keeps the filename the same as the original, but URL refers to the renamed file.
Comment #10
marvil07 CreditAttribution: marvil07 commentedPatch makes sense :-), changing status, not sure about the policy on when to return a string on hook_update_N functions, so skipping RTBC.
Comment #13
aaron CreditAttribution: aaron commentedChanging the version temporarily so the patch can be tested.
Comment #14
aaron CreditAttribution: aaron commented#3: 1691438-file-managed-revert-filename-binary.patch queued for re-testing.
Comment #15
drummI was thinking of the uri column, which has a unique key, and does need to be binary. This filename doesn't have any unique keys, so I guess it is fine.
BTW, Drupal.org has:
Comment #17
crashtest_ CreditAttribution: crashtest_ commentedAdding patch for Drupal 8 to replicate the Drupal 7 patch.
Comment #18
Dave ReidAck, we'll need to ensure that the definition in system_schema() is also fixed too.
Comment #19
quicksketchI'm also a big +1 for rolling back the changes to filename. It's causing issues with the autocomplete widget in FileField Sources over here: #1691438: Regression: {file_managed}.filename should NOT be binary (and case-sensitive by default). I imagine it's the same problem Dave is having in Media, that users now have to know the case of their files in order to find them, unless we introduce a dreaded LOWER() wrapper in the query *shudder*. It would be a huge step backward after all the work in #279851: Replace LOWER() with db_select() and LIKE() where possible did to remove them. As far as I can tell, using the solution from that issue (replacing "= LOWER()" essentially with a LIKE statement) isn't going to work on these new binary columns.
Comment #20
quicksketchRerolled for new update number and including the line removal from the schema definition.
Comment #21
Dave ReidComment #22
jbrown CreditAttribution: jbrown commented#20: file-managed-file-revert-1691438.patch queued for re-testing.
Comment #24
jbrown CreditAttribution: jbrown commentedComment #25
Dave ReidGreat! Tested the update locally so RTBC pending bot approval.
Comment #27
jbrown CreditAttribution: jbrown commented#24: file-managed-file-revert-1691438.patch queued for re-testing.
Comment #28
BerdirSame here, this will conflict with #1468328: Move file entity info, managed file, and file usage functionality into File module and I'd be kinda sad to have to re-roll that one again :)
Comment #29
slashrsm CreditAttribution: slashrsm commentedTests passed, looks good patch and already previously marked RTBC.
Comment #30
webchickOk, gotcha. So we were a little over-zealous with our fix at #966210: DB Case Sensitivity: system_update_7061() fails on inserting files with same name but different case. Sorry about that.
Looks like everyone here, including drumm, is on the same page about the fix, so committed and pushed to 8.x. Moving down to 7.x for backport. Once that's done, we'll probably have to move back to 8.x again to remove the update function.
Comment #31
Devin Carlson CreditAttribution: Devin Carlson commentedD7 patch.
Comment #32
slashrsm CreditAttribution: slashrsm commentedLooks fine.....
Comment #33
webchickGreat, thank you!
Committed and pushed to 7.x, now back to 8.x to get rid of the update function. I think we can reduce the priority of this issue now, since this is just clean-up.
Comment #34
coredumperror CreditAttribution: coredumperror commentedThank you so much for fixing this problem! My users were getting pretty antsy about file search being case sensitive, and I had no idea how to help them.
Comment #35
Devin Carlson CreditAttribution: Devin Carlson commentedA patch to remove the update function.
Comment #36
quicksketchI thought our practice was to leave the update function but replace it with a comment. But checking our current approach, we just vaporize the entire update. That's why we don't have system_update_7010() or system_update_7019(). Patch looks complete and prevents D8 upgrades from re-regressing when updating from D7.
Comment #37
webchickUPDATE FUNCTION: VAPORIZED.
Awesome, thanks a ton. Committed and pushed to 8.x. Happy to see this sucker marked fixed. :D
Comment #38
quicksketch:)
Comment #40
mgpcreative CreditAttribution: mgpcreative commented20: file-managed-file-revert-1691438.patch queued for re-testing.
Comment #42
Gábor HojtsyFix media tag.