Problem/Motivation

The decision tree of CacheFactory::get() looks like this:

1) Use cache bin specific backend configuration if available
2) Use configured default cache backend
3) Use cache bin default backend
4) Database backend.

The fact that it uses the default cache backend (2) before the cache bin default backend (3) causes problems when setting a default cache backend, e.g. redis:

a) For example cache.static suddenly uses redis and persists and is no longer a static/memory cache.
b) discovery/config/bootstrap no longer use the fast chained backend by default but go directly to redis.

This can result in unexpected behavior and performance regressions when using redis, as you get a lot more cache hits against redis for the fast chained backends.

Proposed resolution

Switch the order of 2 and 3. That's a behavior change, but I think it makes sense and based on how core uses the default cache backend, it expects that this wins unless someone *explicitly* overrides it.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#9 cache-factory-get-order-2753989-9.patch3.54 KBBerdir
#9 cache-factory-get-order-2753989-9-test-only.patch2.33 KBBerdir
#2 cache-factory-get-order-2753989-2.patch1.21 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH for Acquia commented
Status: Active » Needs review
FileSize
cache-factory-get-order-2753989-2.patch1.21 KB

And here is a patch.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer commented
Status: Needs review » Reviewed & tested by the community

RTBC - Fully agree, I just had the same problem with FileCache, too.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer commented
Issue tags: +Needs change record
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH for Acquia commented

Thanks, will create a change record. Not sure if we need some (unit) tests to enforce this order?

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer commented

#5: I think some unit tests would be great to have here. Someone writing a test would probably have found the behavior as it would be odd writing a test for the wrong thing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Reviewed & tested by the community » Needs work

Ok, setting back to needs work for that.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer commented
Issue tags: +Needs tests
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
cache-factory-get-order-2753989-9-test-only.patch2.33 KB
cache-factory-get-order-2753989-9.patch3.54 KB

Ok, improved the tests. We had the three other cases tested but only always on their own. This also tests the per-bin default and makes sure the correct order is used.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented
Status: Needs review » Reviewed & tested by the community

RTBC - Nice work!

The last submitted patch, 9: cache-factory-get-order-2753989-9-test-only.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: cache-factory-get-order-2753989-9.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Those are really weird test fails...

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Test fails that the patch had:

Views.Drupal\Tests\views\Kernel\Plugin\RelationshipTest
✗	
testRelationshipQuery
fail: [Other] Line 37 of core/modules/views/tests/src/Kernel/Plugin/RelationshipTest.php:
Drupal\Tests\views\Kernel\Plugin\RelationshipTest::testRelationshipQuery
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'jenkins_default_158231.simpletest596117views_test_data' doesn't exist: UPDATE {views_test_data} SET uid = 1 WHERE id = 1; Array
(
)


/var/www/html/core/lib/Drupal/Core/Database/Connection.php:671
/var/www/html/core/lib/Drupal/Core/Database/Connection.php:635
/var/www/html/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php:81
/var/www/html/core/includes/database.inc:61
/var/www/html/core/modules/views/tests/src/Kernel/Plugin/RelationshipTest.php:39

Views.Drupal\Tests\views\Kernel\Plugin\StyleHtmlListTest
✗	
testDefaultRowClasses
fail: [Other] Line 26 of core/modules/views/tests/src/Kernel/Plugin/StyleHtmlListTest.php:
Drupal\Tests\views\Kernel\Plugin\StyleHtmlListTest::testDefaultRowClasses
Drupal\Core\Database\SchemaObjectExistsException: Table sequences already exists.

/var/www/html/core/lib/Drupal/Core/Database/Schema.php:592
/var/www/html/core/tests/Drupal/KernelTests/KernelTestBase.php:818
/var/www/html/core/modules/views/tests/src/Kernel/ViewsKernelTestBase.php:44
✗	
rupal\Tests\views\Kernel\Plugin\StyleHtmlListTe
fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-1983.xml:
PHPunit Test failed to complete
✗	
nkno
fail: [run-tests.sh check] Line 0 of :
FATAL Drupal\Tests\views\Kernel\Plugin\StyleHtmlListTest: test runner returned a non-zero error code (2).
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record, -Needs tests

Change record created: https://www.drupal.org/node/2754947

Was RTBC before failing with those fails which I assume are random, it also passed already for PHP 7.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +DrupalWTF

Seems completely sensible!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

Wow, yeah that is quite of a WTF. +1
I checked similar subsystems, queue, lock KV to see whether they contain similar code, they don't.

  • catch committed b85b73c on 8.2.x 
    Issue #2753989 by Berdir: CacheFactory::get() should not use default...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed 
+++ b/core/tests/Drupal/Tests/Core/Cache/CacheFactoryTest.php
@@ -70,20 +70,64 @@ public function testCacheFactoryWithCustomizedDefaultBackend() {
+    // and per-bin default.s

Fixed on commit.

Committed/pushed to 8.2.x. Leaving this fixed against that branch, since there's a tiny chance that an 8.1.x site is relying on the current behaviour (like dead code in settings.php which this would re-activate).

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented

Published the change record.

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2820580: Drupal >=8.2.x doesn't allow to override all cache bins with $settings['cache']['default'] anymore, documentation says otherwise

This failed to update documentation. Which caused a new bug report to be filed: #2820580: Drupal >=8.2.x doesn't allow to override all cache bins with $settings['cache']['default'] anymore, documentation says otherwise.