FilteredStorage::readMultiple needs breaking to be in line with core

Good news, the patch for Core also doesn't break any existing tests. I'd much prefer getting the core patch in, so postpone this one?

Wait. But. I'm trying to get it into Core, and if the fix gets into core, this patch will have to go. Or at least be the same as in Core: a strict boolean FALSE comparison instead of the falsy array_filter.

An empty array, essentially anything except FALSE, means the config was found and loaded. Even FALSE might be a hit, although removing those is fine because CachedStorage will save it as FALSE in cache anyway.

DatabaseStorage doesn't have the same check in its readMultiple, unlike FileStorage. As you can see, it's quite a mess down there in general.

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Config%21...

DatabaseStorage returns what it finds, even if it finds FALSE, or NULL, or an empty string. Only when it finds nothing, does it set the data to FALSE explicitly. But if it finds anything it'll return that (disregarding the issues with unserializing corrupt data and the like for the moment), even the boolean FALSE.

CachedStorage::readMultiple respects this in that it caches all those results, but then filters them before returning. Yet CachedStorage's read method does *not* filter anything at all. readMultiple can return nothing if you ask it for, say, ['webform.settings'] in the Dutch language collection because it array_filters it out. But ask the same of read, and you'll get an empty array.

The problem for Config Filter is in the fact that CachedStorage wraps itself again before calling DatabaseStorage. Specifically the fact that the following code is run twice. Once with a list fresh from DatabaseStorage, containing entries with empties, and again with a list filtered already by the internal CachedStorage instance where those empties are now entirely missing from the list.

    // Cache configuration objects that were loaded from the storage, cache
    // missing configuration objects as an explicit FALSE.
    $items = [];
    foreach ($names_to_get as $name) {
      $data = isset($list[$name]) ? $list[$name] : FALSE;
      $data_to_return[$name] = $data;
      $items[$cache_keys_map[$name]] = [
        'data' => $data,
      ];
    }
    $this->cache->setMultiple($items);

The first time it runs, with data from DatabaseStorage, configuration that was found, even if the data itself is only an empty string, is saved to the cache. But the second time it runs, that list has been passed through array_filter, and so everything that is now incorrectly treated as "missing" is set to boolean FALSE. And then *that* is saved to cache. And that cache is what CachedStorage::read will read from.

Even looking at the comments in CachedStorage: the intent is to cache missing configuration as FALSE. And below that is a comment saying it does not want to return missing configuration, but it doesn't explicitly check for FALSE, it uses the loose array_filter.

Target storage setup:
CachedStorage::readMultiple -> CachedStorage::readMultiple -> DatabaseStorage::readMultiple
Source storage setup with filters:
CachedStorage::readMultiple -> FilteredStorage::readMultiple -> ... -> DatabaseStorage::readMultiple (in the case of a hit for a Config Ignore filter)

So, in my mind, the correct fix is to either tighten the filter in CachedStorage so non-boolean FALSE isn't treated as a configuration miss, or change all the other Storage solutions (Database, Filtered, etc) so they do filter their results the same way. Which is what the current patch does, just making it work the same way as CachedStorage.

The fact that FileStorage does filter it as well muddies the waters somewhat. I didn't realize that. I guess either approach would be fine, as long as they all do the same thing. The key is that there needs to be consistency across the board, that's what caused the issue.

But, like you said, in the interim this patch does fix those issues.