Problem/Motivation

This issue adds a Config transformation API to the config_environment module. It does by dispatching an event on import and export.

Proposed resolution

Add events that are dispatched for import and export.

The API will initially be developed as part of the config_environment module. When ready for Beta release, this event dispatching API piece will be moved into the core Config namespace since the events should be dispatched regardless of the specific config_environment use case. This will allow contrib modules such as Config Split to use the new event API.

NOTE: Currently this module defines the core ConfigEvents class and uses namespace aliases. Code is marked with @todo to indicate which pieces will be moved into Core API when config_environment becomes Beta. See #2991683: Move configuration transformation API in \Drupal\Core\Config namespace.

There is a config_filter patch #3050529: Create 2.x branch using the new core api that can be used with this api, and an accompanying drush PR. With that config_split, config_ignore etc can be used with the new api even on drupal 8.6 when one copies the whole new experimental module.

Remaining tasks

  • Write patch
  • Write good docs final event class to really describe how this works.

User interface changes

None

API changes

  • Add new Config Transform import/export events that allow subscribers to modify config.
  • Trigger Config Transform export event in ConfigExportStorage.
  • Trigger Config Transform Import event in ConfigImport.

Data model changes

N/a

Release notes snippet

n/a -- moved to release note snippet from #2991683: Move configuration transformation API in \Drupal\Core\Config namespace since both are now in 8.8.0

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bircher created an issue. See original summary.

bircher’s picture

Adding the event dispatching.

bircher’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: 3047812-2.patch, failed testing. View results

bircher’s picture

Status: Needs review » Needs work

The last submitted patch, 5: 3047812-4.patch, failed testing. View results

alexpott’s picture

Title: Add event dispatching during config import and export » Add a Config Transformation event dispatching during config import and export
Issue summary: View changes

Retitling for clarity.

mpotter’s picture

Issue summary: View changes
mpotter’s picture

Issue summary: View changes
bircher’s picture

Uploading new patch with tests.

bircher’s picture

Status: Needs work » Needs review
bircher’s picture

ricardoamaro’s picture

I see Easter holidays were productive :)
I would like to do a manual test on this config transformation event.

ricardoamaro’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
86.54 KB
58.72 KB
314 bytes

I manually tested this patch today.

1 - added to settings.php:

$config['system.logging']['error_level'] = 'verbose';
$settings['extension_discovery_scan_tests'] = TRUE;

2 - Enabled:
- devel
- config_environment
- config_transformer_test

3 - Ran tests

4 - Exported and imported the resulting tar.gz file via admin/config/development/configuration and got positive results:

Patch is reviewed and tested!

Krzysztof Domański’s picture

Improved coding standards, docs and tests.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/config_environment/src/Core/Config/ConfigEvents.php
@@ -0,0 +1,52 @@
+  /**
+   * Name of the event fired just before importing configuration.
+   *
+   * This event allows subscribers to modify the configuration which is about to
+   * be imported.
+   *
+   * The event listener method receives a
+   * \Drupal\Core\Config\StorageTransformEvent instance.
+   *
+   * @Event
+   *
+   * @see \Drupal\Core\Config\StorageTransformEvent
+   *
+   * @var string
+   */
+  const STORAGE_TRANSFORM_IMPORT = 'config.transform.import';
+
+  /**
+   * Name of the event fired when the export storage is used.
+   *
+   * This event allows subscribers 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
+   *
+   * @var string
+   */
+  const STORAGE_TRANSFORM_EXPORT = 'config.transform.export';

I think we need far more documentation for the developer about what is happening here. This is the core of the new API we're adding. I think we need some examples of the things you can do and why.

Krzysztof Domański’s picture

Status: Needs work » Needs review
FileSize
2.47 KB
36.94 KB
/**
 * Name of the event fired just before importing configuration.
 *
 * This event allows subscribers to perform additional actions before
 * importing the configuration. For example, modifying the imported config
 * storage.
 *
 * The event listener method receives a
 * \Drupal\Core\Config\StorageTransformEvent instance.
 *
 * @Event
 *
 * @see \Drupal\Core\Config\StorageTransformEvent
 *
 * @var string
 */
const STORAGE_TRANSFORM_IMPORT = 'config.transform.import';

The 'config.transform.import' still should be better described, because is similar to 'config.importer.import' from core/lib/Drupal/Core/Config/ConfigEvents.php.

/**
 * Name of the event fired when importing configuration to target storage.
 *
 * This event allows modules to perform additional actions when configuration
 * is imported. The event listener method receives a
 * \Drupal\Core\Config\ConfigImporterEvent instance.
 *
 * @Event
 *
 * @see \Drupal\Core\Config\ConfigImporterEvent
 * @see \Drupal\Core\Config\ConfigImporter::import().
 * @see \Drupal\Core\EventSubscriber\ConfigSnapshotSubscriber::onConfigImporterImport().
 *
 * @var string
 */
