Expose API to sort arbitrary config arrays: TypedConfigManager::getCanonicalRepresentation() should override its parent

Re-titling the issue, because we don't necessarily need a new service it can be used with the typed config.

This blocks Recipes; it prevents Recipes from reliably detecting whether config is the same or has changed: #3416287: ConfigConfigurator should be insensitive to key order when comparing active config to recipe config → .

To be clear, this is a stable blocker for the Recipes initiative. (In practical terms, it is a soft blocker.)

Initially in #3416287: ConfigConfigurator should be insensitive to key order when comparing active config to recipe config → , I identified a workaround that will suffice in the short term, and we can commit that to the Recipes repository for now; see #3416287-27: ConfigConfigurator should be insensitive to key order when comparing active config to recipe config → .

Adjusting the remaining tasks; let's start by bringing Wim's excellent work from #3416287: ConfigConfigurator should be insensitive to key order when comparing active config to recipe config → into a new MR on this issue.

It's gonna be tricky to exactly match the expectations of SchemaConfigListenerTestTrait, i.e. this rather odd output:

    catch (SchemaIncompleteException $e) {
      $this->assertEquals("Schema errors for config_test.types with the following errors: config_test.types:array variable type is integer but applied schema class is Drupal\Core\Config\Schema\Sequence, config_test.types:foo missing schema, 0 [foo] 'foo' is not a supported key.", $e->getMessage());
    }

… I'll need some more time tomorrow to figure out a way forward for that 🤓

That being said, 99% of tests are passing already 😊

Following up on #12: I had two choices:

1. either not changing any of those assertions and hence being forced to save invalid config data first, because that is necessary for keeping SchemaCheckTrait::checkValue unchanged: it must be able to find flaws in stored config data
2. or changing the assertions and disallowing invalid config data to be saved — but this would be a disruptive BC break and a DX regression (not all info about invalid config data/missing schema at once, but one-by-one)

Did that in f2c1390d2ecb0ffde75514e2e75012211d7b98cd.

The 3 remaining failures are all in Views test coverage. They all relate to ViewExecutable wrapping View (the "storage" config entity) and both of them manipulating (in these 3 tests and only these 3!) settings on-the-fly and expecting ViewExecutable, View, display plugin, style plugin etc. all to remain in sync thanks to references to a private array 😱

See \Drupal\views\DisplayPluginCollection::initializePlugin(), which does

    // Retrieve and initialize the new display handler with data.
    $display = &$this->view->storage->getDisplay($display_id);
…
    $this->pluginInstances[$display_id]->initDisplay($this->view, $display);

there is a reference to the raw array in the config from within the ViewExecutable, this one is the culprit. Automatically resetting this is what I did in 60631295ad052a64464d14b758cc3c2d99d55db1 and that fixes 1 of the 3 failures.

Whew. There are 2 functional Views tests that are doing very unholy things — the 1 (out of 3) that was fixed in #14 was the least bad one. See https://git.drupalcode.org/project/drupal/-/merge_requests/6369/diffs?co... + https://git.drupalcode.org/project/drupal/-/merge_requests/6369/diffs?co... for two examples of View and ViewExecutable being expected to magically remain in sync with in-memory-only overrides (i.e. modifications that are not part of the View config entity, but manipulate runtime state). These two needed tiny tweaks to have more reasonable expectations of the connection between View and ViewExecutable.

This looks really good to me. I have a few minor things (mostly commenting-phrasing nits), as I usually do, but in general I understand this -- and when dealing with the typed config system, that's saying something -- and feel okay with it.

That said, I think the final sign-off here should belong to @bircher, who understands this more deeply than I and whose blessing would give me more confidence.

TIL \Drupal\Core\TypedData\TypedDataManagerInterface::getCanonicalRepresentation() was introduced in #2488568: Add a TypedDataManagerInterface and use it for typed parameters → — we should be able to use that instead, rather than expanding the API surface? 🤓

Whew, #17 was tricky, but I managed to do it — with a much simpler and less disruptive MR as the result! 🤩 I'm kinda proud of this one 🤓

https://git.drupalcode.org/project/drupal/-/merge_requests/6369/diffs?co... has 13 files changed, 103 insertions(+), 103 deletions(-) as the diffstat and causes:

… same exact diffstat, but 3 fewer files touched, and no BC breaks/contrib disruption 🥳

And actually, looks like @bircher kinda predicted this years ago, because in the issue summary he stated .

