Problem/Motivation

The page cache id could be long, as the URL might be over 256 characters which were previously possible with the database cache backend.

This was however changed, the cid is automatically and more intelligently (the first part of the cid is kept) hashed.

Proposed resolution

Remove the call to sha1().

Advantage:
- We save a sha1() for all urls + content type < 256 character
Debugging the page cache is easier

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#5 remove-explicit-sha-2386255-3.patch3.84 KBaneek
#5 interdiff-2386255-2-3.txt3.41 KBaneek
#2 2386255-2.patch435 bytesrpayanm

Comments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +php-novice, +D8 cacheability, +DX (Developer Experience)
rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Active » Needs review
StatusFileSize
new 2386255-2.patch435 bytes

Let's me try :)

Status: Needs review » Needs work

The last submitted patch, 2: 2386255-2.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

PageCacheTagsTestBase has a sha1() call too, removing that should fix all the CacheTagsTests. BlockTest might have one too.

aneek’s picture

aneek commented
StatusFileSize
new interdiff-2386255-2-3.txt3.41 KB
new remove-explicit-sha-2386255-3.patch3.84 KB

@rpayanm,
There are more files that depends on removal of SHA1 as suggested by @Berdir.

/Core/modules/block/src/Tests/BlockTest.php
/Core/modules/system/src/Tests/Bootstrap/PageCacheTest.php
/Core/modules/system/src/Tests/Cache/PageCacheTagsIntegrationTest.php
/Core/modules/system/src/Tests/Cache/PageCacheTagsTestBase.php

Just uploading a patch removing these. Please fix if I've missed something.

aneek’s picture

aneek commented
Status: Needs work » Needs review
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Looks great, thanks!

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented

@Berdir @aneek Thank you for you explanation :)

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 6414318 on 8.0.x 
    Issue #2386255 by aneek, rpayanm: Remove explicit sha1() call in...

Status: Fixed » Closed (fixed)

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