New API for config_distro modelled after the new CMI2 config transformation API

Hi,
Great work! you are on a good track!

We need only one event and only one event name because we don't import and export. We transform the active storage into a virtual storage and import that. You can think of the config distro storage as "How would my active storage look like if all the distro updates had been applied.

The new sub module I am asking to be created is not meant as a test module, and the name does not have filter in it by accident.
The module description could be: BC bridge to the previous config filter plugin api.
If we just change wholesale to a new API design then all existing sites will be broken and all the modules currently integrating will stop to work. So instead of having the old api still around we move it to a new module and install it in an update hook. this way if you update everything still works and if you install it new you only get the new api. Technically the other modules then depend on config_distro_filter but that is out of our hands here.

That said, we of course need tests too. But since we don't have any I am not insisting on that here.

Here some initial review.

1. +++ b/config_distro.services.yml
   @@ -1,9 +1,9 @@
   +  config_distro.storage.distro.manager:
   +      class: Drupal\config_distro\DistroStorageManager
   +      arguments: ['@config.storage', '@state', '@database', '@event_dispatcher']
   +      tags:
   +        - { name: event_subscriber }
   

   probably doesn't need state or be an event subscriber itself see below.

2. +++ b/modules/config_distro_ignore/config_distro_ignore.info.yml
   @@ -1,7 +1,13 @@
   +# Information added by Drupal.org packaging script on 2018-10-09
   

   This is added by drupal.org so we don't need it in our module code

3. +++ b/modules/config_distro_transform/config_distro_transform.info.yml
   @@ -0,0 +1,15 @@
   +# @todo: Move this test module under the config module in #2991683.
   

   Too much copy paste I guess

4. +++ b/modules/config_distro_transform/config_distro_transform.services.yml
   @@ -0,0 +1,6 @@
   +  config_distro_transform.event_subscriber:
   +    class: Drupal\config_distro_transform\EventSubscriber
   +    arguments: ['@config.storage', '@config.storage.sync']
   +    tags:
   +      - { name: event_subscriber }
   

   This is what I meant with getting inspired by the 2.x patch for config_filter. This should have as its arguments only @config_filter.storage_factory

   You can do the thing that happens in the import transformation on the config_filter patch
   Ie the storage form the DistroStorageTransformEvent is the "active" so you get a filtered storage for the event storage, then you copy the filtered storage into a temporary one and then the temporary one back to the event storage.

5. +++ b/modules/config_distro_transform/src/EventSubscriber.php
   @@ -0,0 +1,102 @@
   +  public static function getSubscribedEvents() {
   +    // @todo: use class constants when they get added in #2991683
   +    $events['config.transform.import'][] = ['onImportTransform'];
   +    $events['config.transform.export'][] = ['onExportTransform'];
   +    return $events;
   +  }
   

   We don't care about the core events at all, just our new event that is inspired by it. Config Distro is doing something orthogonal.

6. +++ b/src/Controller/ConfigDistroController.php
   @@ -2,33 +2,47 @@
   -    $class->sourceStorage = $container->get('config_distro.storage.distro');
   ...
   +    $this->sourceStorage = $this->importTransformer->transform($this->syncStorage);
   

   We don't need to touch the controller in this patch at all since we keep the distro storage service.

