Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
system.module
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
18 Jul 2012 at 15:18 UTC
Updated:
10 Aug 2016 at 14:30 UTC
Jump to comment: Most recent, Most recent file
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 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 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 commentedChanging the version temporarily so the patch can be tested.
Comment #14
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_ 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 commented#20: file-managed-file-revert-1691438.patch queued for re-testing.
Comment #24
jbrown commentedComment #25
dave reidGreat! Tested the update locally so RTBC pending bot approval.
Comment #27
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 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 commentedD7 patch.
Comment #32
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 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 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 commented20: file-managed-file-revert-1691438.patch queued for re-testing.
Comment #42
gábor hojtsyFix media tag.