First of all, thank you for an awesome module. I have a feature request. There are use cases where it is important to know what was merged and what was ignored or substituted. In combination with config_sync this module is a very powerful tool, so it would be nice to have some report on how the process went through.

I suggest to add static variable and add some log info there and also add some event classes to dispatch events so other modules can react.

The patch is attached.

CommentFileSizeAuthor
#10 3038347-config_merge_logs-10.patch4.17 KBa.dmitriiev
#6 3038347-config_merge_logs-6.patch6.2 KBa.dmitriiev
config_merge_logs.patch6.3 KBa.dmitriiev

Comments

a.dmitriiev created an issue. See original summary.

a.dmitriiev’s picture

a.dmitriiev commented
Status: Active » Needs review
nedjo’s picture

nedjo
he/him/his
Primary language English
commented

@a.dmitriiev apologies, I wasn't properly subscribed to issues on this project so didn't see this when you posted it. This looks like a good idea.

More generally, I am looking for help maintaining the Configuration Synchronizer module set, including Configuration Merge, Configuration Normalizer, Configuration Provider, and Configuration Snapshot. Please contact me if you're potentially interested in co-maintaining some or all of these modules.

a.dmitriiev’s picture

a.dmitriiev commented

@nedjo, please take a look at the patch, does it seem reasonable to have this? What is your opinion?

nedjo’s picture

nedjo
he/him/his
Primary language English
commented
Status: Needs review » Needs work

Looking good! Here are some suggestions.

  1. +++ b/src/ConfigMerger.php
@@ -2,6 +2,7 @@
+

    Unneeded line return. 

  2. +++ b/src/ConfigMerger.php
@@ -9,17 +10,38 @@ use Symfony\Component\Yaml\Inline;
+   * @var array

    Should give description of static. 

  3. +++ b/src/ConfigMerger.php
@@ -9,17 +10,38 @@ use Symfony\Component\Yaml\Inline;
+   * @return array

    Needs documentation of method and return value. 

  4. +++ b/src/ConfigMerger.php
@@ -9,17 +10,38 @@ use Symfony\Component\Yaml\Inline;
+   * @return array

    Should give description of return value. 

  5. +++ b/src/ConfigMerger.php
@@ -60,10 +82,10 @@ class ConfigMerger {
+      // have keys that are either ...

    White space issue. 

  6. +++ b/src/ConfigMerger.php
@@ -60,10 +82,10 @@ class ConfigMerger {
+          // in the original result set or ...

    White space issue. 

  7. +++ b/src/ConfigMerger.php
@@ -76,13 +98,27 @@ class ConfigMerger {
+            $operation = 'ignore';

    Let's pull this and other $operation strings ('update', 'substitute') into defines in this class. 

  8. +++ b/src/Event/ConfigMergeEvent.php
@@ -0,0 +1,90 @@
+  protected $config_name;

    Should be camel case ($providerType) and have description. 

  9. +++ b/src/Event/ConfigMergeEvent.php
@@ -0,0 +1,90 @@
+   * @var array

    Please add description. 

  10. +++ b/src/Event/ConfigMergeEvent.php
@@ -0,0 +1,90 @@
+   * @var string

    Should be camel case ($providerType) and have description. 

  11. +++ b/src/Event/ConfigMergeEvent.php
@@ -0,0 +1,90 @@
+   * @var string

    Please add description. 

  12. +++ b/src/Event/ConfigMergeEvent.php
@@ -0,0 +1,90 @@
+   *   The uuid of the configuration object being changed.

    UUID seems wrong for $config_name?

a.dmitriiev’s picture

a.dmitriiev commented
Status: Needs work » Needs review
StatusFileSize
new 3038347-config_merge_logs-6.patch6.2 KB

The patch was updated. Please check.

nedjo’s picture

nedjo
he/him/his
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Looks great, thanks!

One final detail .I wasn't remembering that the Drupal standard is to use const rather than define(), see the relevant coding standard:

In Drupal 8 and later, constants should be defined using the const PHP language keyword (instead of define()), because it is better for performance

So if before applying this you could switch the defines to consts (and also add PHPdoc for the consts per the coding standard I linked to) that would be great, thx!

nedjo’s picture

nedjo
he/him/his
Primary language English
commented

Sorry, I see you already used const rather than my outdated suggestion of define! Ignore that!

a.dmitriiev’s picture

a.dmitriiev commented

I will add the constants descriptions.

a.dmitriiev’s picture

a.dmitriiev commented
StatusFileSize
new 3038347-config_merge_logs-10.patch4.17 KB

Updated patch, descriptions to constants were added. Please review.

  • a.dmitriiev committed b079a61 on 8.x-1.x 
    Issue #3038347 by a.dmitriiev: Add some reporting on how merging went...
a.dmitriiev’s picture

a.dmitriiev commented
Status: Reviewed & tested by the community » Fixed

  • a.dmitriiev committed 11ead1b on 8.x-1.x 
    Issue #3038347 by a.dmitriiev: Add some reporting on how merging went...

Status: Fixed » Closed (fixed)

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