Unable to revert third party settings via config import

Rerolled the patch from #6 which solves the problem for me (and applies cleanly both to latest 8.1.x and 8.2.x).

I think this is exposing a bug in ConfigEntityStorage::updateFromStorageRecord() it is not copying over the values from the new entity to the existing entity properly.

This solution is completely different from the the solution in #3 because it is a generic solution. Interdiff is to #2 - the test only patch.

For me this is at least a major bug because config imports produce an unexpected result - i.e. you can't remove the data. Also realised the new method on ConfigEntityBase needs to go on the interface. The reason it is underscored is because ConfigEntityBase will be one of the most extend classes we have and there is no point breaking existing code.

Every time I change this interface I see this fail in ViewUI :)

Also we don't need to be mucking with uuid fields in updating an entity.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -267,15 +267,7 @@ public function toArray() {
        $entity_type = $this->getEntityType();
    
   -    $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']));
   -    }
   +    $properties_to_export = $this->_getPropertiesToExport();
   

   There are some config entities that override toArray() and do special things.

   If they add/remove properties completely, could that cause a problem in that the new method would not work as expected? Haven't really thought it through or identified a specific problem, just want to make sure that we don't introduce bugs for them.

   Some of the overrides might also no longer be necessary, some example implementations:

   EntityDisplayBase, just removes sub-keys
   Flag (unecessary I think)
   Currency/CurrencyLocale (not sure, many things could be removed probably but Xano likes to do special things ;))
   search_api Server
   FilterFormat (the roles thing is both unproblematic here I think and weird in general)

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -267,15 +267,7 @@ public function toArray() {
        foreach ($properties_to_export as $property_name => $export_name) {
   @@ -299,6 +291,25 @@ public function toArray() {
   
   @@ -299,6 +291,25 @@ public function toArray() {
      }
    
      /**
   +   * {@inheritdoc}
   +   */
   
   +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
   @@ -217,4 +217,21 @@ public function trustData();
   +   *
   +   * @todo Remove underscore in Drupal 9.
   +   */
   +  public function _getPropertiesToExport();
   

   I think we did add new methods without underscore in minor versions before but probably none that are extended so often.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
@@ -217,4 +217,21 @@ public function trustData();
+  public function _getPropertiesToExport();

Can we add this to a new, optional interface?

@Xano do you have config entities not extending from ConfigEntityBase?

@alexpott: I haven't, but it may break tests using Prophecy (unmocked method calls raise errors). It's technically a BC break, though, and I'm wondering if there is a feasible way to avoid that, and make it pretty again in D9.

We explicitly stated that EntityInterface are subject to change and that you should inherit from the base classes and that we would change these interfaces by making additions in minors.

If they add/remove properties completely, could that cause a problem in that the new method would not work as expected? Haven't really thought it through or identified a specific problem, just want to make sure that we don't introduce bugs for them.

That's a totally valid question to think about.
I think this should not be an issue. When the configuration is removed, it won't be part of the schema, as it will never be exported to YML files in the first place, so the expected data doesn't change.

Can we add this to a new, optional interface?
@Xano do you have config entities not extending from ConfigEntityBase?

I doubt this kind of change will land in a patch release anyway. When its not a patch release we have clearly stated that adding methods is possible.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
   @@ -217,4 +217,21 @@ public function trustData();
   +   *
   +   * @todo Remove underscore in Drupal 9.
   +   */
   +  public function _getPropertiesToExport();
   

   Personally as long we do that in a minor release, not using the underscore would be find.

2. +++ b/core/modules/system/src/Tests/Entity/ConfigEntityImportTest.php
   @@ -172,6 +173,33 @@ protected function doSearchPageUpdate() {
   +    $custom_data = $this->container->get('config.storage')->read($name);
   ...
   +    $original_data = $this->container->get('config.storage')->read($name);
   

   Can we use '.sync' to make this test code more clear?

I ran into a problem where we removed field groups from our node display but during configuration import on dev and production, it would not import. I wouldn't get any errors or anything.

Patch #17 fixed the issue for me. (Thanks @Berdir for pointing me here from my Drupal Answers question).

Not sure if I should mark this as RTBC or Needs Work. (I'm just not a big fan of having the underscore in _getPropertiesToExport()).

Re #26 - thanks for the review. I've fixed point 1. Re point 2. I'm not sure what you are getting at \Drupal\Tests\system\Functional\Entity\ConfigEntityImportTest::assertConfigUpdateImport() works by writing to the sync directory. I've improved the variable names - hopefully that helps.

This looks great for me!

+++ b/core/modules/system/tests/src/Functional/Entity/ConfigEntityImportTest.php
@@ -187,16 +187,17 @@ protected function doThirdPartySettingsUpdate() {
+    // Ensure that a configuration import can remove a third party settings.

I guess this should be singular here? A third party setting.

Fixed #31 = thanks @dawehner

hmmm... I've looked at this patch some more and I'm uncomfortable with adding getPropertiesToExport() to the ConfigEntityInterface when we have exactly the same method on the ConfigEntityTypeInterface. I think what we should do is move the config schema checking fallback to the ConfigEntityType implementation and deprecate it - basically make the config_export mandatory in Drupal 9. This is good for performance and doing things one way.

Rerolled patch against 8.4.5, cuz I need it.

Thanks for reminding me of this issue! Here's a patch with fixed tests.

Let's move the deprecation to another issue so this can be just about fixing the bug. See #2949021: Deprecate schema fallback in ConfigEntityType::getPropertiesToExport

Created a change record - https://www.drupal.org/node/2949023

Hi,

What's the status on this one? I am Running Drupal 8.5 in production, and got stung by this with the DisplaySuite module!

Can I use the patch in #37, #38 or #39 in 8.5?

Cheers,
Michel

Rerolled.

@mdrescher the latest patch is probably the one to use. Out of the three you've asked about #39.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
@@ -450,9 +450,16 @@ public function updateFromStorageRecord(ConfigEntityInterface $entity, array $va
-    foreach (array_keys($values) as $property) {
...
+    foreach ($entity_type->getPropertiesToExport($updated_entity->get($id_key)) as $property) {

It's because we're using the annotation (or schema) to get the list of properties to update. So both UUID and third_party are in there even if they are not in the incoming values. Without this change Drupal\KernelTests\Core\Config\ConfigImporterTest will fail.

That said the uuid key is hardcoded in config storage so let's use that. Config Entities need to have a key of uuid - there's just no way to use a different value and the configuration system work. So there's one little bit of tidy up we can do.

I've also improved the comment to explain how this occurs.

@alexpott, comment #43

In the end I didn't use the patch, but got rid of the settings through manual intervention on PROD and committing to master, which isn't ideal, but it worked.

Thanks for working on this!

Two minor issues, otherwise I think this is OK. It's hard to follow with the annotation and configuration schema fallback but agreed deprecating it in a follow-up is the best option here.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityType.php
   @@ -142,30 +143,40 @@ protected function checkStorageClass($class) {
   +      $definition = \Drupal::service('config.typed')->getDefinition($config_name);
   
   +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityTypeInterface.php
   @@ -65,11 +65,22 @@ public function getConfigPrefix();
   +   *   Thrown when the configuration entity type does not have the annotation or
   

   Is this not injected because we want to remove the code path? If so is it worth a comment to say that explicitly

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityTypeInterface.php
   @@ -65,11 +65,22 @@ public function getConfigPrefix();
       *
   +   * If the config entity properties are not declared it will fallback to
   +   * determining the properties using configuration schema.
   +   *
   

   We should reverse the sentence, the 'it' is a bit confusing in there.

   "Falls back to determining the properties using configuration schema, if the config entity properties are not declared".

Re #49.1 Not really - it's because service injection into an entity type class it not possible. See \Drupal\Core\Entity\EntityType::getBundleConfigDependency() and \Drupal\Core\Entity\EntityType::getBundleLabel().

Fixed .2

Committed b55ed6b and pushed to 8.6.x. Thanks!

Didn't this introduce a subtle BC break?

Per the CR (https://www.drupal.org/node/2949023):

In Drupal 8.6 all of the functionality is present in ConfigEntityType::getPropertiesToExport() which now optionally is passed the configuration entity ID. Without the ID we cannot fallback to schema.

But "properties to export" only is referring to top-level schema keys, and those are always the same, aren't they? There are situations where you don't have a configuration entity ID to pass in, yet still need the schema fallback — for example when determining which top-level keys ("fields") will be exposed via REST API or JSON API or GraphQL for a configuration entity type.

(We encountered this in #2977669: Spec Compliance: some entity types have an "id", "type" or "uuid" field, this violates the spec. First reported at #2483407-33: Allow configuration schema fallback in ConfigEntityType::getPropertiesToExport() to work without an ID.)

@Wim Leers nope this didn't introduce any BC - it was the same before this patch was committed. If we didn't have an ID then schema fallback didn't weork. Please don't re-open this issue. Let's continue to discuss the problem in #2483407: Allow configuration schema fallback in ConfigEntityType::getPropertiesToExport() to work without an ID.

Okay, so this didn't turn #2483407: Allow configuration schema fallback in ConfigEntityType::getPropertiesToExport() to work without an ID into a duplicate of this after all. Thanks for clarifying!

Sorry for reopening — looking back, I don't know how I concluded this was a subtle BC break… 🤔

P.S.: this issue's CR was not yet published — did that.

Update: apparently there is a subtle BC break, but a different one than I described 😅

See #2986901: Followup for #2666392: ConfigEntityTypeInterface::getPropertiesToExport no longer returns NULL, throws exception instead.