Move configuration transformation API in \Drupal\Core\Config namespace

+++ b/core/lib/Drupal/Core/Config/ConfigEvents.php
@@ -143,4 +143,39 @@
+   * Name of the event fired when the config storage transformer does an import.
...
+   * This event allows modules to modify the configuration which is about to be
+   * imported.
...
+   * The event listener method receives a
+   * \Drupal\Core\Config\StorageTransformImportEvent instance.
...
+  const STORAGE_TRANSFORM_EXPORT = 'config.transformer.export';

Just a fly-by review. Should this read export?

Leaving at NR for other feedback.

I've worked extensively in contrib with the plugin type provided by Config Filter. It does the trick, but I have run up against limitations.

Conceptually, modelling a config alter API through events and event subscribers does indeed seem like a big improvement. The proposed event approach looks both a lot simpler than the current Config Filter plugin API and also more flexible. Nice!

Adding #2960941: Provide an API for persistent configuration alters, including for extension-provided config as a related issue. I'm thinking here that ideally a configuration transformation API would also apply at extension install time, allowing just-installed extensions to transform config in the active storage (config which may have been provided by previously-installed extensions). For that type of transformation, at least, there are existing instances in core that could be upgraded to a new API, such as various config alters that currently are done programmatically in standard_install().

+++ b/core/lib/Drupal/Core/Config/ConfigExporter.php
@@ -0,0 +1,101 @@
+  public static function copyConfig(StorageInterface $source, StorageInterface $target) {

Some of this method closely parallels ConfigManager::createSnapshot(). Could some of it be abstracted out into a trait or utility class?

+++ b/core/lib/Drupal/Core/Config/StorageTransformer.php
@@ -0,0 +1,89 @@
+    $mutable = new DatabaseStorage($this->connection, 'config_transfomer_import');

Typo in the table name. Ditto for 'config_transfomer_export'.

It looks like we're using this as a way to have a storage used just while we're doing the transform? If so, in a similar situation in contrib, I used a custom config storage class, InMemoryStorage. Not saying that's necessarily better, just noting it as an option. I'm not immediately seeing how we ensure that these storages are emptied prior to a subsequent use. Did you consider adding a service for each of these storages?

I'm not immediately seeing how we ensure that these storages are emptied prior to a subsequent use.

Oh, I think I see, we empty it out on the next run in ::copyConfig(). Hmm, that works, but it leaves vestigial config stored after use. Feels preferable to delete it when we're done.

1. +++ b/core/core.services.yml
   @@ -306,6 +306,12 @@ services:
   +  config.exporter:
   +    class: Drupal\Core\Config\ConfigExporter
   +    arguments: ['@config.storage.sync', '@config.storage_transformer']
   

   I think this makes a lot of sense, but I think it would be a good idea to introduce this regardless of this issue. This would already be useful to Drush right now and I guess to Drupal Console, as well. Maybe we can try to utilize utilizing it from the export UI by creating an ArchiveStorage or something like that.

2. +++ b/core/lib/Drupal/Core/Config/ConfigEvents.php
   @@ -143,4 +143,39 @@
   +  const STORAGE_TRANSFORM_IMPORT = 'config.transformer.import';
   ...
   +  const STORAGE_TRANSFORM_EXPORT = 'config.transformer.export';
   

   Related to the above point I think we are missing a config.exporter.export event that gets invoked from the exporter directly - to match config.importer.import.

3. +++ b/core/lib/Drupal/Core/Config/ConfigExporter.php
   @@ -0,0 +1,101 @@
   +        $destination_collection->deleteAll();
   ...
   +      foreach ($source_collection->listAll() as $name) {
   +        $destination_collection->write($name, $source_collection->read($name));
   

   Playing around with the export a fair bit recently in the context of Config Overlay I had some issues with this approach, which is the same one currently used by Drush. What I did instead for testing Config Overlay is using the StorageComparer to compute the required delete/create/update operations and perform those directly. See here for an example. Note that Config Overlay is a) somewhat of a specific use-case and b) still explicitly alpha, so the fact that this fixed an issue I was having is not super significant in its own right, but thinking about it for a bit I actually find using the storage comparer a bit nicer. When you export with Drush, for example, it actually tells you what it's going to do and it computes that list using the storage comparer, but then it doesn't use it for the actual operations.

