Add ExportStorage to allow config export in third party tools

Drush will happily use the new factory described here.

Not clear from summary what this factory decoration simplifying, from code it just decorates active storage with memory storage

Quick codereview

1. +++ b/core/lib/Drupal/Core/Config/ExportStorageFactory.php
   @@ -0,0 +1,52 @@
   +   * @param \Drupal\Core\Config\StorageInterface $activeStorage
   ...
   +  public function __construct(StorageInterface $activeStorage) {
   

   for constructors we still not using cameCase arguments in core(

2. +++ b/core/lib/Drupal/Core/Config/ExportStorageFactoryInterface.php
   @@ -0,0 +1,17 @@
   +   * Returns a StorageInterface which can be used to export configuration.
   

   returns storage, not interface

3. +++ b/core/lib/Drupal/Core/Config/ExportStorageFactoryInterface.php
   @@ -0,0 +1,17 @@
   +   * @return \Drupal\Core\Config\StorageInterface
   +   */
   

   return require description according current coding standards

4. +++ b/core/modules/config/src/Controller/ConfigController.php
   @@ -92,10 +103,11 @@ public static function create(ContainerInterface $container) {
   -  public function __construct(StorageInterface $target_storage, StorageInterface $source_storage, ConfigManagerInterface $config_manager, FileDownloadController $file_download_controller, DiffFormatter $diff_formatter, FileSystemInterface $file_system) {
   +  public function __construct(StorageInterface $target_storage, StorageInterface $source_storage, ConfigManagerInterface $config_manager, ExportStorageFactoryInterface $exportStorageFactory, FileDownloadController $file_download_controller, DiffFormatter $diff_formatter, FileSystemInterface $file_system) {
   

   for compatibility reasons new dependency should be added last and trigger error that new argument will be required in 9.0.0

The code is pretty straight forward, only some few nit-picks.

1. +++ b/core/lib/Drupal/Core/Config/ExportStorageFactory.php
   @@ -0,0 +1,52 @@
   + * Class ExportStorageFactory.
   

   Guessing this is programmatically generated, perhaps we could say "Defines the export storage factory."

2. +++ b/core/lib/Drupal/Core/Config/ExportStorageFactory.php
   @@ -0,0 +1,52 @@
   +    //@TODO: Use StorageCopyTrait once it is in core.
   +    //@see https://www.drupal.org/project/drupal/issues/3016429
   

   @TODO should be lower case.

3. +++ b/core/tests/Drupal/Tests/Core/Config/ExportStorageFactoryTest.php
   @@ -0,0 +1,105 @@
   + * @group Config
   + * @coversDefaultClass \Drupal\Core\Config\ExportStorageFactory
   

   Move @coversDefaultClass to the top

4. +++ b/core/tests/Drupal/Tests/Core/Config/ExportStorageFactoryTest.php
   @@ -0,0 +1,105 @@
   +   * @param string|null $message
   ...
   +  protected function assertStorageEquals(StorageInterface $expected, StorageInterface $actual, $message = NULL) {
   

   You could just type hint for string, and set $message to = '' as that's what assertEquals does. However we don't do strict typing so it doesn't really matter. ¯\_(ツ)_/¯

In the patch, the `getExportStorage()` creates a MemoryStorage and copies the config to it, then returns. Then in your Change Record you show the example of using this returned storage to copy to a "sync" storage. Couldn't the `getExportStorage` simply be `exportStorage($dest)` and then in your Change Record you just show calling `\Drupal::service('config.export.factory')->exportStorage($sync)`?

I worry about the extra memory usage of creating the duplicate MemoryStorage object and also having the caller need to replicate the copy code. (although using the StorageCopyTrait will make this easier)

Perhaps still return the export storage and have it create the MemoryStorage only if the $sync argument is null?

After call with @bircher I have a clearer understanding of this and why the factory just returns the new Storage object to be written. I updated the Change Record https://www.drupal.org/node/3037022 to reduce its complexity and make it more obvious that the caller can do anything they need to write the config and won't necessarily be replicating the code being used within the export factory.

So my comment #13 is handled.

Last nit-pick in reviewing: Since the service is called "config.export.factory", having the function called "getExportStorage" seems redundant and confusing.

+++ b/core/lib/Drupal/Core/Config/ExportStorageFactory.php
@@ -0,0 +1,52 @@
+  public function getExportStorage() {

change to just getStorage(). So usage would look like:
$source = \Drupal::service('config.export.factory')->getStorage();

1. +++ b/core/tests/Drupal/Tests/Core/Config/ExportStorageFactoryTest.php
   @@ -0,0 +1,105 @@
   +   * @covers ::getExportStorage
   

   getStorage

2. +++ b/core/tests/Drupal/Tests/Core/Config/ExportStorageFactoryTest.php
   @@ -0,0 +1,105 @@
   +  public function testGetExportStorage() {
   

   testGetStorage

1. +++ b/core/modules/config/src/Controller/ConfigController.php
   @@ -113,13 +129,14 @@ public function downloadExport() {
   +    $export = $this->exportStorageFactory->getExportStorage();
   

   getStorage

2. +++ b/core/tests/Drupal/Tests/Core/Config/ExportStorageFactoryTest.php
   @@ -0,0 +1,105 @@
   +    $export = $this->exportStorageFactory->getExportStorage();
   

   getStorage

This looks great now! Will make it much easier for tools like Drush to support new stuff coming in CMI 2.

I like this, its a good step towards the goals

1. +++ b/core/lib/Drupal/Core/Config/ExportStorageFactory.php
   @@ -0,0 +1,52 @@
   + * @package Drupal\Core\Config
   

   I don't know that this is something we normally do

2. +++ b/core/modules/config/src/Controller/ConfigController.php
   @@ -113,13 +129,14 @@ public function downloadExport() {
   -    // Get raw configuration data without overrides.
   -    foreach ($this->configManager->getConfigFactory()->listAll() as $name) {
   -      $archiver->addString("$name.yml", Yaml::encode($this->configManager->getConfigFactory()->get($name)->getRawData()));
   +    // Get the configuration to export from the export storage factory.
   +    $export = $this->exportStorageFactory->getStorage();
   +    foreach ($export->listAll() as $name) {
   +      $archiver->addString("$name.yml", Yaml::encode($export->read($name)));
   

   previously this only ever read one item into memory at once, but now we read it all. Could we do this with yield and iterators to prevent OOM on sites with a lot of config?

I think #2 is a great idea and we need to address memory concerns here because this is reading all of config into memory. Anecdotally I've be told that sites with large amounts of field struggle with memory on config sync and this more add a new place for such sites to fail.

We should make more use of generators when dealing with the entire site config and this would be a great place to start.

+++ b/core/lib/Drupal/Core/Config/ExportStorageFactory.php
@@ -0,0 +1,38 @@
+    // Return the active storage for now for performance reasons.
+    // Callers should not modify the storage.
+    return $this->activeStorage;

if we take this approach, could we decorate Active storage with a ReadOnlyStorage to enforce this?

+++ b/core/core.services.yml
@@ -306,6 +306,9 @@ services:
+  config.export.factory:
+    class: Drupal\Core\Config\ExportStorageFactory
+    arguments: ['@config.storage']

Can we do a proper factory in the container?

So we have a service called 'config.export' that is not shared and created from a service called config.export.factory?

See https://symfony.com/doc/current/service_container/factories.html

That way you don't have to inject the factory and you get a storage interface object to work with.

This looks very good to me. One very minor nitpick:

1. +++ b/core/tests/Drupal/Tests/Core/Config/ReadOnlyStorageTest.php
   @@ -0,0 +1,197 @@
   +    $fixture = $fixture = [
   

   Should just be "$fixture = ["

This could use a release notes snippet before commit, but otherwise looks good to me. Since it's 99% adding a minimal new class and test coverage I think it would be OK to get in before beta to unblock other work.

This looks good to me too.

Sorry I missed the update here. We've missed the beta window but committed 52bd9a1 and pushed to 8.8.x. Thanks!

Changing tag to highlights so we can call this out but it's not disruptive so doesn't need a full release note snippet.

Thanks @bircher. We use the tag "8.8.0 release notes" (or similar) for an issue that has some sort of disruptive impact beyond what a typical deprecation identifies: It requires a manual ugprade path step, or has a public BC break, or causes tests to fail for contrib, or affects which dependencies are packaged, or changes coding standards such that other Drupal code will need to be updated to comply. Anything that has a chance of breaking some usecase or isn't strictly allowable under policy. Typically, the release note snippet for those needs to explain, at a glance, how a site owner or module developer can tell if they need to change something, and what they need to change, with a link to the CR for more detailed explanations. See: https://www.drupal.org/issue-summaries#release-notes

"8.8.0 highlights", on the other hand, is used to separate out new features of the minor version that are worth celebrating in messaging about the release. A release snippet is still useful for such issues because it saves Gábor an hour of trying to understand the value of the change, but it's not a requirement to release the alpha. Sometimes it's used alongside the other tag if an issue both delivers significant value but also has disruptive impacts.

We used to use "8.8.0 release notes" for both types of things (several minors back), so committers occasionally forget and put the old tag on issues with no disruption. It's probable that that's what @catch did above. However, if this change does require changes of module developers or site owners beyond what any deprecation notices for it might surface, then the release note should be updated with an explanation of that and we can re-tag with "8.8.0 release notes".