1. The only failing test was ConfigEntityAdapterTest::testValidate() — ConfigEntityAdapter was added in #1818574: Support config entities in typed data EntityAdapter → . With very thin test coverage, because config entities were even less validatable back then than they are today.
2. In fixing that, it turns out that https://git.drupalcode.org/project/drupal/-/merge_requests/6369/diffs?co... was insufficient: that only caught the case of assigning the empty array [] to e.g. StringData or BooleanData. What's needed is to instead ensure that as soon as any PrimitiveInterface contains non-compliant data, the raw value is used in the canonical representation.
3. But then another test starts failing, because there is one test case in all of core that requires FloatData and IntegerData which have the empty string '' assigned to be converted to NULL 🤪 Still, always better to minimize disruption, so matched that logic from StorableConfigBase::castValue(), by treating those as valid values instead (which exactly matches the intent) in PrimitiveTypeConstraintValidator and allowing (Float|Integer)Data::getCastedValue() to do whichever casting they please. 👍
4. Then a wide range of additional failures occurred … because apparently a LOT of tests rely on BooleanData accepting 'TRUE'/'FALSE' as an equivalent of TRUE/FALSE, despite no explicit PHP logic existing to do that 😳 … nor test coverage 😅
5. I was able to work around that, but then followed a huge range of migration test failures (and some node type-related test failures). Lots of migrations are migrating nonsense  — for example it's setting the type: boolean settings.alt_field (for the "image" field type's settings) to Test alt, which obviously is not a boolean 😅)
6. … to be continued tomorrow

The end result will be a more consistent (same representation during validation + saving — issue summary updated to reflect this) and understandable (no global knowledge in a protected method in a base class but each TypedData object has knowledge about itself) config system.

It also makes it possible to deprecate weird things (see the boolean/float/integer special cases above) at some point in the future.

1. Found a similar problem with Integer and Float (https://git.drupalcode.org/project/drupal/-/merge_requests/6369/diffs?co...) and another one for Integer (https://git.drupalcode.org/project/drupal/-/merge_requests/6369/diffs?co...)
2. That got us again to a single test failure. This time in \Drupal\KernelTests\Core\TypedData\TypedDataTest::testGetAndSet(). This bit:
   

       // Boolean type.
       $typed_data = $this->createTypedData(['type' => 'boolean'], TRUE);
       $this->assertInstanceOf(BooleanInterface::class, $typed_data);
   …
       $typed_data->setValue('invalid');
       $this->assertEquals(1, $typed_data->validate()->count(), 'Validation detected invalid value.');
   

   … this is only passing because it's testing an alternate reality: there's LOTS of examples of string values being assigned to type: boolean and the configuration system silently ignoring this (not even reporting it via SchemaCheckTrait), because as soon as we disallow this (by reverting the boolean-related additions to PrimitiveTypeConstraintValidator), lots of tests fail.

   Which means the only choice we have here is to change this expectation, so that it matches reality.

   P.S.: in reading that method, it's clear there's many more known bugs in this area, for example it points to the following @todo: 🐛 DateTimeIso8601::setDateTime() vs. DateTimeItem::schema(): (at least) one of them is broken Needs work .

3. NOW IT IS GREEN AGAIN!
4. While working on this, I discovered how closely related this issue is to 📌 Config export key order is not predictable, use config schema to order keys for maps Fixed . This builds on top of what that did, and simplifies it; it makes the overall system more consistent. I bet it'd have been far more difficult if that had not happened! In principle, many of the test changes that happened there should no longer be necessary, as well as the various explicit pieces of sorting logic. To prove this, https://git.drupalcode.org/project/drupal/-/merge_requests/6369/diffs?co... reverted the changes that issue made to ConfigEntityBase::preSave() to sort dependencies correctly.
5. Nope, that didn't work out because ConfigInstaller does $entity->trustData()->save();, which bypasses the call to ::getCanonicalRepresentation(). That's easily mitigated by making \Drupal\Core\Config\Config::save() always use ::getCanonicalRepresentation(), even for trusted data: https://git.drupalcode.org/project/drupal/-/merge_requests/6369/diffs?co...
6. This unveiled another range of invalid config in tests, and hence tests of an alternate reality rather than the real world. 🤪
7. So another range of trivial test expectation adjustments was needed: https://git.drupalcode.org/project/drupal/-/merge_requests/6369/diffs?co..., https://git.drupalcode.org/project/drupal/-/merge_requests/6369/diffs?co..., https://git.drupalcode.org/project/drupal/-/merge_requests/6369/diffs?co....
8. This also needed to invert a single assertion that #2432791: Skip Config::save schema validation of config data for trusted data → introduced — the test suite takes just as long as to run as in HEAD after the change to ALWAYS save with the canonical representation! Turns out that #2432791 especially made core faster thanks to not using config schema in ::toArray() 👍 That inverted expectation: https://git.drupalcode.org/project/drupal/-/merge_requests/6369/diffs?co... (which related to #2432791: Skip Config::save schema validation of config data for trusted data → )
9. Down to zero kernel test failures again, now a handful of functional test failures. The first two were for InstallerExistingConfigTestBase … turns out the multilingual fixture in there is wrong, it should've been updated in 📌 Get rid of using 'views.settings':skip_cache in ViewsData Fixed but wasn't.

(To be continued. Down to 2 functional test failures. And a LOT more sanity.)