ConfigConfigurator should be insensitive to key order when comparing active config to recipe config

This blocks 🐛 Error when installing a recipe that has configuration files already in the system, even if there is no difference Postponed: needs info .

99% ready!

One last thought: https://git.drupalcode.org/project/distributions_recipes/-/merge_request...

🚢

I don't want to derail this, but the solution doesn't match the argument in the issue summary because to check if it is a mapping array_is_list is not enough.

Also I would rather argue that doing the comparison at all is wrong, A recipe which adds a new role should not fail to apply when the role has gotten a new permission. But I haven't dug into the code enough in a sufficiently long time to not be in a position to make any claims... So I leave the issue status as is and just post a comment.

A recipe which adds a new role should not fail to apply when the role has gotten a new permission

This is why config actions exist.

In general we've been working towards more ordered config over the last few years so I'm wondering where this has come up as a problem - it would be great to have specific examples. Such examples are missing from the issue summary.

Note that I'm particularly against this change but it is a massive assumption that the order is not meaningless. It should not be. But when we first implemented it in core this was not the case. And @bircher is correct - you can only really sort maps by keys this way. @bircher does config split have some code we can copy here and should we be thinking about making this functionality a utility class or something outside of recipes for others to use?

Setting to needs work for the array_is_list() function - and changing that to use config schema if available.

