Third party settings dependencies cause config entity deletion

Can we add the

+++ b/core/config/schema/core.data_types.schema.yml
@@ -250,6 +250,11 @@ config_entity:
+        - type: fixme.third_party.[%key]

Yeah, this. No idea how we can automate that key? There is an alter hook now, that might help?

Shameless kind-of-related plug about adding new "system" properties to all config entities : #2362317: No namespacing of "system" properties in ConfigEntities / yaml files means lockdown after 8.0 is out .

I think this is more than normal - it opens lots of possibilities in contrib we can't even imagine now

You'll still need to define the third party settings on each schema. You SHOULD NOT use dynamic schema generation if you want translation integration at all. Localize.drupal.org will not be able to set up sample sites so they have your runtime to reproduce the dynamic schema. It needs to be statically explorable.

Right, that's the "fixme" here :

+++ b/core/config/schema/core.data_types.schema.yml
@@ -269,6 +264,11 @@ config_entity:
+    third_party_settings:
+      type: sequence
+      label: 'Third party settings'
+      sequence:
+        - type: fixme.third_party.[%key]

We can provide a generic entry that describes the 'third_party_settings' entry as a sequence, but I'm not sure there's a way to provide the name of the entry for the elements in that sequence - that name is up to each config entity :
theme_settings.third_party.[%key]
field_config.third_party.[%key]
entity_view_display.third_party.[%key]
contact_form.third_party.[%key]
...

BTW, found this when looking in our current uses of ThirdPartyDisplay : #2397807: EntityDisplay schema for third_party_settings is wrong

It is also possible to add some clue to the **data** of each config entity about their type, so the third party schema type can feed from that. Eg. an entity_type key or a config_prefix key. Then the config schema piece can be defined based on that and there would be no need to define it on each entity separately. However that requires more data to be stored to inform the schema. That would of course be statically explorable because both the data and schema are available.

I don't really get why we can't explore it statically based on the schema, because we do have that available :)

Anyway, I think we should only do this if we can find a way for the schema to just work? If not, then each entity type will still have to add the schema and it might look as if it works but then the data will not be saved or will not validate..

@Berdir: what would be the schema for the third party settings then if we don't add that?

Fixed the schema issue and third_party_settings are only present if the config entity has them. Added a new ability to traversing schema - you can use the definition type.

Nice :)

Should ConfigEntityInterface also implement ThirdPartyInterface then?

Fix tests (hopefully) - couple of mistakes in the schema changes. And realised that we need to deal with dependencies - all third party dependencies can be removed when the module that provides them is uninstalled.

The onDependencyRemoval changes move this into bugfix and potentially critical - since uninstalling a module like content_translation is at this point almost impossible without deleting a tonne of configuration. It could be split off into a separate patch but making every config entity have third party settings makes the problem even more serious.

Should ConfigEntityInterface also implement ThirdPartyInterface then?

Maybe - although it's not mandatory so...

Was just about to post a patch with the same config schema fixes, without the dependency stuff ;)

+++ b/core/config/schema/core.data_types.schema.yml
@@ -268,7 +273,7 @@ config_entity:
+        - type: '[%parent.%definition_type].third_party.[%key]'

+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
@@ -238,6 +238,7 @@ protected function replaceName($name, $data) {
+   * - '%definition_type', to reference the definition type of the parent.

Woo, magic. Why not just %type? Did you want to emphasize that its not the resolved type (eg. mapping?).

Improved name after discussion with @Gábor Hojtsy in IRC.

Changing to a bug since the dependency removal part of this really is fixing a big bug. Yes it could be fixed separately and individually for each config entity that currently implements third party settings but I think this is an acceptable shortcut. Because with this change we get horizontally extensible config entities with third party settings that are both deployable and translatable.

The renamed key looks better to me, thanks!

Yay, patch looks awesome.
Funny to think that after all this time, we've come around to "all config entities are extensible" :-)

1. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
   @@ -221,7 +221,9 @@ public function uninstall($type, $name) {
          // Inform the entity.
   -      $entity->onDependencyRemoval($affected_dependencies);
   +      if ($entity->onDependencyRemoval($affected_dependencies)) {
   +        $entity->save();
   +      }
   

   Code now does a bit more than the comment says :-)

   Other than that, double yay for centralizing this behavior, since apparently some of the ConfigEntity types with ThirdParty failed to to it correctly even in core ?

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
   @@ -170,6 +170,9 @@ public function calculateDependencies();
   +   * @return bool
   +   *   TRUE if the entity has changed, FALSE if not.
   

   Not sure I'd now what to do with that info. Could we add a hint of the actual effect it has ? "TRUE if needs to be re-saved in reaction to the change" ?

3. +++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
   @@ -238,6 +238,8 @@ protected function replaceName($name, $data) {
   +   * - '%parent_type', to reference the schema definition type of the parent.
   +   *   Can only be used in combination with %parent.
   
   @@ -247,6 +249,9 @@ protected function replaceName($name, $data) {
   +   * - '%parent_type', will be replaced by the schema type of the parent.
   

   Seems contradictory ? "can only be used in combination with %parent", yet we give an example where it's used without %parent ?

   It seems a bit inconsistent / confusing to have a %property that encapsulates going up one level *and* reading a specific property.

   I might be missing something, but since we already have %parent to go one level, and already build syntax on that (%parent.foo, %parent.%key...) can't we just support '%type' for "the schema type of the *current* level" ? Then if you want the type of the parent, you do %parent.%type, and that's consistent with the rest of the syntax ?

4. Just wondering : is it worth keeping the trait, if ConfigEntityBase uses it ? What other classes might want to use it ?

@yched - thanks for the review.

1. Fixed and added a test for content_translations usage.
2. Fixed
3. Fixed now it is just %type but you have to use %parent - it actually makes the code simpler!
4. Agree - I've removed it but kept the interface.

I've refocused the issue on the bug that it is solving. We are solving by generalising the functionality.

Thanks @alexpott :-)

it is just %type but you have to use %parent

No biggie, but not sure why we need to have this restriction ? Can't we handle it just like %parent and %key : set the initial value in buildDataDefinition(), and then update it in replaceVariable() iif we move one parent up ?

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
@@ -171,7 +171,8 @@ public function calculateDependencies();
+   *   TRUE if the entity has changed, FALSE if not. If this method returns TRUE
+   *   then the entity needs to be re-saved for the changes to take effect.

Sorry for being a pain: not sure it's fully clear as to who's expected to do the save(), the implementor of onDependencyRemoval(), or the caller.

@yched - well we're trying to determine the type of the current level so that be super recursive and unfathomable :)

I've fixed the documentation.

Created a draft CR - https://www.drupal.org/node/2419827

@alexpott : oh, right, buildDataDefinition() cannot initialize the %type for the current value, because it doesn't have a TypeData object for that value, only for the $parent ?

Anyway. Looks RTBC to me ?

Minor conflict with #2414991: Prevent hook_config_schema_info_alter from adding or removing definitions in config_schema_test.schema.yml.

Also near-posthumously bumping this to critical/D8 upgrade path, this could end up with horrible data loss.

Committed/pushed to 8.0.x, thanks!

Should ConfigEntityInterface also implement ThirdPartyInterface then?

Maybe - although it's not mandatory so...

Yes, I think it is, it does not make sense without that: #2425637: ConfigEntityInterface should extend ThirdPartySettingsInterface

Opened a small followup to remove one extra trait reference that wasn't removed: #2427349: Remove ThirdPartySettingsTrait leftover in ConfigEntityBase

Published change record. We should maybe do something with the other one https://www.drupal.org/node/2419827?