Updated: Comment #0

Problem/Motivation

We have two main config storage services, config.storage (for the active store) and config.storage.staging (for the staging/import).

In #2099363: Allow single config files to be imported and exported (Resolve regression from Views in Drupal 7) we have added a UI to allow quick importing and exporting of a single config file. This has highlighted a major problem of our assumptions about the active config: that it has the same format as staging.

All of our default config is written in YAML, and that is not going to change. However, our active config storage is swappable, and doesn't need to be YAML. Therefore, when showing an "export" of active config, we actually want to use the staging config service, so that what we present is importable.

This furthermore leads to the idea that our staging/import config should not be a service at all, and should be a hardcoded class. If we want to change it, we'll also have to change every bit of default config.

Proposed resolution

Make the encode/decode methods of active config protected, so that they are never called directly.
Replace the staging config service with a hardcoded class (FileStorage).

Remaining tasks

User interface changes

N/A

API changes

See resolutions

#2099363: Allow single config files to be imported and exported (Resolve regression from Views in Drupal 7)

Comments

beejeebus’s picture

yes, this. thanks timplunkett for opening the issue, and thanks chx for prodding me in IRC and putting up with me ranting.

i don't have anything useful to add to the summary right now, but i'll think on it.

tim.plunkett’s picture

Priority: Major » Critical

The compromise approach in #2099363-7: Allow single config files to be imported and exported (Resolve regression from Views in Drupal 7) didn't work, making this even more essential than before.

chx’s picture

> Replace the staging config service with a hardcoded class (FileStorage).

Yes... likely put a method for it on ConfigStorage.

Also, make encode/decode a static method and remove it from the Storage interface.

beejeebus’s picture

just bumping to it goes in front of all the string cast issues.

slashrsm’s picture

Also, make encode/decode a static method and remove it from the Storage interface.

Agree on that. There are backends that need no encoding. In MongoDB implementation we currently implement those two methotd simply just because they have to be there.

tim.plunkett’s picture

The export storage part is not a factor anymore, since the full and single exports are hardcoded to use Yaml::encode() now.
However, config.storage.staging is still used, and defined like this

  config.storage.staging:
    class: Drupal\Core\Config\FileStorage
    factory_class: Drupal\Core\Config\FileStorageFactory
    factory_method: getStaging

It is used (outside of tests) by ConfigImportForm, ConfigSync, and ConfigController::diff()

Do we want to hardcode that as FileStorage instead? Is there a way to lock down the service? Do we still care?

catch’s picture

Priority: Critical » Major

The export storage part is not a factor anymore, since the full and single exports are hardcoded to use Yaml::encode() now.

Yeah it was funny seeing the export UI produce nice serialized PHP when beejeebus was working on the db storage change :)
The export issue was a real, critical, bug since we actually wanted that to be swappable and it wasn't.

I think we should hard code the to FileStorage just to be clear, but also I don't think it's an API change so we could do it after release if we want.

alexpott’s picture

xjm’s picture