I thought array_is_list() would be a reasonable way to detect whether something is a sequence or not — but you're right: type: sequence only means arbitrary keys are allowed, not that it's only integer-indexed keys (which is what https://www.php.net/manual/en/function.array-is-list.php checks).

My bad!

So, unfortunately this means that the logic here needs to become orders of magnitude more complex: the entire comparison must be schema-aware.

@bircher Is there logic this could borrow from Config Split? 🙏

Per #16.

We need something like \Drupal\config_split\Config\ConfigSorter … or https://www.drupal.org/project/config_normalizer → , which the code in config_split is recommending 😄

IOW: we need to define how to build a canonical/normalized representation … because then we'd be able to do the comparison we need here.

Unfortunately, both of those seem to be outdated — with last commits in 2021 and mostly saying "do X better when Y happens".

So here's instead an initial stab at adding the necessary infrastructure, and … it was surprisingly simple? 😅 Unless I overlooked something, which I probably did.

Been thinking on this and I think I finally see where @bircher, @alexpott, and Wim are coming from.

Long story short - sequences are tricky. The approach I originally took here would detect "these two arrays contain different items, regardless of order" -- but it would not detect "these two arrays contain the same items, in a different order".

Granted, in most situations, that probably would not matter too much, and the functional result would be identical, from the site builder's perspective. But the CKEditor toolbar is an excellent example of a place where the order of the items is exactly as important as what the items are.

Having said that - my concern in this issue is more that it can make recipes unusable for people who may or may not have taken any conscious action to alter their config. So if getting past it is as relatively simple as Wim's initial work in the most recent commits, then I'm fully on board with that. But if it turns out to be a nest of dragons, my preference is to do a recursive-sort-by-key and then slay the dragons in other issues (which, to be clear, would need to be slain before the recipe system would be release-ready).

I want to iterate quickly, especially since my current goals are about config validation with regard to recipes, which isn't really related to this problem. Given those circumstances, I'll take a quick, temporary workaround/fix over the long-slog, proper, dragonslayer fix for now.

Addressed @bircher's feedback — I totally forgot about that for a bit 😇 Was trivial to add 😊

Looking further: I see no reason we cannot land this in Drupal core? 😄 I did add a new method to \Drupal\Core\Config\Schema\Element (which is an abstract class), so that qualifies as a API change/BC break. But there's ways around that (e.g. use an interface to make it optional instead of required).

I will update the IS here.

I'm glad we landed on this solution. I think this fix is much more "correct", and feels more proper, than what I was proposing. I was quite worried by Wim's feeling in #16 that this would be incredibly complicated, because Wim and I have ping-ponged our way through very complex config schema issues and I'm still in therapy over them. I'm deeply relieved to see that it's not so bad at all!

A few small points of review, and then I'd be happy RTBCing this if @alexpott and/or @bircher agree.

I am +1 for getting it fixed in core as well.

I think @bircher should provide final review here.

Alternatively to all the complexity here can't we cast the incoming config - and if it matches all good :)

We're going to need to somehow extract stuff from the Config and StorableConfigBase objects to do this... but that code probably should not be there anyway.

1. Alternatively to all the complexity here can't we cast the incoming config - and if it matches all good :)

   I don't see much complexity? 😅

   I think you're saying that an alternative could be to add a new public method that calls protected function castValue($key, $value) { … } on StorableConfigBase (and hence also on Config). But that is only available for simple config, not for config entities (which uses \Drupal\Core\Config\Entity\ConfigEntityStorage::mapToStorageRecord(), which in turn calls \Drupal\Core\Config\Entity\ConfigEntityBase::toArray(), which does NOT do any sorting).

   That's why I implemented this at the config schema/typed config level, so that it'd work for all config.

2. I think you're looking at this and are thinking "hm, that's quite a bit of duplication of snippets of logic that already exist". But AFAICT this is doing it at the abstraction layer that A) makes it available for external use, B) moves the logic out of a single central place into the class associated with each type? Isn't this preferable?
   

3. We're going to need to somehow extract stuff from the Config and StorableConfigBase objects to do this... but that code probably should not be there anyway.

   Ahhh! 😄 That's pretty much what I wrote above 😊

   Also, @bircher wrote in Slack:

   and finally the sorting in storable config should use this canonical representation when saving

   I tried to do what I think the two of you are looking for:

   1. use PrimitiveInterface::getCastedValue()
   2. match the "value" sorting logic from StorableConfigBase::castValue()
   3. match the "null" and "empty string" logic from StorableConfigBase::castValue()
   4. match the "do not drop mapping keys not defined in schema" logic from StorableConfigBase::castValue()
   5. Finally: Remove StorableConfigBase::castValue() in favor of Element::getCanonicalRepresentation().

   Result: the +306,-128 diffstat consists of +146,-23 test-only changes and +21,-3 recipe-logic-only changes.

   And … ::getCanonicalRepresentation() is used to save config. It behaves identically to StorableConfigBase::castValue(), but it's now a public API available via the Typed Data (i.e. schema-aware) representation of config.

AFAICT this is the API that @bircher indicated he needed in:

• config_split: https://git.drupalcode.org/project/config_split/-/blob/2.0.x/src/Config/...
• config_normalizer: https://git.drupalcode.org/project/config_normalizer/-/blob/2.0.x/src/Co...

Yes I think @alexpott in #23 is trying to say the same as me. And I also think you understood the idea.
I don't have time this morning to properly look at it. But I think this should be in core!
📌 Expose API to sort arbitrary config arrays Needs work

And yes RE #25

From Slack:

alexpott
I feel like we need to move most of this to a core issue and land in there. FI we commit this to our repo it will make keeping inline with core way harder and we already know there are things out there that will benefit.

alexpott
Imo what we could do is open a core issue and work on that, and commit @phenaproxima’s original work with a massive @todo to use the core work.

I completely agree with this.

This MR, although absolutely the fix we want, now touches enough parts of core that it will be painful to chase HEAD with the recipe system patch. In the name of getting past the problem immediately facing recipe users, and the fact that the core issue will take longer to land, this feels like a solid approach to me.

Tagging for a follow-up to either identify an existing core issue, or open a new one.

@bircher identified 📌 Expose API to sort arbitrary config arrays Needs work as the core issue (already filed with a great summary!) where we should land the proper fix that Wim developed.

Per #29.

FYI: 📌 Expose API to sort arbitrary config arrays Needs work is now green and ready for review 👍

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