Make it easy to get typed config representations of entities

--- a/core/lib/Drupal/Core/Config/TypedConfigManagerInterface.php
+++ b/core/lib/Drupal/Core/Config/TypedConfigManagerInterface.php
@@ -72,4 +73,25 @@ public function hasConfigSchema($name);
+  public function getTypedDataForConfig(ConfigBase $config);
...
+  public function getTypedDataForConfigEntity(ConfigEntityInterface $config_entity);

Is it okay to expand this interface? Is that not a BC break?

It's not marked @internal. Should it be?

I went through the actual code base looking for places this could be used, and came up wanting.
Most callers of this pattern already have the config name and data split out.
Here's an example of my thinking.

Also I think the two new getFor* methods should be createFrom*, since the final call is a create.

+1

I think \Drupal\locale\LocaleConfigManager::getTranslatableDefaultConfig() and \Drupal\Core\Config\TypedConfigManager::get() itself should also be changed, and then this is done.

This looks great!
Thanks @dawehner

1. +++ b/core/modules/locale/src/LocaleConfigManager.php
   @@ -140,12 +140,8 @@ public function getTranslatableDefaultConfig($name) {
   -      $typed_config = $this->typedConfigManager->create($data_definition, $data);
   -      if ($typed_config instanceof TraversableTypedDataInterface) {
   -        return $this->getTranslatableData($typed_config);
   ...
   +      $typed_config = $this->typedConfigManager->createFromNameAndData($name, $data);
   +      return $this->getTranslatableData($typed_config);
   

   This seems like a behavior change. As this is only ever called with (top-level) config objects, this seems safe, but also kind of unnecessary to do here?

2. So we already have the following in Drupal\Core\Entity\Entity:

     public function getTypedData() {
       if (!isset($this->typedData)) {
         $class = \Drupal::typedDataManager()->getDefinition('entity')['class'];
         $this->typedData = $class::createFromEntity($this);
       }
       return $this->typedData;
     }
   

   I like the method being introduced here, but shouldn't ConfigEntityBase somehow override this method? Or instead should we add a ConfigEntityAdapter or something like that? Or is that left for a followup?

I'm fine with a follow-up for that. I don't have a strong opinion in either direction, but I think we should either deprecate EntityInterface::getTypedData() wholesale or fix it for ConfigEntityBase in that follow-up. Opening that follow-up should block this issue, though, IMO.

Thank you! Let's get this in, then.

Hmm... that's a good question. I think in an ideal world my answer would be a different one, but since content entities also don't validate on save, I think we are well advised to follow suit for config entities. On the other hand, if I read the current code correctly we already pass $has_trusted_data = FALSE to Config::save() when saving configuration entities. So if we do constraint validation if $has_trusted_data is FALSE we would automatically trigger it for config entities by default. On the third hand I'm not sure if that wouldn't be considered a BC-break. On the fourth hand I guess currently no one will actually have constraints in their schema at the moment, so maybe we can get away with it? And also maybe we even if it's a slight BC break it's acceptable because the only thing that breaks is if people are saving invalid configuration, so it's good that we prevent it?

As you can see, not really sure...

Agreed with #17. @tstoeckler++

I wonder what @alexpott's thoughts about this are.

But since content entities also don't validate on save, I think we are well advised to follow suit for config entities

Just chiming in to point out that I'm actually trying to change that behavior to some extent: #2847319: Always enforce basefield and entity-level constraints

Let's not use a mistake we made in one place as an excuse to repeat it in another :) For all we know, we might fix the fact that content entities don't validate on save and then have a broken config entity save because we "just went along with it".

#2300677: JSON:API POST/PATCH support for fully validatable config entities is major, so bumping this to major too.

This patch looks pretty great to me. I still want to read the comments in this issue before committing to make sure I'm not missing anything. In the meantime, I noticed the following unused use statements. Just pointing them out in case they reflect something that should be in the patch but isn't.

+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
@@ -4,6 +4,7 @@
+use Drupal\Core\Config\Entity\ConfigEntityInterface;
...
+++ b/core/lib/Drupal/Core/Config/TypedConfigManagerInterface.php
@@ -2,6 +2,7 @@
+use Drupal\Core\Config\Entity\ConfigEntityInterface;
...
+++ b/core/tests/Drupal/KernelTests/Config/TypedConfigTest.php
@@ -2,7 +2,9 @@
+use Drupal\config_test\Entity\ConfigTest;

I think we should consider the TypedConfigManager largely as an internal object and certainly not an extension point. We're adding a method to its interface here. I think we should have a CR to tell people about the nee method.

Theoretically we could avoid the new method by having a new static method which gets the typed config manager in as first method parameter.

I don't think doing so is necessary, since adding methods to interfaces in minor releases is explicitly allowed for in our BC policy. However, a static method might actually make sense in this case, since it doesn't seam to me to have an implementation that would ever need swapping. In other words, if it truly is merely a utility wrapper around three other interface methods, then a static method kind of makes sense IMO, and is not "super ugly".

I'm ok with either approach though, so I leave it to those who've worked on this patch as to whether to update the patch to use a static method instead of an interface method, or whether to write a CR to inform people about the interface addition.

I read through the issue comments and looks to me like all points are addressed (except per #28, doing one of either #26 or #27).

#17 hasn't been addressed, but that discussion is also taking place in #2869792: [meta] Add constraints to all config entity types, so if we need to open a new issue for it, I think it makes sense to do so as a related issue of that one, not this one.

+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
@@ -387,4 +386,23 @@ protected function alterDefinitions(&$definitions) {
+   * @return \Drupal\Core\TypedData\TraversableTypedDataInterface
+  public static function createFromNameAndData($config_name, array $config_data, TypedConfigManagerInterface $typed_config_manager = NULL) {

I think what I like least about #30 is that it's a static function with a name that starts with "create", but returns an instance of something entirely different than a typed config manager. I think all our other static "create" functions in core return an instance of the class they're on (or at least an instance of a very similar class).

If someone thinks of a more natural home for this static method, great, let's explore that.

Otherwise, I think I prefer #12 as well, despite it needing a CR.

If someone thinks of a more natural home for this static method, great, let's explore that.

What about Drupal\Core\Config\Schema\Mapping::createFromNameAndData()? Or is there a case where createFromNameAndData() creates something other than a Mapping instance?

Or maybe Drupal\Core\Config\Schema\ArrayElement instead of presuming it's a Mapping?

#12 was RTBC'd by @tstoeckler in #15. Do we want to re-upload #12 and re-RTBC it?

Ok, so we do. Great!

• Test failure is due to #2870878: Add config validation for UUIDs. Rerolled for that.
• Removed the unused use statements per #25.
• Edited the title of the change record from "Add..." to "Added...".

Ticking credit box for @tstoeckler for his reviews.

:)

Committed a9fe99c and pushed to 8.4.x. Thanks!

Yay, #2300677: JSON:API POST/PATCH support for fully validatable config entities is now unblocked!