Normalization should not strip uuid on deeper configuration levels

OK, that seems reasonable... Want to make a patch?

Let's see... I think that your conclusion here is correct: removing UUID and _core at all levels of the configuration was a bad implementation decision, so it is a bug that we can fix, and it is OK to change buggy behavior. So I think that we can just change the documentation of the $ignore parameter to the constructor, to say "Config components to ignore at the top level", and leave it at that.

The only caveat I have is that I would want to test this on a real site and verify that with the patch, I don't get a lot more items in the report marked as "changed" because we are no longer stripping out things that are not actually different. But I agree, stripping UUID out of the insides of image styles is not correct, and I'm also pretty sure _code is only added by the config system at the top level.

So... Implementation: Yes, let's just use the existing constructor parameter and change the docs slightly.

It looks like we'd need to add a second argument to the normalize() method, to detect whether we're at the top level or not, because this method is called recursively, and we only want to strip the ignore list at the top level. I would do something like

function normalize($config, $top_level = TRUE)

and then when it is called recursively, put in FALSE.

The patch looks mostly good to me, and our tests passed, so that is also good. I'm assuming you tested it on your case?

One thing needs a quick fix:

+  /**
+   * Recursively sort an array by key.
+   *
+   * @param array $array
+   *   An array that should be sorted by keys.
+   *
+   * @return array
+   *   Returns a sorted array by keys.
+   */

I'll fix up that method documentation, just pasting it here to remind myself to do that.

Another question I have is what these additions to the test are:

@@ -201,6 +251,7 @@ class ConfigDifferTest extends UnitTestCase {
       'id_to_remove' => 'test.remove.id',
       'type' => 'old_type',
       'true_value' => TRUE,
+      'null_value' => NULL,
       'nested_array' => [
         'flat_array' => [
           'value2',
@@ -216,6 +267,7 @@ class ConfigDifferTest extends UnitTestCase {
       'id' => 'test.config.id',
       'type' => 'new_type',
       'true_value' => FALSE,
+      'null_value' => FALSE,
       'nested_array' => [
         'flat_array' => [
           'value2',
@@ -284,11 +336,13 @@ class ConfigDifferTest extends UnitTestCase {
           'orig' => [
             'nested_array::flat_array::1 : value1',
             'nested_array::flat_array::2 : value3',
+            'null_value : (NULL)',
             'true_value : 1',
             'type : old_type',
           ],
           'closing' => [
             'nested_array::flat_array::1 : value3',
+            'null_value : ',
             'true_value : ',
             'type : new_type',
           ],

It seems like those might have been things you tried but decided you didn't need? Thanks!

Oh, it looks like the weekly test of this module is failing, that is the t() thing you fixed. I created a separate issue for that, so let's take the fix for that out of this patch and put it on
#2961886: Unit test for Config Differ is failing

So, I looked at this some more.

This patch is doing more than what the issue summary says. Normalization used to remove empty arrays from config, and this patch reverses that. This is not a good idea, based on my manual testing when I was first writing this module. It was leading to a lot of false positives, where I had installed a site a while back, and since then some new component had been added to the config, but it was empty. There was no functional difference between the config in my system and the new config, hence I didn't want it to show up in my reports as being changed when there was no real change.

So, I've put that back, and changed the test accordingly. Also updated the documentation mentioned in #6. The test passes locally... let's see what the bot says about all the tests.

Fix test failure... hopefully! Odd, this didn't fail locally for me but anyway, I understand the problem.

Passes this time, but two little coding standards fixes...

That is a good point about empty arrays, and thanks for catching my syntax error!

So, let's think about this question of arrays.
- It seems like empty arrays should *not* be removed if they are in numerically-sequenced config.
- I still think empty arrays should be removed if they are in keyed config, to avoid meaningless config differences.

Let's look at an example:

a: 1
b: {}
c:
  - one
  - two
  - {}
  - four
d: something

In this example, I think we should remove b, because it has no values in it. But we shouldn't remove the empty array under the C sequence, because that would break the sequence numbering.

This points to the fact that we don't have any tests that include numeric sequences. So... let's see if we can put this example into the test (or something substantially similar) and make it work. Thoughts?

It's tricky. The problem is that this module doesn't decide the meaning of configuration. Different implementations do. And it is possible for an empty array to mean something different from not being set. It wouldn't be great for the the user but it is totally possible and probably that someone has configured something to work that way. The same unfortunately goes for order of configuration. Ideally it'd never make a difference or we'd be enforcing orders via schema (there are patches to do this). But there are definitely examples in the wild were the order matters.

OK, I would be willing to get rid of the "remove empty arrays" thing.

But are there actually examples of config where the order of the keys, at some level, in a mapping (not a sequence) matters? I don't even know if that is possible to retain, given that there is a schema, and if you save config, it gets saved in the order of the schema.

The problem is that if you don't normalize the config at all, you end up seeing a bunch of "THIS CONFIG CHANGED" entries in your config update reports. Most of them are not real differences, they're just that you imported config a while back, and since then the schema has changed, and hopefully the module developer has also updated their stored config accordingly, so it has a bunch of minor changes that don't affect your actual site behavior. Like adding new options that have sensible (empty) defaults, and reordering things.

So the point of the normalization is to omit the things that don't matter, so you can see what has actually changed. I think it should cover the 99% case, so that the report is pretty good at telling you if there have been substantive changes that you should be aware of. If some module is doing something weird and unstable with config, I don't think we need to cover that rare and probably ill-advised possibility. But something like removing empty arrays in the middle of a sequence, we shouldn't do.

So with that in mind, ... I think I'm still in favor of (a) reordering config and (b) removing empty arrays except in sequences, because most of the time the reason the active config item has nothing and the one in config/install has an empty array is that there's a new key added to the schema since you installed, and it has a sensible (empty) default. Your site is working, after all, so there must have been a default... and if there was some non-empty data in there, it would show up in the diff, and if the active config needed to have that empty array in there, there would have been a hook_update_N() to add it.

Good idea. So, I committed the change (with the syntax fix). I created a child issue #2962529: Figure out normalization for ConfigDiffer as well as a change notice for this change.
https://www.drupal.org/node/2962527

Thanks for the comments and patches!