7. +++ b/src/Core/Config/ConfigEvents.php
   @@ -0,0 +1,79 @@
   +final class ConfigEvents {
   

   too much copy paste, we don't need this at all. We might have one that describes the new event we are adding. but then it is definitely not related to the core one.

8. +++ b/src/Core/Config/ImportStorageTransformer.php
   @@ -0,0 +1,83 @@
   +class ImportStorageTransformer {
   

   Too much copy paste, we don't need the import transformer because our distro storage is already doing that job.

9. +++ b/src/Core/Config/ManagedStorage.php
   @@ -0,0 +1,157 @@
   +// @todo: Move this back to \Drupal\Core\Config in #2991683.
   ...
   +class ManagedStorage implements StorageInterface {
   

   yes the managed storage we copy from core, we may want to add a different todo though

10. +++ b/src/DistroStorageManager.php
    @@ -0,0 +1,113 @@
    +  use StorageCopyTrait;
    

    I forgot in which Drupal version we added that I think 8.7, we can copy the trait too or make the minimal version 8.7

11. +++ b/src/DistroStorageManager.php
    @@ -0,0 +1,113 @@
    +    if ($this->state->get(self::NEEDS_REBUILD_KEY, TRUE)) {
    ...
    +      $this->state->set(self::NEEDS_REBUILD_KEY, FALSE);
    

    We don't need to do that conditionally, we dispatch the event every time the storage is needed. The managed storage only calls this once.

12. +++ b/src/DistroStorageManager.php
    @@ -0,0 +1,113 @@
    +      $this->eventDispatcher->dispatch('config.transform.export', new DistroStorageTransformEvent($this->storage));
    

    This is the wrong event name, I would use 'config_distro.transform'
    Also make sure that the storage is either a memory storage or uses the config_distro table, we don't want to interfere with the internals of the core API.

13. +++ b/src/DistroStorageManager.php
    @@ -0,0 +1,113 @@
    +  public static function getSubscribedEvents() {
    +    $events[ConfigEvents::SAVE][] = ['onConfigChange', 0];
    +    $events[ConfigEvents::DELETE][] = ['onConfigChange', 0];
    +    $events[ConfigEvents::RENAME][] = ['onConfigChange', 0];
    +    return $events;
    +  }
    

    No need to cache anything.

14. +++ b/src/Event/DistroStorageTransformEvent.php
    @@ -0,0 +1,48 @@
    +// @todo: below removed when namespace is \Drupal\Core\Config in 2991683.
    

    We will always keep this event in config_distro

We don't need to change the controller here. But if you want to check out some of my experiments for the new core module check out
#3048860: Create Config Environment API comment #4
We may want to complicate things too for config distro or have some more options, like selecting which "packages of updates" to include etc. But for a fist approach I would keep it simple.

ah another small comment..
we can make the config_distro_ignore depend on config_distro_filter and not change anything in that for now.
Then updating config_distro_ignore would become a example exercise on how to update modules to use the new api in a followup.

Yes looks very good!

I am just on my phone at the moment, so not an in-depth review, just some things I noticed even without dreditor.

1) I think it is much better, yes.
4) The following line is using the wrong storage name:
$filtered = $this->filterStorageFactory->getFilteredStorage($storage, ['config.storage.sync']); we want the distro storage, ie what plugins would annotate for, see the config_distro_ignore plugin.
5) yea, to make it more readable it could be onDistroTransform, that is internal code though so it would work with any name.
9) yes we use it in the same way, but in config_environment the comment is about moving it to the right location, here we remove the class altogether when 8.8 becomes the minimal version we support. (provided the class gets to 8.8)
10) I think requiring 8.7 is ok. I think also the read-only storage was added in 8.7

15) The config_distro_ignore has now its core key commented out, it should instead depend on config_distro_filter
16) We also need an update hook enabling config_distro_filter

Cool!
Thanks for the patch, there are a few dependencies that need to be sorted out though.
This is just a very quick review, manual testing on a site that this module is used would be great too.

1. +++ b/config_distro.info.yml
   @@ -4,5 +4,6 @@ description: Framework for managing configuration updates from distributions.
   +# @todo: remove dependency on config_environment in #2991683.
   +  - drupal:config_environment
   

   We don't want to depend on config_environment.

2. +++ b/modules/config_distro_filter/config_distro_filter.info.yml
   @@ -0,0 +1,9 @@
   +  - drupal:config
   

   But instead of depending on cores config this module depends on config_filter

3. +++ b/modules/config_distro_filter/config_distro_filter.info.yml
   @@ -0,0 +1,9 @@
   +# @todo: remove dependency on config_environment in #2991683.
   +  - drupal:config_environment
   

   also don't depend on config_environment

4. +++ b/modules/config_distro_ignore/config_distro_ignore.info.yml
   @@ -4,4 +4,4 @@ description: Retain custom configuration when importing the distribution configu
   -  - config_distro:config_distro
   +  - config_distro:config_distro_filter
   

   config_distro_ignore should depend on both

