CachedStorage::readMultiple uses array_filter instead of strict comparisons

Thanks, I'm uploading a patch just to see how much stuff it breaks if we replace it with a strict comparison instead of array_filter. It still doesn't fix the core issue, but reworking how to handle missing configuration will probably break a whole lot more. At least now it should only "mess" with configuration where the value is boolean FALSE, and in that case it'll still return boolean FALSE so.

It's come to my attention in the issue in config_filter, that FileStorage also filters its results in ::readMultiple and doesn't in ::read. So perhaps the fix is to apply the same filtering to DatabaseStorage::readMultiple (and any other Storage class that doesn't already) instead of removing it.

I'll try that locally tomorrow to see if that also fixes the problem (I believe it will).

We just need to introduce some consistency and we'll be alright I think.

Okay, so I took a look at the StorageInterface docblock and all implementations of it. It's not perfect.

StorageInterface definitions:

  /**
   * ...
   *
   * @return array|bool
   *   The configuration data stored for the configuration object name. If no
   *   configuration data exists for the given name, FALSE is returned.
   */
  public function read($name);

In short: if it exists in any form, return it. If not, return FALSE.

  /**
   * ...
   *
   * @return array
   *   A list of the configuration data stored for the configuration object name
   *   that could be loaded for the passed list of names.
   */
  public function readMultiple(array $names);

In short: if it exists in any form, make it a part of the returned list. If not, omit it from said list.

Let's see how each implementation of this interface implements these methods.

DatabaseStorage

DatabaseStorage::readMultiple

If it is found, it is unserialized and added to the return list. Empty string, empty array, anything.
If it is not found, it is omitted.
Pass

DatabaseStorage::read

If it is found, it is unserialized and returned. Empty string, empty array, anything.
If it is not found, FALSE is returned.
Pass

FileStorage

FileStorage::readMultiple

Uses the output from FileStorage::read in a loop, with a truthy check. Currently, because of the PHP bug mentioned below, this doesn't matter. Once that is fixed, this check will filter out valid yet empty configuration data.
Fail

FileStorage::read

If it is found, it is returned. Currently PHP doesn't treat empty YAML files (only comments for example) as valid. But the spec says they are, so once PHP fixes that this method will also return whatever it finds.
If the file can't be found, FALSE is returned.
Pass

InstallStorage

Extends FileStorage.

ExtensionInstallStorage

Extends InstallStorage.

TestInstallStorage

Extends InstallStorage.

NullStorage

NullStorage::readMultiple

Always returns an empty array, indicating no configuration is found.
Pass

NullStorage::read

Always returns an empty array, indicating empty configuration is found.
Fail

StorageReplaceDataWrapper

StorageReplaceDataWrapper::readMultiple

Wraps other Storages, just like CachedStorage. But because it doesn't cache missing configuration, it returns exactly whatever the wrapped storage gives it. And the check to see if it is cached is an isset(), not empty().
Can also wrap itself without issues.
Pass

StorageReplaceDataWrapper::read

Checks if it has it in its own cache with an isset() and either returns that or passes through to the wrapped storage.
Pass

CachedStorage

CachedStorage::readMultiple

Reads from wrapped storage, sets missing configuration (which isn't returned from valid Storages' readMultiple) to FALSE. Caches everything. Uses array_filter afterwards, removing falsy configurations from its return.
Can also not wrap itself due to this. On subsequent readMultiples it pollutes the cache with FALSEs where empty values should be, influencing the result from ::read.
Fail

CachedStorage::read

Reads from wrapped storage, unless it's in cache. Returns whatever it finds wherever it finds it, and caches it too.
Pass

Tomorrow (or soon anyway) I'll write two patches. One which brings everything in line with the current Interface definitions. And one which changes the interface definition to equate empty config with missing config, and then brings everything in line with that definition. See which one breaks less things. I'm hoping both will pass. Then there's further research: I checked all of Core's Storages. But there's Contrib too, and we need to see if this will impact them as well. There's a temporary (incorrect, array_filter-based) patch committed to config_filter so config_ignore can filter language Collections properly (which is full of empty config), which will be patched correctly if and when one of these fixes goes in. But there could be more implementations out there. And they may be wrong, or they may be right. So we'll need a, uh, Environmental Impact Report, if you will.

Then there's a choice to be made. Which solution will make it in? Or perhaps there's another solution out there I've missed. I'm just hoping a solution makes it in, clearly the current state is not ideal.

@borisson_: Sure thing.

Lost track of time, but here's the patch which fixes the implementations.