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
Comment | File | Size | Author |
---|---|---|---|
#35 | 2361775.35.patch | 39.85 KB | alexpott |
#30 | 2361775.30.patch | 39.84 KB | alexpott |
#30 | 27-30-interdiff.txt | 1.83 KB | alexpott |
#27 | 2361775.27.patch | 38.89 KB | alexpott |
#27 | 22-27-interdiff.txt | 17.33 KB | alexpott |
Comments
Comment #1
swentel CreditAttribution: swentel commentedComment #2
swentel CreditAttribution: swentel commentedComment #3
swentel CreditAttribution: swentel commentedComment #5
BerdirCan we add the
Yeah, this. No idea how we can automate that key? There is an alter hook now, that might help?
Comment #6
yched CreditAttribution: yched commentedShameless 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 .
Comment #7
larowlanI think this is more than normal - it opens lots of possibilities in contrib we can't even imagine now
Comment #8
swentel CreditAttribution: swentel commentedreroll to begin with - thinking about the fix now
Comment #9
Gábor HojtsyYou'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.
Comment #10
yched CreditAttribution: yched commentedRight, that's the "fixme" here :
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]
...
Comment #11
yched CreditAttribution: yched commentedBTW, found this when looking in our current uses of ThirdPartyDisplay : #2397807: EntityDisplay schema for third_party_settings is wrong
Comment #13
Gábor HojtsyIt 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.
Comment #14
BerdirI 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..
Comment #15
Gábor Hojtsy@Berdir: what would be the schema for the third party settings then if we don't add that?
Comment #16
alexpottFixed 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.
Comment #18
BerdirNice :)
Should ConfigEntityInterface also implement ThirdPartyInterface then?
Comment #19
alexpottFix 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.
Maybe - although it's not mandatory so...
Comment #20
BerdirWas just about to post a patch with the same config schema fixes, without the dependency stuff ;)
Comment #21
Gábor HojtsyWoo, magic. Why not just %type? Did you want to emphasize that its not the resolved type (eg. mapping?).
Comment #22
alexpottImproved 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.
Comment #23
alexpottComment #24
alexpottComment #25
Gábor HojtsyThe renamed key looks better to me, thanks!
Comment #26
yched CreditAttribution: yched commentedYay, patch looks awesome.
Funny to think that after all this time, we've come around to "all config entities are extensible" :-)
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 ?
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" ?
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 ?
Comment #27
alexpott@yched - thanks for the review.
%type
but you have to use%parent
- it actually makes the code simpler!I've refocused the issue on the bug that it is solving. We are solving by generalising the functionality.
Comment #29
yched CreditAttribution: yched commentedThanks @alexpott :-)
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 ?
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.
Comment #30
alexpott@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.
Comment #31
alexpottCreated a draft CR - https://www.drupal.org/node/2419827
Comment #32
yched CreditAttribution: yched commented@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 ?
Comment #35
alexpottMinor conflict with #2414991: Prevent hook_config_schema_info_alter from adding or removing definitions in
config_schema_test.schema.yml
.Comment #36
catchAlso near-posthumously bumping this to critical/D8 upgrade path, this could end up with horrible data loss.
Comment #37
catchCommitted/pushed to 8.0.x, thanks!
Comment #39
BerdirYes, I think it is, it does not make sense without that: #2425637: ConfigEntityInterface should extend ThirdPartySettingsInterface
Comment #40
pcambraOpened a small followup to remove one extra trait reference that wasn't removed: #2427349: Remove ThirdPartySettingsTrait leftover in ConfigEntityBase
Comment #41
penyaskitoPublished change record. We should maybe do something with the other one https://www.drupal.org/node/2419827?