5. +++ b/src/Core/Config/ManagedStorage.php
   @@ -0,0 +1,157 @@
   +// Use this class with its class alias Drupal\Core\Config\ManagedStorage
   

   we don't have or need a class alias, so this comment is not necessary

6. +++ b/src/Core/Config/StorageManagerInterface.php
   @@ -0,0 +1,24 @@
   +// @todo: Move this back to \Drupal\Core\Config in #2991683.
   +// Use this class with its class alias Drupal\Core\Config\StorageManagerInterface
   

   This comment also need to be updated. We can remove this class when 8.8 is the minimum supported version.

Some more remarks.

1. diff --git a/3080778-12.patch b/3080778-12.patch
   new file mode 100644
   index 0000000..9df68c4
   --- /dev/null
   +++ b/3080778-12.patch
   @@ -0,0 +1,39 @@
   

   :D Of course we shouldn't add the patch file.

2. +++ b/modules/config_distro_filter/src/ConfigDistroFilterEventSubscriber.php
   @@ -0,0 +1,60 @@
   \ No newline at end of file
   

   This new line is a super-nit-pick. We can also fix it on commit.

3. +++ b/modules/config_distro_ignore/config_distro_ignore.install
   @@ -0,0 +1,13 @@
   +/**
   + * Installs Config Distro Filter module.
   + */
   +function config_distro_ignore_update_8101() {
   +  \Drupal::service('module_installer')->install(['config_distro_filter']);
   +}
   

   Sorry for not having noticed this earlier.
   While of course config_distro_ignore now depends on config_filter through config_distro_filter, this update hook needs to be on the config_distro module. This is the best we can do to not disrupt current installations. Since all current modules that extend config_distro do so by means of config filter plugins. So we need this update hook to run regardless of config_distro_ignore but config_distro will not depend on this new module, so site builders are free to uninstall it again.

RE #16:
Good questions!

• Update hook only for <8.8: No. For sure not, we need to enable config_distro_filter because before this patch all sites that used config_distro will have done so using a config filter plugin for the distro storage. This patch has no dependency on Drupal 8.8 as we borrow all the 8.8 only code and remove them only once 8.8 is the minimal supported version. (In 8.8 with this version of config_distro there will be a couple of identical classes but with different namespaces. This is on purposes as it is due to backporting it for 8.7)
• Does it need 2.x of config_filter. No. config_filter 2.x is only for 8.8 and is a bridge for config filter plugins for the sync storage to the new core api in 8.8. The api of config_filter apart from not switching out the sync storage is unchanged in 2.x, so this will work with both 1.x and 2.x.

RE #26 Yes we could use a MemoryStorage instead of a database, in core it is a database because of concerns with memory usage. We can not use the GhostStorage (can see but can't interact with) because we expect event subscribers to actually modify the storage.

RE #27 Yes we should also add a lock (and in general copy paste from core and use our storage database) +1 also for the final.

RE #28 Yes given where we are now we can drop support for 8.7, sites still on 8.7 do not get the security coverage and they can keep using the previous version of the module.
And since we have config_distro_filter that we enable in a update hook we kan keep this on 1.x, this way we have the backwards compatibility (yes modules using the filter plugins would now have to depend on config_distro_filter but we do that for config_distro_ignore here and we can follow up with third party modules providing these filters to either add a dependency on config_distro_filter (easy) or switch to the new API (more code). Since probably most of these modules are maintained by one of the people active in this issue I think we are fine with this too.
config_distro_filter works with both 1.x and 2.x of config_filter so once we remove config_distro_filter we cut 2.x in my opinion.

config_distro_filter is the bridge between the current config_distro and the api after this patch the same way config_filter 2.x is the bridge between config_filter 1.x and the new core API. (that is also why config_filter 1.x and 2.x have the same API but are different versions because the behaviour is different)

NB: I would remove the classes that are tagged in the patch to be removed, but crutially keep the event ours even if it looks very similar to the core event because then we can add stuff if we want later without a problem.