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.
Updated: Comment #N
Problem/Motivation
The CacheFactory
permits overriding of the cache backend service on a per-bin basis from within settings.php
. However this is currently broken because CacheFactory::get
fails to access the proper key of the settings array.
Proposed resolution
Remaining tasks
None
User interface changes
None
API changes
None
Comment | File | Size | Author |
---|---|---|---|
#14 | 2233337-TEST-ONLY-fix-per-bin-cache-backend-service-14.diff | 3.83 KB | znerol |
#8 | 2233337-fix-per-bin-cache-backend-service-8.diff | 4.45 KB | znerol |
#8 | 2233337-TEST-ONLY-fix-per-bin-cache-backend-service-8.diff | 3.83 KB | znerol |
Comments
Comment #1
znerol CreditAttribution: znerol commentedComment #2
znerol CreditAttribution: znerol commentedComment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedyes, this is awesome.
Comment #4
znerol CreditAttribution: znerol commentedComment #6
BerdirLooks nice, some minor nitpicks...
Contains \Drupal\... ;)
Look at some of the new test classes, they use @coversDefaultClass + @covers on methods, we should do that her as well.
I think most tests use an actual ContainerBuilder instead of mocking it, that's easier I think.
It will also fail nicer (unknown service exception instead of fatal error)
Wondering if we should use something else than contrib here, memory maybe? or example?
Comment #7
BerdirComment #8
znerol CreditAttribution: znerol commentedThanks for the review @Berdir. Changed the name and comment for the test you mentioned in 4, I hope it is now clear what it is about.
Comment #10
BerdirStill missing a leading \, but can be fixed on commit...
Looks good, let's fix this.
Comment #11
dawehnerGreat work!
Comment #12
catchCommitted/pushed to 8.x, thanks!
Comment #14
znerol CreditAttribution: znerol commentedThanks @catch. However
CacheFactoryTest
seems to be missing from the commit. Attached the test-only patch, also addressing the remark from #10.Comment #15
catchArggh. 'fixed locally' yeah right...
Committed/pushed to 8.x, thanks!