Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
cid's that only differ in the case trigger a notice because they are found by the query but then the cid mapping fails.
To reproduce:
Enable by-url cache context for a block.
Then first go to yoursite?test=a and then yoursite?test=A. => This should give you a php notice. Make sure that display errors is enabled.
Proposed resolution
Add binary => TRUE to the cid column. Should also increase index performance?
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#12 | interdiff-10-12.txt | 617 bytes | martin107 |
#12 | cache-case-sensitive-2352207-12.patch | 1.84 KB | martin107 |
#10 | cache-case-sensitive-2352207-10.patch | 1.92 KB | martin107 |
#10 | interdiff-7-10.txt | 605 bytes | martin107 |
#7 | cache-case-sensitive-2352207-7-test-only.patch | 849 bytes | Berdir |
Comments
Comment #1
martin107 CreditAttribution: martin107 commentedThis issue will help resolve a problem in a critical issue...
#1965074: Add cache wrapper to the UrlGenerator
so I am bumping its priority and because, while the other issue has separate errors to resolve, once they are clear that issue should be postponed on this
See https://www.drupal.org/node/1965074#comment-9371727 for more details
The problem
In short we now have two issues which as part of the algorithms generating cache identifier ( cid's )
we want to include fragments of the URL and have separate caches for the following example URLs
/user ( for which the correct response is a 302 redirect )
/USER ( for which the correct response is a 404 error )
Comment #2
mikeytown2 CreditAttribution: mikeytown2 commentedhttp://stackoverflow.com/questions/4558707/case-sensitive-collation-in-m...
http://dev.mysql.com/doc/refman/5.6/en/charset-collate.html
http://dev.mysql.com/doc/refman/5.6/en/case-sensitivity.html
Comment #3
catchOlder issues:
#333054: Make all varchar columns case sensitive (regression) (was make page cache case sensitive) and #99970: Page cache matching is case insensitive.
Comment #4
martin107 CreditAttribution: martin107 commentedPlucking a quote from #333054: Make all varchar columns case sensitive (regression) (was make page cache case sensitive) regarding the case-sensitivity of cid's
So I understand that there will be resistance and backporting issues.....
I would like to move forward with a consensus in the community
To be clear I don't like or want to champion this idea ....
Now that we can take advantages of Interfaces. The path of minimum disruption ...where we special case the times where we need a case sensitive cache identifier and create a class something like
Ugh .. I just thought I should mention it. We are in beta and losing momentum generally.
Comment #5
catch#2075889: Make Drupal handle incoming paths in a case-insensitive fashion for routing is another older issue where this came up.
If we only make the cid column in the database cache case sensitive (i.e. change to binary), I'm personally fine with making that change.
If this is blocking a different critical issue, it's fine for it to be critical.
Comment #6
mikeytown2 CreditAttribution: mikeytown2 commentedQuery needs to look like this from very limited testing in D7.
Better would be to change the column; output from phpmyadmin so I'm sure it can be simplified.
Comment #7
BerdirOur schema API supports this we already use it in other cases like the file URI, as I wrote in the initial issue summary :)
On the actual bug, this is a plain and simple bug imho that needs to be fixed. The code assumes this is the case (there is a notice within the database backend, see test-only patch) and obviously, the other backends work the same way as well.
Comment #9
mikeytown2 CreditAttribution: mikeytown2 commentedLooks like Berdir's patch in #7 works with the given test. +1 from me on this.
Comment #10
martin107 CreditAttribution: martin107 commented+1 from me.
Mundane TRUEisms
a) The Routing and Block subsystems, code to CacheBackendInterfacre and assume case sensitive cid's
b) DatabaseBackend implements the CacheBackendInterface and ensures case sensitive cid's
So the CacheBackendInterface is king .... or at least that is where we should document this decision.
Comment #11
catchI think just 'Cache IDs are case sensitive.' is fine here.
If we need more explanation, "cid's" and 'seperate' both need fixing would also drop the "That is".
Patch looks fine. No update hook needed, but tagging upgrade path since that's only true until we start providing one.
Comment #12
martin107 CreditAttribution: martin107 commentedOk, less is more.
Comment #13
catchLooks great!
Comment #14
alexpottThis issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 40a7440 and pushed to 8.0.x. Thanks!
Tested this on postgres too - no problems. Of course postgres is already case-sensitive. So now MySQL and Postgress behave the same nice wrt to cache cids. Nice.
Comment #17
mikeytown2 CreditAttribution: mikeytown2 commentedJust a heads up that I was able to see around a 10-20% speed improvement by using ascii_bin instead of utf8_bin for the collation for most db queries. I know that most cid's are mainly ascii characters so this could speed things up. Maybe we transliterate all UTF8 chars into ascii on the cid column? I know this needs a new issue.