const IMPORT = 'config.importer.import';
bircher’s picture

Attached a version with more information and explanation.

bircher’s picture

For the brave that want to test this on their complex sites with config split etc.
You can use the config_filter patch from #3050529: Create 2.x branch using the new core api and the drush PR.

borisson_’s picture

I only have small nitpicks/questions. This looks really solid.

I think the Needs tests tag should be removed, because it has testcoverage.

  1. +++ b/core/modules/config_environment/config_environment.module
    @@ -5,6 +5,18 @@
    +@class_alias('Drupal\config_environment\Core\Config\StorageTransformEvent', 'Drupal\Core\Config\StorageTransformEvent');
    

    This is really cool and it should make it easier to get this to beta/stable. +1

  2. +++ b/core/modules/config_environment/src/Core/Config/ConfigEvents.php
    @@ -0,0 +1,75 @@
    + * @deprecated Do not use this class, its constants will be moved to Drupal\Core\Config\ConfigEvents
    

    Shouldn't this be 80 cols?

  3. +++ b/core/modules/config_environment/src/Core/Config/ConfigEvents.php
    @@ -0,0 +1,75 @@
    +   * This event is also fired when just viewing the difference of configuration
    +   * to be imported independently of whether the import takes place or not.
    +   * Use the \Drupal\Core\Config\ConfigEvents::IMPORT event to subscribe to the
    +   * import having taken place.
    

    Oh, that is a pity. Shouldn't that be a different event?

  4. +++ b/core/modules/config_environment/src/Core/Config/ImportStorageTransformer.php
    @@ -0,0 +1,81 @@
    +// @todo: below removed when namespace is \Drupal\Core\Config in 2991683.
    

    remove below could lead to trouble if/when something new is added to this file before we do this. So it should probably mention wich use should be removed?

larowlan’s picture

