Problem/Motivation

Unit tests would like to disable all potential side effects. That means FileCacheFactory::get() should always return a NullFileCache class.

They do so by calling:

FileCacheFactory::setConfiguration(['default' => ['class' => NullFileCache::class]]);

That however will not be reliable in case a backend specified a different class itself.

Proposed resolution

- Add:

$conf['file_cache']['file_cache_disable'] = TRUE;

or

FileCacheFactory::setConfiguration(['file_cache_disable' => TRUE]]);

which acts like a global kill switch for the FileCache and will it have to use the NullFileCache directly.

Remaining tasks

- Discuss
- Do it
- Write unit tests to test it

CommentFileSizeAuthor
#10 provide_a_setting_to-2756307-10.patch3.99 KBFabianx
#10 interdiff.txt1.32 KBFabianx
#7 provide_a_setting_to-2756307-7.patch3.46 KBFabianx
#7 interdiff.txt2.74 KBFabianx
#5 provide_a_setting_to-2756307-5.patch3.15 KBFabianx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Fabianx created an issue. See original summary.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer commented
Title: Provide a setting to disable file_cache completely for unit tests » Provide a setting to disable FileCache completely for unit tests
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

Option a) seems to be a good approach. Sadly we cannot avoid that classes specify their own default backends.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer commented
Issue summary: View changes
Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented
Status: Active » Needs review
FileSize
provide_a_setting_to-2756307-5.patch3.15 KB

Here is the patch.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Issue tags: -Needs tests, -Needs change record

Added a really quick CR, we have tests already.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented
FileSize
interdiff.txt2.74 KB
provide_a_setting_to-2756307-7.patch3.46 KB

Added a FileCacheFactory::DISABLE_CACHE constant per idea by dawehner. (Much better!)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Thank you @Fabianx!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/tests/Drupal/Tests/Component/FileCache/FileCacheFactoryTest.php
@@ -28,6 +30,9 @@ protected function setUp() {
+    // Remove the disable cache flag.
+    unset($configuration[FileCacheFactory::DISABLE_CACHE]);
+

This unset looks really strange. I think it would be best to make setUp() like this:

  protected function setUp() {
    parent::setUp();

    $configuration = [
      'test_foo_settings' => [
        'collection' => 'test-23',
        'cache_backend_class' => '\Drupal\Tests\Component\FileCache\StaticFileCacheBackend',
        'cache_backend_configuration' => [
          'bin' => 'dog',
        ],
      ],
    ];
    FileCacheFactory::setConfiguration($configuration);
    FileCacheFactory::setPrefix('prefix');
  }
Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented
Status: Needs work » Reviewed & tested by the community
FileSize
interdiff.txt1.32 KB
provide_a_setting_to-2756307-10.patch3.99 KB

I agree.

This was pre-existing before UnitTestCase disabled the FileCache, where we wanted to keep the current configuration to avoid leaking into global state.

Fixed in attached patch!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed 57ad987 and pushed to 8.2.x. Thanks!

  • alexpott committed 57ad987 on 8.2.x 
    Issue #2756307 by Fabianx, dawehner: Provide a setting to disable...

Status: Fixed » Closed (fixed)

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