Problem/Motivation

We have a powerful ThirdPartySettingsTrait that allows config entities to hold information of settings of third party modules. However when a module (eg. content_translation) that uses this is uninstalled the config entities that have third party settings will be removed.

Only a couple of config entities in core already implement ThirdPartySettingsTrait, but ideally any config entity that exist should have this out of the box. This way any config entity in core and contrib will have this by default.

Proposed resolution

Remove the trait and implement the functionality in ConfigEntityBase. Add the ability for third party settings to be removed from config entities on module uninstall through a generic onDependencyRemoval on ConfigEntityBase. This has the powerful side effect of making all configuration entities horizontally extensible through third party settings that are both deployable and translatable.

Remaining tasks

Review
Commit

User interface changes

None

API changes

  • Removed ThirdPartySettingsTrait
  • Adds the ability to dynamically set config schema types using the schema definition type using the
    %parent_type<code> placeholder.
    <code>
    config_entity:
      type: mapping
      mapping:
    //...
        third_party_settings:
          type: sequence
          label: 'Third party settings'
          sequence:
            - type: '[%parent.%parent_type].third_party.[%key]'
    

    Resolves to a type based on the config entity schema. So for example field config entities resolve to field.field.*.*.*.third_party.content_translation

Files: 
CommentFileSizeAuthor
#35 2361775.35.patch39.85 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,931 pass(es). View
#30 2361775.30.patch39.84 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2361775.30.patch. Unable to apply patch. See the log in the details link for more information. View
#30 27-30-interdiff.txt1.83 KBalexpott
#27 2361775.27.patch38.89 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,866 pass(es). View
#27 22-27-interdiff.txt17.33 KBalexpott
#27 2361775.27.test-only.patch1.63 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,857 pass(es), 1 fail(s), and 0 exception(s). View
#22 2361775.22.patch27.98 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,869 pass(es). View
#22 19-22-interdiff.txt3.15 KBalexpott
#19 2361775.19.patch27.42 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,617 pass(es). View
#19 16-19-interdiff.txt9.23 KBalexpott
#16 2361775.16.patch20.92 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#16 8-16-interdiff.txt12.08 KBalexpott
#8 2361775-8.patch10.79 KBswentel
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#1 2361775-1.patch11.33 KBswentel
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,547 pass(es), 165 fail(s), and 0 exception(s). View

Comments

swentel’s picture

Status: Active » Needs review
FileSize
11.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,547 pass(es), 165 fail(s), and 0 exception(s). View
swentel’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 1: 2361775-1.patch, failed testing.

Berdir’s picture

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?

yched’s picture

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 .

larowlan’s picture

Priority: Normal » Major

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

swentel’s picture

Status: Needs work » Needs review
FileSize
10.79 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

reroll to begin with - thinking about the fix now

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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.

yched’s picture

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]
...

yched’s picture

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

The last submitted patch, 8: 2361775-8.patch, failed testing.

Gábor Hojtsy’s picture

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.

Berdir’s picture

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..

Gábor Hojtsy’s picture

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

alexpott’s picture

Status: Needs work » Needs review
FileSize
12.08 KB
20.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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.

Status: Needs review » Needs work

The last submitted patch, 16: 2361775.16.patch, failed testing.

Berdir’s picture

Nice :)

Should ConfigEntityInterface also implement ThirdPartyInterface then?

alexpott’s picture

Status: Needs work » Needs review
FileSize
9.23 KB
27.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,617 pass(es). View

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...

Berdir’s picture

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

Gábor Hojtsy’s picture

+++ 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?).

alexpott’s picture

Category: Task » Bug report
FileSize
3.15 KB
27.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,869 pass(es). View

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.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

The renamed key looks better to me, thanks!

yched’s picture

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 ?
    alexpott’s picture

    Title: Move ThirdPartySettingsTrait to ConfigEntityBase » Third party settings dependencies cause config entity deletion
    Issue summary: View changes
    Issue tags: +Configurables, +Needs change record
    FileSize
    1.63 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,857 pass(es), 1 fail(s), and 0 exception(s). View
    17.33 KB
    38.89 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,866 pass(es). View

    @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.

    The last submitted patch, 27: 2361775.27.test-only.patch, failed testing.

    yched’s picture

    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.

    alexpott’s picture

    FileSize
    1.83 KB
    39.84 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2361775.30.patch. Unable to apply patch. See the log in the details link for more information. View

    @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.

    alexpott’s picture

    yched’s picture

    Status: Needs review » Reviewed & tested by the community

    @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 ?

    catch queued 30: 2361775.30.patch for re-testing.

    Status: Reviewed & tested by the community » Needs work

    The last submitted patch, 30: 2361775.30.patch, failed testing.

    alexpott’s picture

    Status: Needs work » Reviewed & tested by the community
    FileSize
    39.85 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,931 pass(es). View
    catch’s picture

    Priority: Major » Critical
    Issue tags: +D8 upgrade path

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

    catch’s picture

    Status: Reviewed & tested by the community » Fixed

    Committed/pushed to 8.0.x, thanks!

    • catch committed 02b9e08 on 8.0.x
      Issue #2361775 by alexpott, swentel: Third party settings dependencies...
    Berdir’s picture

    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

    pcambra’s picture

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

    penyaskito’s picture

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

    Status: Fixed » Closed (fixed)

    Automatically closed - issue fixed for 2 weeks with no activity.