4. +++ b/core/lib/Drupal/Core/Config/StorageTransformExportEvent.php
   @@ -0,0 +1,67 @@
   +class StorageTransformExportEvent extends Event {
   
   +++ b/core/lib/Drupal/Core/Config/StorageTransformImportEvent.php
   @@ -0,0 +1,71 @@
   +class StorageTransformImportEvent extends Event {
   

   Not sure if I'm missing something, but couldn't this be collapsed into a StorageTransformEvent

   Edit: Ahh I see you answered this in #3. Sure, not a big deal either way...

Thanks for the feedback. You're explanation for #3 makes sense in that it shouldn't be a problem with the proposed API. However, I still think it's strange that we are using the storage comparer to preview a list of changes and then not actually use the storage comparer to perform those changes. But I guess that can be discussed independently of this issue...

Opened #2992763: Add a configuration exporter for the exporter issue.

You've been busy!

I've given 2991683-11-transformer-only.patch a quick first pass. Overall, it looks straightforward. It closely mirrors existing config events.

A few comments and suggestions:

1. +++ b/core/lib/Drupal/Core/Config/CachedStorage.php
   @@ -254,6 +254,15 @@ public function getCollectionName() {
   +   * Get the decorated storage.
   

   Should be "Gets" rather than "Get".

2. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
   @@ -61,13 +61,6 @@ class ConfigManager implements ConfigManagerInterface {
   -  protected $storages;
   

   Nice cleanup.

3. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
   @@ -165,28 +158,58 @@ public function diff(StorageInterface $source_storage, StorageInterface $target_
   +  public static function copyConfig(StorageInterface $source, StorageInterface $target) {
   

   Because we don't have this public method in the interface and it's too late to add it for this major version, we may need to move this to a new (utility?) class.

4. +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -312,6 +312,13 @@ public function getAllCollectionNames() {
   +    if (!file_exists($directory)) {
   

   It seems awkward to bail at this point. Can we ensure the method is not called if the directory doesn't exist?

   What scenario can lead to a non-existent directory? Is this a bug fix, and if so, do we need to do it here?

5. +++ b/core/lib/Drupal/Core/Config/StorageTransformEvent.php
   @@ -0,0 +1,144 @@
   + * is being transformed. This allows
   

   Incomplete sentence.

6. +++ b/core/lib/Drupal/Core/Config/StorageTransformEvent.php
   @@ -0,0 +1,144 @@
   +   * This storage can be interacted with by event subscribers and will be
   +   * used instead of the original storage after all event subscribers have been
   +   * called.
   

   I don't immediately follow this comment. What is "the original storage"?

7. +++ b/core/lib/Drupal/Core/Config/StorageTransformEvent.php
   @@ -0,0 +1,144 @@
   +   * The transformation is for importing config.
   

   For parallelism with other existing isSomething() methods, this and the other new is methods could be "Checks whether the..." or "Whether the..."

8. +++ b/core/lib/Drupal/Core/Config/StorageTransformEvent.php
   @@ -0,0 +1,144 @@
   +   * @return bool
   

   Missing blank line above.

9. +++ b/core/lib/Drupal/Core/Config/StorageTransformEvent.php
   @@ -0,0 +1,144 @@
   +   * Event listeners may add context to the event.
   

   Should have a first line of "Adds a context to the event." or similar.

10. +++ b/core/lib/Drupal/Core/Config/StorageTransformEvent.php
    @@ -0,0 +1,144 @@
    +   * Get the event listeners.
    

    "Gets"

11. +++ b/core/lib/Drupal/Core/Config/StorageTransformerInterface.php
    @@ -0,0 +1,69 @@
    + * A storage transformer generates a configuration storage for either importing
    + * into the active storage with the ConfigImporter or exporting the active
    + * configuration to a secondary storage such as the sync storage.
    

    The existence of the generic ConfigEvents::STORAGE_TRANSFORM implies there may be additional use cases, so we should cover that here.

12. +++ b/core/lib/Drupal/Core/Config/TransformedStorageWrapper.php
    @@ -0,0 +1,137 @@
    +  public function getTransformerContexts() {
    

    Because we have this custom public method, we should implement a new interface that extends StorageInterface.

No real review yet - some coding standards tidy-ups to make less noise for in whilst reviewing and test fixes to make less noise here.

Also addressed the following points from @nedjo's review - 1, 5, 7, 8, 9, 10.

Some thoughts. Pasting on before a meeting. Only a start on a review.

1. +++ b/core/lib/Drupal/Core/Config/CachedStorage.php
   @@ -254,6 +254,15 @@ public function getCollectionName() {
   +  /**
   +   * Gets the decorated storage.
   +   *
   +   * @return \Drupal\Core\Config\StorageInterface
   +   */
   +  public function getWrappedStorage() {
   +    return $this->storage;
   +  }
   

   If we need this we need a new interface. Do we need it or can we always pass in the raw storage?

2. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
   @@ -104,6 +120,8 @@ public function installDefaultConfig($type, $name) {
   +        // Transform the default config storage.
   +        $storage = $this->storageTransformer->transform($storage, 'config_install.' . $type . '.' . $name . '.install', 'config_install.default_config');
   
   @@ -179,6 +197,9 @@ public function installOptionalConfig(StorageInterface $storage = NULL, $depende
   +    // Transform the default config storage.
   +    $storage = $this->storageTransformer->transform($storage, 'config_install.optional_config', 'config_install.optional_config');
   
   @@ -637,7 +658,9 @@ protected function getProfileStorages($installing_name = '') {
   -          $profile_storages[] = new FileStorage($profile_path . '/' . $directory, StorageInterface::DEFAULT_COLLECTION);
   +          $storage = new FileStorage($profile_path . '/' . $directory, StorageInterface::DEFAULT_COLLECTION);
   +          $storage = $this->storageTransformer->transform($storage, 'config_install.profile.' . $profile, 'config_install.profile.' . $directory);
   +          $profile_storages[] = $storage;
            }
   

   I think transformation of config during extension install should be a secondary consideration. And one dealt with in a a follow-up. The are numerous complexities here - one of the top of my head is that during a config import we use the storage from the ConfigImporter which will already be transformed so we'll be transforming twice. Also the $type and $name are bing used for install from config/install but not the other locations which feels inconsistent.

3. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
   @@ -165,28 +158,58 @@ public function diff(StorageInterface $source_storage, StorageInterface $target_
   +  public static function copyConfig(StorageInterface $source, StorageInterface $target) {
   

   I think we need to somehow say this is not config import. And maybe prevent from copying to the active storage.

4. +++ b/core/lib/Drupal/Core/Config/InMemoryStorage.php
   @@ -0,0 +1,180 @@
   +class InMemoryStorage implements StorageInterface {
   

   For other storages in Drupal this is called MemoryStorage see \Drupal\Core\KeyValueStore\MemoryStorage for example or MemoryBackend for caches.

Just getting start reviewing.

1. --- a/core/lib/Drupal/Core/Config/FileStorage.php
   +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -312,6 +312,13 @@ public function getAllCollectionNames() {
       */
      protected function getAllCollectionNamesHelper($directory) {
        $collections = [];
   +    if (!file_exists($directory)) {
   +      // Return early as \DirectoryIterator throws an exception if the directory
   +      // does not exist. Some code in Drupal expects FileStorage to work even
   +      // when a non existing directory is passed.
   +      return $collections;
   +    }
   +
   

   Is this really related to ConfigTransform or is this a more generic core improvement that should be done regardless?

2. +  public function addContext($context) {
   +    $this->contexts[] = $context;
   +  }
   

   As we discussed on the call, we really shouldn't overload the "context" terminology as it will be confusing. Looks like this is just "extraData" that can be used for whatever purpose the caller wants.

3. +  /**
   +   * The transformer contexts.
   +   *
   +   * @var array
   +   */
   +  protected $transformerContexts;
   

   As an example of the confusion, are these "Contexts" the additional data for the Event above, or are these "contexts" something else?

4. +
   +/**
   + * Class TransformedStorageWrapper.
   + */
   +class TransformedStorageWrapper implements StorageInterface {
   

   It's possible I'm not understanding the purpose of this (till I look at the other patches), but feels like there should be a better way to handle this rather than having to re-implement all the $this->storage->* methods.

Haven't looked much as the full patch yet, but on initial glance my worry would be around "config_dev". If we are going to put an implementation of ConfigTransform into core, I'd like to see it deal with more than just a single "dev" environment and be more like "config_environment" that could be used for dev, stage, prod, etc. Also, "config_dev" made me think it should be a dev-only module and not something added to composer.json.

Since it's going to be messy to review these different patches, maybe split "config_dev" into a new contrib module that requires the configTransform patch. Then we can also discuss separately bringing that new module into core once the basic API is vetted.

For the ConfigInstaller changes, still thinking about those, but probably minor quip to pass the ConfigTransformer at the end of the argument list instead of in the middle.

Re #17.1 opened #3001979: \Drupal\Core\Config\FileStorage::getAllCollectionNames() should work when the directory does not exist

1. A small set of thoughts ... not a complete review at all.

2. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
   @@ -165,28 +158,59 @@ public function diff(StorageInterface $source_storage, StorageInterface $target_
   +  public static function copyConfig(StorageInterface $source, StorageInterface $target) {
   

   I wonder if we can have this as a utility class rather than another bit of API in ConfigManager - otherwise we have to consider this API and add it to the interface. I think it might be better to have StorageCopier class.

3. +++ b/core/lib/Drupal/Core/Config/MemoryStorage.php
   @@ -0,0 +1,180 @@
   +    $list = [];
   +    foreach ($names as $name) {
   +      if ($data = $this->read($name)) {
   +        $list[$name] = $data;
   +      }
   +    }
   

   I think this might be able work with array_intersect_key() ie. return array_intersect_key($this->config[$this->collection], array_flip($names));

4. +++ b/core/lib/Drupal/Core/Config/MemoryStorage.php
   @@ -0,0 +1,180 @@
   +    return new static(
   +      $this->config,
   +      $collection
   +    );
   

   This feels wrong. I think we should return the same object but with collection set differently. We don't need a whole new object here.

update config_filter / config_split etc to work with new API

Does this imply that both config_filter & config_split will still remain needed in order to achieve the use case of environment specific config or will they merely be a UI on top of the new API?

I believe the aim of this issue is to improve DX (c.f. Dries' blogpost) so it would make sense if these modules were no longer needed. On the other hand, writing custom code to override environment specific config does not seem like a DX improvement?

Perhaps someone can clarify where we want to head with this issue?

The memory storage is being handled in a separate issue here: #3015886: Add an in-memory config storage so added the reference.

Adding #3028179: Config Environment module (original issue) as related issue. The config_dev work in #14 and #19 can be moved over to that issue for continued work.

Confirming that the patch in #27 still works when testing via the config_exporter in #28 and the config_transformer_test module. Also to clarify that the patch in #28 is applied after #27.

omg. Best alter ever.

You can't get much better performance than memory storage. I share the concerns for larger sites though.

@bircher I really liked the storage factory service you were talking about in #7. I checked the memory foot print of my current site with this script and it's only 35MB, but I can imagine that bigger sites or sites with a bunch of webforms would get pretty resource intensive. We might not want to hard wire MemoryStorage in.

A couple of minor issues in the patch for #27:

+++ b/core/lib/Drupal/Core/Config/StorageTransformerInterface.php
@@ -0,0 +1,70 @@
+   *   The storage to transform for importing from it.

This description could use some clarity, its a bit confusing as written.

+++ b/core/lib/Drupal/Core/Config/ConfigEvents.php
@@ -143,4 +143,55 @@ final class ConfigEvents {
+  /**
+   * Name of the event fired when the config storage transformer does an export.
+   *
+   * This event allows modules to modify the configuration which is about to be
+   * exported.
+   *
+   * The event listener method receives a
+   * \Drupal\Core\Config\StorageTransformEvent instance.
+   *
+   * @Event
+   *
+   * @see \Drupal\Core\Config\StorageTransformEvent
+   * @see \Drupal\Core\Config\StorageTransformer::transformImport
+   *
+   * @var string
+   */
+  const STORAGE_TRANSFORM_EXPORT = 'config.transformer.export';

the line - `@see \Drupal\Core\Config\StorageTransformer::transformImport` - should reference the transformExport method

As discussed in the CMI 2 Slack chat, moving this API into the config_environment experimental module for now #3028179: Config Environment module (original issue). API can be moved back into core when ready for 8.8. Marking this issue as Postponed until that time.

1. +++ b/core/lib/Drupal/Core/Config/StorageTransformer.php
   @@ -0,0 +1,88 @@
   +    if ($event->isImport()) {
   +      $event_name = ConfigEvents::STORAGE_TRANSFORM_IMPORT;
   +    }
   +    elseif ($event->isExport()) {
   +      $event_name = ConfigEvents::STORAGE_TRANSFORM_EXPORT;
   +    }
   +    else {
   +      $event_name = ConfigEvents::STORAGE_TRANSFORM;
   +    }
   

   feels like we could have an $event->getEventName()?

2. +++ b/core/modules/config/src/Controller/ConfigController.php
   @@ -59,7 +59,7 @@ class ConfigController implements ContainerInjectionInterface {
   +      $container->get('config.storage_transformer')->transformImport($container->get('config.storage.sync'), 'config.storage.sync'),
   
   +++ b/core/modules/config/src/Form/ConfigImportForm.php
   @@ -37,7 +37,7 @@ public function __construct(StorageInterface $config_storage) {
   +      $container->get('config.storage_transformer')->transformImport($container->get('config.storage.sync'), 'config.storage.sync')
   
   +++ b/core/modules/config/src/Form/ConfigSync.php
   @@ -149,7 +149,7 @@ public function __construct(StorageInterface $sync_storage, StorageInterface $ac
   +      $container->get('config.storage_transformer')->transformImport($container->get('config.storage.sync'), 'config.storage.sync'),
   

   is doing this in a constructor a good idea?

@larowlan the code from (1) was removed in the latest patch in #3028179: Config Environment module (original issue) (#33). Please post additional review comments there. Once we have an Alpha experimental module we will split out the API again and bring it back here.

We need to spread the issue summary over to #3047812: Add a Config Transformation event dispatching during config import and export

The problem is that the \Drupal\Core\Config\ExportStorageManager::onConfigChange() is firing in the installer prior to the state being created. Thinking about this I'm not sure that this event needs to be active during a Drupal install so I've disabled it using \Drupal\Core\Installer\NormalInstallerServiceProvider.

Not reviewed the patch yet.

1. #3083065: Remove EventSubscriberInterface from ExportStorageManager and remove StorageRebuildNeededEvent was commited.

2. Changes from #43 are no longer necessary so I reverted 42-43-interdiff.txt.

-- ignore wrong patch --

-   * @see \Drupal\config_environment\Core\Config\ExportStorageManager::getStorage
+   * @see \Drupal\Core\Config\ExportStorageManager::getStorage

Seems to be working now. I've tried testing this briefly using simplytest.me but it seems to be having problems today.
Putting this into needs work so that we can test launching a site instance with it.

I've applied the patch:

[13:16] ricardo@ricardo-t440s:~/DrupalCore/drupal 8.8.x(1011+/569-,1)⇉*
$ git apply 2991683-47.patch

installed the site and got this error:

User 	admin
Location 	http://drupalcore.site/en/admin/config/development/configuration
Referrer 	http://drupalcore.site/en/admin/config
Message 	Error: Class 'Drupal\Core\Config\ImportStorageTransformer' not found in Drupal\Component\DependencyInjection\Container->createService() (line 273 of /app/core/lib/Drupal/Component/DependencyInjection/Container.php) #0 /app/core/lib/Drupal/Component/DependencyInjection/Container.php(173): Drupal\Component\DependencyInjection\Container->createService(Array, 'config.import_t...') #1 /app/core/modules/config/src/Form/ConfigSync.php(190): Drupal\Component\DependencyInjection\Container->get('config.import_t...') #2 /app/core/lib/Drupal/Core/DependencyInjection/ClassResolver.php(28): Drupal\config\Form\ConfigSync::create(Object(Drupal\Core\DependencyInjection\Container)) #3 /app/core/lib/Drupal/Core/Controller/HtmlFormController.php(48): Drupal\Core\DependencyInjection\ClassResolver->getInstanceFromDefinition('\\Drupal\\config\\...') #4 /app/core/lib/Drupal/Core/Controller/FormController.php(76): Drupal\Core\Controller\HtmlFormController->getFormObject(Object(Drupal\Core\Routing\RouteMatch), '\\Drupal\\config\\...') #5 [internal function]: Drupal\Core\Controller\FormController->getContentResult(Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\Routing\RouteMatch)) #6 /app/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(123): call_user_func_array(Array, Array) #7 /app/core/lib/Drupal/Core/Render/Renderer.php(572): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() #8 /app/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(124): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure)) #9 /app/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(97): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) #10 /app/vendor/symfony/http-kernel/HttpKernel.php(151): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() #11 /app/vendor/symfony/http-kernel/HttpKernel.php(68): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1) #12 /app/core/lib/Drupal/Core/StackMiddleware/Session.php(57): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #13 /app/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(47): Drupal\Core\StackMiddleware\Session->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #14 /app/core/modules/page_cache/src/StackMiddleware/PageCache.php(106): Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #15 /app/core/modules/page_cache/src/StackMiddleware/PageCache.php(85): Drupal\page_cache\StackMiddleware\PageCache->pass(Object(Symfony\Component\HttpFoundation\Request), 1, true) #16 /app/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(47): Drupal\page_cache\StackMiddleware\PageCache->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #17 /app/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(52): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #18 /app/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #19 /app/core/lib/Drupal/Core/DrupalKernel.php(693): Stack\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #20 /app/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request)) #21 {main}.
Severity 	Error
Hostname 	

@ricardoamaro I've tried to reproduce the error you are experiencing. I did this:

1. I applied the patch
2. Installed standard
3. Logged in as admin
4. Visited admin/config/development/configuration
5. Exported config

=> No error

I also applied the patch to an existing site. I visited admin/config/development/configuration before clearing the caches. This resulted in an error about a missing service - not the same error you got. But this went away after clearing the cache.

@ricardoamaro can you provide precise instructions on how to reproduce the error you are seeing?

[13:16] ricardo@ricardo-t440s:~/DrupalCore/drupal 8.8.x(1011+/569-,1)⇉*
$ git apply 2991683-47.patch

@ricardoamaro #47 is wrong. Try #48.

You are right. applying 48

$ git apply 2991683-48.patch
$ drush cex
Configuration successfully exported to sites/default/files/config/sync.                                                                                                                                                                              [success]

Also page is working now.

https://www.drupal.org/files/issues/2019-09-26/2991683-48.patch looks good!

Tagging for RM review

• Marking APIs that are clearly intended to be called or extended @internal is insufficient (and misleading and confusing). It leads to people basically just ignoring @internal.
• @internal should only be used for code that is actually designed as internal, and it should also always be accompanied by a docblock explanation of why the thing is internal (i.e. is it protected, a plugin implementation, part of an experimental module, not intended for reuse because $reasons, etc.).
• Having stuff outside the module directory also prevents us from cleanly removing alpha code prior to tagging a minor milestone (which has been the policy for several years now; only beta or higher code can be shipped in tagged releases).
• Finally, @internal also doesn't get you out of providing BC, because our policy is now to provide BC even for internal code (except in uncommon cases where providing BC is useless or actually dangerous, etc., at release manager discretion).

For all these reasons (based on reading the IS, not the patch or comments), the approach needs to be reconsidered. There are two options:

1. If the API is actually useful on its own, outside the experimental module, and meets all the quality expectations of a public core API, commit to providing full BC for it and actually provide it as a public API addition, with framework manager signoff.
    
2. Otherwise, keep the API within the experimental module, but use machine names that match what the machine names will be when the API is moved to \Drupal\Core. That way, when the module is eventually marked stable, the code can be added to the core namespace, the experimental module's old classes/methods/etc. can simply be deprecated and wrap the new core version, and there shouldn't be any disruption. There might be some complications related to configuration discovery, haven't thought it through, but that's the alternative that's been used in the past.

As a framework manager I'm +1 about the new API going in to core because it allows us to do #3079029: Move module config exclusion from config_environment to core.services which is one of the key missing features for configuration management.

The new core API classes being added are:

• \Drupal\Core\Config\ExportStorageManager (final)
• \Drupal\Core\Config\ImportStorageTransformer (final)
• \Drupal\Core\Config\ManagedStorage (final)
• \Drupal\Core\Config\StorageManagerInterface (not marked)
• \Drupal\Core\Config\StorageTransformEvent (not marked)

We are also adding two services: config.storage.export.manager and config.import_transformer. As far as I can see config.storage.export.manager could be public: false as the only accesses to it via a $container->get() is in tests. config.import_transformer is accessed in controllers so we can't make that public: false.

So for the ones marked final we need to document why. And did I miss any of the new API - what do you think @bircher?

On adding new things as final - I think this is a good plan because it restricts on API extension point and we can always remove final it as use-cases are made.

Another thing we need to do is to improve the release note section - I'm sure that

The config storage transformation api events can now be used without the config_environment module.

really sells this. I guess #3079029: Move module config exclusion from config_environment to core.services is bigger news.

Manual test and review done.
The pages seem also working fine.

[16:51] ricardo@ricardo-t440s:~/DrupalCore/drupal 8.8.x()*
$ git apply 2991683-64.patch
[16:51] ricardo@ricardo-t440s:~/DrupalCore/drupal 8.8.x(123+/1272-,25)⇉✖*
$  drush cex
The active configuration is identical to the configuration in the export directory (sites/default/files/config/sync).                                                                                                                                [ok]
[16:51] ricardo@ricardo-t440s:~/DrupalCore/drupal 8.8.x(123+/1272-,25)⇉✖*
$ rm -rf sites/default/files/config/sync/
[16:52] ricardo@ricardo-t440s:~/DrupalCore/drupal 8.8.x(123+/1272-,25)⇉✖*
$  drush cex
Configuration successfully exported to sites/default/files/config/sync.                                                                                                                                                                              [success]

OK we're in a good state for this going to stable, and the classes need to be in the core namespace for that to happen. Patch looks fine and has good manual testing, so committed 1076c7a and pushed to 8.8.x. Thanks!

The release note here doesn't describe the disruption from this issue. Can it be updated to explain the disruption? Docs here have some recommendations and examples for the release notes: https://www.drupal.org/issue-summaries#release-notes

Come to think of it, this also maybe actually isn't disruptive, since this was only alpha code before and not in any tagged release. I think this is another issue that should possibly be tagged as a highlight instead?

Yep I agree this is not disruptive. It's a highlight. All the authors of the large contributed module config ecosystem have a new API to play with - think config_split, config_filter, config_make_me_a_sandwich.