Allow configuration schema fallback in ConfigEntityType::getPropertiesToExport() to work without an ID

Re-titling so the issue is a bit more specific.

See #2432791-105: Skip Config::save schema validation of config data for trusted data - I'm not convinced about this change.

Something like this.

Right now in HEAD, if you call toArray() twice in a row, it checks the schema twice. Not true with this patch.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityType.php
@@ -165,9 +166,9 @@ protected function checkStorageClass($class) {
-  public function getPropertiesToExport() {
...
+  public function getPropertiesToExport($entity_id) {

Still not convinced about the coupling of ID to the entity type method.

re #4 on the second check it will be cached in schema so it's not a big saving - right? Also it only checks schema if you don't use the annotation.

Duh, we don't need that because config schema is smarter than us.

Because there's no way anyone is sneaky enough to define a per-entity-id schema, right? (please, right!?)

Heh, I so much wish accessing the schema didn't require a valid entity ID, alas currently it does.
Because of the schema is at prefix.*.*.* (with an unknown number of *s)

See #1977206: Default serialization of ConfigEntities, where I hit that wall as well (comments #26 to #31).
In short, we know the config prefix for an entity type, but not the number of dynamic parts in its IDs, which is what config schemas are keyed by...

Yes, @alexpott and I had a conversation about that. He also mentioned you had an idea about tying the schema to the entity type more closely, which is what I've done here.

I think I like this change - it'd be good to get @yched's thoughts on this because I think this issue has caused him head scratches in the past.

+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
@@ -159,6 +159,20 @@ public function getDefinition($base_plugin_id, $exception_on_invalid = TRUE, $is
+  public function getDefinitionForEntityType($entity_type_id) {
+    $id = 'undefined';
+    foreach ($this->getDefinitions() as $definition_id => $definition) {
+      if (isset($definition['config_entity_type']) && $definition['config_entity_type'] === $entity_type_id) {
+        $id = $definition_id;
+        break;
+      }
+    }
+    return $this->getDefinition($id);
+  }

Given the number of definitions it might be worth building up a static map here.

Agreed.

Huge +1 on having a more direct relation between an entity type and its config schema :-D

I would have thought that the simpler way to have that relationship is to put the missing info on the entity type (since it already has the "config_prefix"), rather than pointing back from the schema to the config entity type ID (which then could be inconsistent with what the config entity's "config_prefix" says) ?

It looks like the proposed change means a hard BC break anyway (your config entity stops working if its config schema doesn't have a 'config_entity_type' entry). If we're ready to break APIs for this (which I'm all for), then replacing
'config_prefix' = 'my.prefix'
by
'config_pattern' = 'my.prefix.*.*.*'
in the entity annotation would be more intuitive, faster, and with no risk of inconsistencies ?

The "config_pattern" directly points to the correct schema entry, and you can deduce the config_prefix from it (the part before the first '.*')

The patch as it stands is only a BC break for those NOT using the new config_export annotation. If that exists, this code never runs.
(Of course, that's brand new and I doubt anyone is using it yet)

Changing config_prefix to config_pattern would be a BC break for everyone.

Honestly I am +1 to being able to look at a config schema and see the entity type it's for. If your entity type has some weird prefix, it can be hard to track backwards from the schema to the exact entity type...

The patch as it stands is only a BC break for those NOT using the new config_export annotation. If that exists, this code never runs.

Well, the new public function getDefinitionForEntityType() breaks if the config schema for your entity type doesn't point back to the entity_type_id. OK, the method is new, but still, the definition of "being a valid config entity for which public methods work" does change, that seems like a BC break to me ?

If your entity type has some weird prefix, it can be hard to track backwards from the schema to the exact entity type...

Yeah, dunno. This is still putitng two separate related informations on both ends of the relationship (config_prefix is on the Entity, entity_type is on the schema, which also implictly includes a config_prefix, which might or might not be the same). Feels like a convoluted denormalized mental model, esp. when one single information in one place would be enough :-)

+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
@@ -159,8 +168,26 @@ public function getDefinition($base_plugin_id, $exception_on_invalid = TRUE, $is
+      $this->entityTypeDefinitions[$entity_type_id] = $this->getDefinition($id);

let's just store the definition ID instead so we don't have unnecessary duplication between $this->definitions and $this->entityTypeDefinitions.

#16, I'll take a look at rewriting things in the other direction tomorrow.

#17, sure, okay

+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
@@ -169,7 +169,7 @@ public function getDefinition($base_plugin_id, $exception_on_invalid = TRUE, $is
+    if (TRUE || !isset($this->entityTypeDefinitionIds[$entity_type_id])) {

debug?

Reroll.

Updated https://www.drupal.org/node/2481909 because it didn't link to this issue despite claiming that this was already supported.

I ran into this at #2977669-55: Spec Compliance: some entity types have an "id", "type" or "uuid" field, this violates the spec.

We definitely do fallback to using schema but this happens in ::toArray(). See

    $properties_to_export = $entity_type->getPropertiesToExport();
    if (empty($properties_to_export)) {
      $config_name = $entity_type->getConfigPrefix() . '.' . $this->id();
      $definition = $this->getTypedConfig()->getDefinition($config_name);
      if (!isset($definition['mapping'])) {
        throw new SchemaIncompleteException("Incomplete or missing schema for $config_name");
      }
      $properties_to_export = array_combine(array_keys($definition['mapping']), array_keys($definition['mapping']));
    }

However I suspect this issue is a duplicate of #2666392: Unable to revert third party settings via config import and that that issue will fix this and the problems seen in #2977669: Spec Compliance: some entity types have an "id", "type" or "uuid" field, this violates the spec

@alexpott, toArray won't work for JSON API because we're iterating over all config entity types and trying to set up field aliases during a router rebuild, meaning that we don't have any individual entities "in hand", we're operating on the type level alone.

#2666392: Unable to revert third party settings via config import actually exacerbated the issue: See: #2986899: Drupal core compatibility: SchemaIncompleteException for some config entity types

UPDATE: I read more into the issue and realized that the schema fallback was moved to getPropertiesToExport, so it does seem like this is now a duplicate.

We still have the same fundamental issue though. We don't have an entity in hand, which would be needed in order to pass a value for the $id argument.

One problem here is that the schema often depends on the data - for example a views schema depends on the display plugins etc. even the top level elements in a config entity can be dynamically determined based on the data.

One problem here is that the schema often depends on the data - for example […]

Darn, indeed! :( However, top-level keys' schema remains the same AFAIK. And those are the "properties to export".

Let's continue this discussion at #2666392-53: Unable to revert third party settings via config import and close this as a duplicate?

Re-titling to scope this issue more tightly to the problem that now needs to be solved.

Okay, so this isn't a duplicate of #2666392: Unable to revert third party settings via config import after all. Thanks for clarifying!

This is becoming relevant again now that #2164373: [META] Untie config validation from form validation — enables validatable Recipes, decoupled admin UIs … is moving forward again 🤓

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I started working on #2300677: JSON:API POST/PATCH support for fully validatable config entities and encountered this. Adding one more tag to improve discoverability.