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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Issue tags: +Needs backport to D7

This must be backported to D7.

Dave Reid’s picture

Issue tags: +sprint, +Media Initiative

Also adding to the Media initiative sprint task list.

Dave Reid’s picture

drumm’s picture

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

Dave Reid’s picture

The 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?

Dave Reid’s picture

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

iamEAP’s picture

In what cases would you want the file name and Uri to be different?

Dave Reid’s picture

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

Dave Reid’s picture

Also, 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.

marvil07’s picture

Status: Active » Needs review

Patch makes sense :-), changing status, not sure about the policy on when to return a string on hook_update_N functions, so skipping RTBC.

Status: Needs review » Needs work
Issue tags: -Needs backport to D7, -sprint, -Media Initiative

The last submitted patch, 1691438-file-managed-revert-filename-binary.patch, failed testing.

The last submitted patch, 1691438-file-managed-revert-filename-binary.patch, failed testing.

aaron’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review

Changing the version temporarily so the patch can be tested.

aaron’s picture

drumm’s picture

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

mysql> SELECT count(filename), count(DISTINCT filename), count(DISTINCT BINARY filename) FROM files\G
*************************** 1. row ***************************
                count(filename): 505528
       count(DISTINCT filename): 430648
count(DISTINCT BINARY filename): 432358
crashtest_’s picture

Version: 7.x-dev » 8.x-dev
Assigned: Unassigned » crashtest_
FileSize
820 bytes

Adding patch for Drupal 8 to replicate the Drupal 7 patch.

Dave Reid’s picture

Status: Needs review » Needs work

Ack, we'll need to ensure that the definition in system_schema() is also fixed too.

quicksketch’s picture

I'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.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
1.06 KB

Rerolled for new update number and including the line removal from the schema definition.

Dave Reid’s picture

Assigned: crashtest_ » Unassigned
jbrown’s picture

Status: Needs review » Needs work
Issue tags: +Needs backport to D7, +sprint, +Media Initiative

The last submitted patch, file-managed-file-revert-1691438.patch, failed testing.

jbrown’s picture

Status: Needs work » Needs review
FileSize
1.17 KB
Dave Reid’s picture

Status: Needs review » Reviewed & tested by the community

Great! Tested the update locally so RTBC pending bot approval.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs backport to D7, -sprint, -Media Initiative

The last submitted patch, file-managed-file-revert-1691438.patch, failed testing.

jbrown’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D7, +sprint, +Media Initiative
Berdir’s picture

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

slashrsm’s picture

Status: Needs review » Reviewed & tested by the community

Tests passed, looks good patch and already previously marked RTBC.

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Ok, 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.

Devin Carlson’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.04 KB

D7 patch.

slashrsm’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine.....

webchick’s picture

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

Great, 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.

coredumperror’s picture

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

Devin Carlson’s picture

Status: Needs work » Needs review
FileSize
785 bytes

A patch to remove the update function.

quicksketch’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

Status: Reviewed & tested by the community » Fixed

UPDATE FUNCTION: VAPORIZED.

Awesome, thanks a ton. Committed and pushed to 8.x. Happy to see this sucker marked fixed. :D

quicksketch’s picture

:)

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

mgpcreative’s picture

The last submitted patch, 20: file-managed-file-revert-1691438.patch, failed testing.

Gábor Hojtsy’s picture

Issue summary: View changes
Issue tags: -Media Initiative +D8Media

Fix media tag.