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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

znerol’s picture

Status: Active » Needs review
FileSize
636 bytes
znerol’s picture

Assigned: Unassigned » znerol
Issue tags: +Needs tests
Anonymous’s picture

yes, this is awesome.

znerol’s picture

The last submitted patch, 4: 2233337-TEST-ONLY-fix-per-bin-cache-backend-service-3.diff, failed testing.

Berdir’s picture

Looks nice, some minor nitpicks...

  1. +++ b/core/tests/Drupal/Tests/Core/Cache/CacheFactoryTest.php
    @@ -0,0 +1,120 @@
    + * Definition of Drupal\Tests\Core\Cache\CacheFactoryTest.
    

    Contains \Drupal\... ;)

  2. +++ b/core/tests/Drupal/Tests/Core/Cache/CacheFactoryTest.php
    @@ -0,0 +1,120 @@
    +/**
    + * Tests the cache CacheFactory.
    + *
    + * @group Cache
    + */
    +class CacheFactoryTest extends UnitTestCase {
    

    Look at some of the new test classes, they use @coversDefaultClass + @covers on methods, we should do that her as well.

  3. +++ b/core/tests/Drupal/Tests/Core/Cache/CacheFactoryTest.php
    @@ -0,0 +1,120 @@
    +    $container->expects($this->once())
    +      ->method('get')
    +      ->with('cache.backend.database')
    +      ->will($this->returnValue($builtin_default_backend_factory));
    

    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)

  4. +++ b/core/tests/Drupal/Tests/Core/Cache/CacheFactoryTest.php
    @@ -0,0 +1,120 @@
    +   * Test that the cache factory falls back to default contrib service.
    

    Wondering if we should use something else than contrib here, memory maybe? or example?

Berdir’s picture

Status: Needs review » Needs work
znerol’s picture

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

The last submitted patch, 8: 2233337-TEST-ONLY-fix-per-bin-cache-backend-service-8.diff, failed testing.

Berdir’s picture

Assigned: znerol » Unassigned
Status: Needs review » Reviewed & tested by the community
+++ b/core/tests/Drupal/Tests/Core/Cache/CacheFactoryTest.php
@@ -0,0 +1,123 @@
+/**
+ * @file
+ * Contains Drupal\Tests\Core\Cache\CacheFactoryTest.
+ */

Still missing a leading \, but can be fixed on commit...

Looks good, let's fix this.

dawehner’s picture

Great work!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

  • Commit 8e29b0f on 8.x by catch:
    Issue #2233337 by znerol: Impossible to specify per-bin cache backend...
znerol’s picture

Status: Fixed » Reviewed & tested by the community
FileSize
3.83 KB

Thanks @catch. However CacheFactoryTest seems to be missing from the commit. Attached the test-only patch, also addressing the remark from #10.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Arggh. 'fixed locally' yeah right...

Committed/pushed to 8.x, thanks!

  • Commit eca1d75 on 8.x by catch:
    Issue #2233337 by znerol: Impossible to specify per-bin cache backend...

Status: Fixed » Closed (fixed)

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