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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

martin107’s picture

Priority: Normal » Critical

This 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 )

catch’s picture

martin107’s picture

Plucking a quote from #333054: Make all varchar columns case sensitive (regression) (was make page cache case sensitive) regarding the case-sensitivity of cid's

this has been the status quo for the entire lifetime of both current releases of Drupal.

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

CaseSensitiveBackend extends CacheBackendInterface {

Ugh .. I just thought I should mention it. We are in beta and losing momentum generally.

catch’s picture

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

mikeytown2’s picture

Query needs to look like this from very limited testing in D7.

SELECT cid, data, created, expire, serialized 
FROM cache_page
WHERE cid COLLATE utf8_bin = 'user'

Better would be to change the column; output from phpmyadmin so I'm sure it can be simplified.

ALTER TABLE `cache_page` CHANGE `cid` `cid` VARCHAR(255) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL DEFAULT '' COMMENT 'Primary Key: Unique cache ID.';
Berdir’s picture

Status: Active » Needs review
FileSize
1.33 KB
849 bytes

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

Status: Needs review » Needs work

The last submitted patch, 7: cache-case-sensitive-2352207-7-test-only.patch, failed testing.

mikeytown2’s picture

Status: Needs work » Needs review

Looks like Berdir's patch in #7 works with the given test. +1 from me on this.

martin107’s picture

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

catch’s picture

Status: Needs review » Needs work
Issue tags: +D8 upgrade path
+++ b/core/lib/Drupal/Core/Cache/CacheBackendInterface.php
@@ -14,6 +14,9 @@
+ * cid's 'APPLE' and 'apple' are seperate and distinct.

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

martin107’s picture

Status: Needs work » Needs review
FileSize
1.84 KB
617 bytes

Ok, less is more.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks great!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed 40a7440 on 8.0.x
    Issue #2352207 by martin107, Berdir: Database cache backend does not...

Status: Fixed » Closed (fixed)

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

mikeytown2’s picture

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