This looks good, the dance around keeping this untangled from core is really difficult.

  1. +++ b/core/modules/config_environment/src/Core/Config/ConfigEvents.php
    @@ -0,0 +1,75 @@
    +final class ConfigEvents {
    
    +++ b/core/modules/config_environment/src/Core/Config/ExportStorageManager.php
    @@ -0,0 +1,111 @@
    +class ExportStorageManager implements StorageManagerInterface, EventSubscriberInterface {
    

    lets also put @internal on these to make it super-clear that there is no API here

  2. +++ b/core/modules/config_environment/src/Core/Config/ManagedStorage.php
    @@ -0,0 +1,153 @@
    +class ManagedStorage implements StorageInterface {
    
    +++ b/core/modules/config_environment/src/Core/Config/StorageManagerInterface.php
    @@ -0,0 +1,20 @@
    +interface StorageManagerInterface {
    
    +++ b/core/modules/config_environment/src/Core/Config/StorageTransformEvent.php
    @@ -0,0 +1,50 @@
    +class StorageTransformEvent extends Event {
    

    same here

bircher’s picture

New patch addressing the comments form #20 and #21.

RE #20:
1. Thanks, credits to @alexpott
2. Right, I rephrased it a bit, I am never sure when it is allowed due to namespaces.
3. No, it should be the same event whether the config is really imported or just prepared to show the difference of what is about to be imported. If they were different events one would have to subscribe to both in order for the import diff not to lie. We have the 'config.importer.import' event to react to config having been imported and new the 'config.transform.import' event to influence the config to be imported. If one wants to interact with the import more directly the 'config.importer.validate' event is for that. I think that should be enough.
4. I removed the comment, in fact it is probably covered with moving the files as then the namespace matches and is redundant, it applies also to all other classes in the Drupal\config_environment\Core\Config namespace of course.

RE #21:
Yes I would have been fine also just adding this directly to core. But of course I understand that we don't want to add API without consuming it and this is the compromise to make it easy to pull all that code in one swoop by deleting the experimental module.
1. & 2. yes I added the internal annotation, we can always remove it later when needed.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

This now looks really good. I don't see anything in the patch here that confuses me or that could be improved.

larowlan’s picture

Title: Add a Config Transformation event dispatching during config import and export » [PP-1] Add a Config Transformation event dispatching during config import and export
Status: Reviewed & tested by the community » Postponed

This was discussed in the core committers meeting #3061345: [Core Committers] Meeting notes 2019/06/06 and we decided to formalize the class alias approach in documentation - please see the policy issue #3061347: [Policy, no patch] Document use of class aliases by Experimental modules where the API's final destination will be a core namespace, lets get the policy/documentation updated and then get this in

bircher’s picture

Title: [PP-1] Add a Config Transformation event dispatching during config import and export » Add a Config Transformation event dispatching during config import and export
Status: Postponed » Reviewed & tested by the community
FileSize
37.97 KB
1001 bytes

The documentation is now in place, so this can go back to RTBC.

bircher’s picture

Issue summary: View changes

Added the link to the drush PR and config_filter patch that uses this API.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs change record

This is ready to go, can we get a change record describing the new APIs - when done, please ping me so it doesn't sit at RTBC again.

bircher’s picture

Issue summary: View changes
Issue tags: -Needs change record

I also added the release note snippet to the issue summary.

bircher’s picture

Issue summary: View changes

adding link to the change notice to the release snippet.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

This now has a change record and release note. Back to RTBC.

larowlan’s picture

adding review credits including mpotter for issue summary updates

larowlan’s picture

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

I went to commit this but there are quite a few coding standards issues, well not issues per-se, but violations from our current rule-set 😭

FILE: ...core/modules/config_environment/src/Core/Config/ConfigEvents.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 3 | ERROR | [x] Namespaced classes, interfaces and traits should not
   |       |     begin with a file doc comment
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 224ms; Memory: 8MB


FILE: ...ules/config_environment/src/Core/Config/ExportStorageManager.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 3 | ERROR | [x] Namespaced classes, interfaces and traits should not
   |       |     begin with a file doc comment
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 568ms; Memory: 8MB


FILE: .../config_environment/src/Core/Config/ImportStorageTransformer.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 3 | ERROR | [x] Namespaced classes, interfaces and traits should not
   |       |     begin with a file doc comment
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 184ms; Memory: 8MB


FILE: ...re/modules/config_environment/src/Core/Config/ManagedStorage.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 3 | ERROR | [x] Namespaced classes, interfaces and traits should not
   |       |     begin with a file doc comment
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 302ms; Memory: 8MB


FILE: ...s/config_environment/src/Core/Config/StorageManagerInterface.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 3 | ERROR | [x] Namespaced classes, interfaces and traits should not
   |       |     begin with a file doc comment
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 95ms; Memory: 8MB


FILE: ...les/config_environment/src/Core/Config/StorageTransformEvent.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 3 | ERROR | [x] Namespaced classes, interfaces and traits should not
   |       |     begin with a file doc comment
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 115ms; Memory: 8MB

PHPCS: core/modules/config_environment/src/Form/ConfigSync.php passed
PHPCS: core/modules/config_environment/tests/config_transformer_test/config_transformer_test.info.yml passed
PHPCS: core/modules/config_environment/tests/config_transformer_test/config_transformer_test.services.yml passed
PHPCS: core/modules/config_environment/tests/config_transformer_test/src/EventSubscriber.php passed

FILE: ...ent/tests/src/Functional/TransformedConfigExportImportUITest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 3 | ERROR | [x] Namespaced classes, interfaces and traits should not
   |       |     begin with a file doc comment
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 148ms; Memory: 8MB


FILE: ...onment/tests/src/Kernel/Core/Config/ExportStorageManagerTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 3 | ERROR | [x] Namespaced classes, interfaces and traits should not
   |       |     begin with a file doc comment
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 141ms; Memory: 8MB


FILE: ...nt/tests/src/Kernel/Core/Config/ImportStorageTransformerTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 3 | ERROR | [x] Namespaced classes, interfaces and traits should not
   |       |     begin with a file doc comment
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 270ms; Memory: 8MB


FILE: ...ment/tests/src/Kernel/Core/Config/Storage/ManagedStorageTest.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 3 | ERROR | [x] Namespaced classes, interfaces and traits should not
   |       |     begin with a file doc comment
 4 | ERROR | [x] There must be one blank line after the namespace
   |       |     declaration
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Most of them relate to our todos

I think we're going to need to update phpcs.xml to prevent these violations - as we don't want to change the code.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
6.63 KB
38.58 KB

As we're going to remove all these @todos I think we can use // @codingStandardsIgnoreStart and // @codingStandardsIgnoreEnd to get around this.

As this is only adding comments and adding things that will be removed setting back to rtbc.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

@larowlan wrote about #34

Agree but don't have laptop

Also #34 was my first patch so I think it's fine for me to commit this.

Committed 3ae3e52 and pushed to 8.8.x. Thanks!

  • alexpott committed 3ae3e52 on 8.8.x
    Issue #3047812 by bircher, Krzysztof Domański, alexpott, ricardoamaro,...
alexpott’s picture

I think we should only publish the CR once it is part of core. Otherwise the CR is very very confusing.

bircher’s picture

I guess @andypost didn't see #37. I saw it pop up in my rss reader. I just edited it to put the warning about experimental modules be at the top.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

bircher’s picture

Issue summary: View changes