Problem/Motivation

In #2991683: Move configuration transformation API in \Drupal\Core\Config namespace @bircher discovered the \Drupal\Core\Config\FileStorage::getAllCollectionNames() behaves differently when the directory doesn't exist compared to other methods on the class. For example ::read() and ::listAll() work perfectly fine and we have a test for this - see \Drupal\KernelTests\Core\Config\Storage\FileStorageTest and \Drupal\KernelTests\Core\Config\Storage\ConfigStorageTestBase::testInvalidStorage()

Proposed resolution

If the directory does not exist return []

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#2 3001979-2.patch1.9 KBalexpott
#2 3001979-2.test-only.patch935 bytesalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: \Drupal\Core\Config\FileStorage::getAllCollectionNames should work when the directory does not exist » \Drupal\Core\Config\FileStorage::getAllCollectionNames() should work when the directory does not exist
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 3001979-2.test-only.patch935 bytes
new 3001979-2.patch1.9 KB

I debated whether the fix should go in \Drupal\Core\Config\FileStorage::getAllCollectionNamesHelper() but I'm thinking not.

The last submitted patch, 2: 3001979-2.test-only.patch, failed testing. View results

bircher’s picture

bircher
Location 🇨🇿
commented
Status: Needs review » Reviewed & tested by the community

I tested the patch with the Config Split tests and I realized that I don't have test coverage for different collections there.
The reason I mention it is because I am not sure how it fares with \org\bovigo\vfs\vfsStreamWrapper in tests.

But since here we use a real file system anyway it is ok.
Another alternative would have been to catch the exception in \Drupal\Core\Config\FileStorage::getAllCollectionNamesHelper() but the patch here is also a good solution.

I like that it is a straight forward fix which makes it consistent, so RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@bircher it should fare just fine - in KernelTestBase land the public file directory that this test uses is virtual :)

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/lib/Drupal/Core/Config/FileStorage.php
@@ -279,6 +279,9 @@ public function getCollectionName() {
+    if (!is_dir($this->directory)) {
+      return [];
+    }

Should we be providing some sort of feedback/logging that the directory is broken.

Silently gulping the error might end up with someone sinking a heap of time for no reason

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Reviewed & tested by the community

I think it's absolutely fine for this to be called in the situation before the config directory exists. See the testing around \Drupal\KernelTests\Core\Config\Storage\ConfigStorageTestBase::$invalidStorage - we do automatic directory creation on write.

If this directory does not exist then there are no collections.

  • larowlan committed de15d04 on 8.7.x 
    Issue #3001979 by alexpott: \Drupal\Core\Config\FileStorage::...

  • larowlan committed e09b77c on 8.6.x 
    Issue #3001979 by alexpott: \Drupal\Core\Config\FileStorage::...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Version: 8.7.x-dev » 8.6.x-dev
Status: Reviewed & tested by the community » Fixed

Committed de15d04 and pushed to 8.7.x. Thanks!

c/p as e09b77c346 and pushed to 8.6.x

Status: Fixed » Closed (fixed)

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