Problem/Motivation
Altering in random settings to a config entity is not a supported way of extending a configuration entity. The entity will not know how to deal with them, we cannot provide configuration schemas for them, if modules are disabled, remains of prior unattended settings may remain, the dependency management is broken in this case, and so on.
The problem with fields is worst. Examples of modules altering in random entity settings for fields include Content translation (the field_sync setting for translatability), Field permissions module, etc. Node already have some way to store third party settings but need unification, that is handled in #2324121: NodeType's settings array was meant to be able to store information from mutliple modules.
Proposed resolution
- Add a generic ThirdPartySettingsInterface, that config entities may implement to support third party settings. Implement this in FieldConfigInterface. Other config entities may implement this later on.
- Add common implementation for this in ThirdPartySettingsTrait, so config entities can take a common implementation for easier developer experience.
- The trait implements settings storage for key/value pairs for each third party module separately under a third_party_settings key in configuration. Add configuration schema coverage for this based on dynamic key typing, so eg. content translation module can add schema for its setting.
- Add all modules that provide third party settings as dependencies to the configuration entity dependency list.
- Remove obsolete code and test workarounds. Add tests.
The solution is a general approach that other configuration entity types may use. Eg. this will be used in #2324121: NodeType's settings array was meant to be able to store information from mutliple modules as well.
Remaining tasks
Review. Commit.
User interface changes
None.
API changes
Field settings schemas changed to include third party settings. New ThirdPartySettingsInterface, ThirdPartySettingsTrait.
Comment | File | Size | Author |
---|---|---|---|
#141 | interdiff.txt | 1.47 KB | Gábor Hojtsy |
#141 | 2224761.141.patch | 30.54 KB | Gábor Hojtsy |
#138 | interdiff.txt | 2.57 KB | effulgentsia |
#138 | 2224761.138.patch | 30.05 KB | effulgentsia |
#131 | interdiff.txt | 1.79 KB | Gábor Hojtsy |
Comments
Comment #1
swentel CreditAttribution: swentel commentedComment #2
jessebeach CreditAttribution: jessebeach commentedComment #3
jessebeach CreditAttribution: jessebeach commentedUnpostponed!
Comment #4
pfrenssenComment #5
pfrenssenAm saving the translation sync settings in 'content_translation.field_settings.{entity}.{bundle}.{field_name}.translation_sync'.
Thanks to @WimLeers for pointing out that content_translation_form_field_ui_field_instance_edit_form_alter() was using the field definition rather than the field instance.
Comment #6
plachEverything looks good aside from this line. Is it really needed?
Comment #7
pfrenssenI just checked and the line is indeed needed. It ensures that the content translation for the entity type is enabled. Without it the test fails.
Comment #8
plachOk, thanks :)
Comment #9
alexpottWe're missing a configuration schema for the new file. Also as this is simple configuration the content_translation module should provide a default config file.
Also I'd swap manage the storage differently.
The file should be like this:
The current structure is:
I don't think fields which have a value of false should be in the file.
Comment #10
plachThe changes proposed by @alexpott make sense to me, sorry for missing the schema issue: I am not really into config yet...
Comment #11
pfrenssenComment #12
pfrenssenAddressed the remarks.
Comment #13
yched CreditAttribution: yched commentedA couple things :
- In D8, a field is identified by [entity_type, field_name], just field_name is not enough. Two different entity types can have fields that are named the same but are completely unrelated - enabling translation sync for one doesn't mean that we intend to enable it for the others.
- Translation sync is currently an instance-level setting - meaning, even within a given entity type, the it is enabled or enabled bundle by bundle
So if we keep the structure proposed in #9, it should be
Comment #14
pfrenssenIt even needs to go one level deeper, as the translation sync is managed on field column group level. Example for an image field:
Luckily it's already implemented like this, it's just the config schema that is wrong.
Comment #15
pfrenssenFixed the config schema and added the default config file that I forgot to add in the previous patch.
Comment #17
pfrenssenI have trouble with the configuration schema. Looking at the other examples in core I think we should alter the structure a bit.
Currently we have:
The problem is that the entity type, bundle and field name are not known and cannot be put in a mapping in the schema. Looking at the other examples in core I think it should be:
Here are some other config schemas that use this pattern:
Comment #18
alexpottThis is just a simple config file so the schema key should start with "content_translation.field_settings" it should then describe the data.
So something like
Comment #19
pfrenssenOh so you can do that :) I did not find any sequences that contained sub-items. Thanks a lot! I will update the patch as well as the config schema documentation.
Comment #20
yched CreditAttribution: yched commentedThis still assumes a translation_sync property directly keyed by field name instead of by [entity type, bundle, field name].
Using the $instance->id() works, since it is === [entity_type].[bundle].[field_name]
It is a bit confusing to see the key built sometimes using $instance->id(), and sometimes by explicit concatenation of $entity_type, $bundle, etc.
For consistency, it would be best to always use the same construct - preferably the latter, since we want to avoid hardcoding on the $instance structure that is specific to "configurable fields". Base fields can be translatable too, so in theory translation_sync could be applied to them too ? (which is a nice gain of moving the translation_sync out of the FieldInstanceConfig CMI structures, suddenly base fields base can have it too)
More generally, it would be cleaner if reading / writing to the config was isolated in dedicated API functions that receive $entity_type, $bundle, $field_name params (or possibly just a FieldDefinition object ?) and take care of the internal structure specifics, instead of having the structure hardcoded in various places.
Comment #21
pfrenssenIt works on content_translation_field_sync_widget() since I am now passing the field instance rather than the field definition. I could not get the bundle from the field definition. I forgot to update the type hint for $field.
But yeah it would be much cleaner and less confusing to use dedicated functions that take care of writing and reading the config. Getting on it right away.
Comment #22
pfrenssenI have been looking into providing translation sync on base fields in addition to field instances but that would take some significant work and seems to be out of scope for this issue. We would need another config schema, and additional logic to find out if we are dealing with a base field or a field instance. Also we need to figure out what to do with the translation sync data stored on the base field, I suppose we can use this as a default every time a new instance is made.
Anyway, that change is not in this patch, but I have addressed the other remarks. I had to use a FieldInstanceConfig rather than a FieldDefinition to be able to get the entity type, bundle and field that are needed to store the config.
Comment #23
effulgentsia CreditAttribution: effulgentsia commentedReroll.
Comment #24
effulgentsia CreditAttribution: effulgentsia commentedRe alexpott's suggestion in #9 and the corresponding implementation in the patches since then: but that then makes this inconsistent with the content_translation.settings structure in HEAD (see content_translation_get_config_key() and the functions that call it). Oh, and not this issue's fault, but that file is missing a default and schema; I wonder what else is: #2231059: [meta] Find out what config schemas are still missing and add them.
Comment #25
effulgentsia CreditAttribution: effulgentsia commentedPer #24, I still think that if we have a content_translation.field_settings.yml config file, then its structure should be consistent with content_translation.settings.yml. I don't have much preference on whether we achieve that by changing the former or the latter. However, another idea: do we really need them as separate files, or can we adjust content_translation.settings.yml to allow for field-level settings inside it?
Comment #26
effulgentsia CreditAttribution: effulgentsia commentedI left this as "needs review" when I wrote #24 and #25, in the hopes others would also weigh in on what approach to take. Trying out "needs work" to see if that helps move this along. Again, my recommendation, per #25, is to merge the field-level settings into content_translation.settings.yml, but if whoever picks this up wants to present reasons to do something else, that's fine too.
Comment #27
alexpottWe need to change this code too...
Comment #28
mlncn CreditAttribution: mlncn commentedComment #29
mlncn CreditAttribution: mlncn commentedReroll + incremental progress, using boolean for value and attempting to address #27. Looking to the bot for non-breakage acceptance and perhaps a human reviewer for help with next steps. For instance, only write an entry if translation is enabled, which means the code in hook_install() can come out entirely?
Comment #31
xjmComment #32
xjmAdding some tags to get more eyes in here. :)
Comment #33
xjmGoing back to a straight rebase of #23, which passes
ContentTranslationSyncImageTest
locally for me. Will still need work for #26 and #27.In general, when rerolling and then adding changes, it's best to provide the reroll first, and then an interdiff with the changes, so that it's easy to see what changes were made and whether any test failures were introduced by them or by the reroll due to changes in HEAD. :)
Comment #34
xjmNW for #26 and #27 now that the bot has picked it up.
Comment #35
YesCT CreditAttribution: YesCT commented#171 in #1067408: Themes do not have an installation status might lend some light here?
Comment #36
alexpottMerge
content_translation.settings
andcontent_translation.field_settings
together and removed the unnecessary hook_install code.The config schema needs work - I could not get
Drupal\content_translation\Tests\ContentTranslationSyncImageTest
to pass with the schema in the patch.Comment #38
alexpottWeird... new patch.
This patch results in
content_translation.settings
like:Comment #39
alexpottComment #41
vijaycs85To fix the schema part of config in #38, we got #2245721: Add missing configuration schema in language component
Comment #42
alexpott@vijaycs85 yes but we're altering the schema here so let's get it done here.
Comment #43
vijaycs85Ok, lets try this one.
Comment #44
alexpottre: #43 this will not work at all. The config object name is content_translation.settings not content_translation.settings.something.something
Fixed the schema - and discovered that the root type of a simple config file can not be a sequence - if it then traversal to discover the inner types does not work! Need to fix that as the entities key added here is superfluous.
Leaving at needs work because testbot is very broken.
Comment #45
alexpottVery weird d.o bugs...
Anyhow new patch which has a valid schema but a superfluous key.
Comment #46
vijaycs85Schema in #45 looks good to me.
Comment #47
alexpottCreated #2248709: The root type of a configuration object can not be a sequence to address the superfluous key issue (is possible)
Comment #48
xjm38: 2224761.38.patch queued for re-testing.
Comment #49
alexpottbots are back
Comment #50
Gábor HojtsyI think the solution looks fine. The use of global functions is not very Drupal 8 like, but aligns with the rest of the approach of content translation AFAIS. It could be in a helper class, but as its integrated with all the other form alters and field info alters which are functions, this fits in.
I'm not very concerned for #2248709: The root type of a configuration object can not be a sequence and its not a beta blocker, so should not block this. It can update the structure of this if there is agreement there, but there does not yet seem to be.
From what I see, this should be good to go.
Comment #51
Gábor HojtsyBTW I looked at concerns expressed by @yched above, and those seem to be all taken into account and implemented as suggested also.
Comment #52
Gábor HojtsyComment #53
Gábor HojtsyCreated a draft change record at https://drupal.org/node/2252851 too :)
Comment #54
catchThis is storing all the sync settings for all field instances in a single item. Are we sure that it's not going to get too large and/or suffer from race conditions?
I couldn't find any code that removes keys from this file when instances are deleted, but might just have missed it.
Comment #55
Gábor HojtsyI don't think we expected any concurrency around this setting but it indeed may happen. Jane may edit article fields while Julio edits commerce product field settings, especially that we have different UIs to edit these settings in the field setting and as one overview. Not sure how best to resolve that, splitting for each field instance into its own config would be rather extreme. In general CMI has a last-one-wins approach where if Jane and Julio both edit the article content type itself, their last edit would win.
Implementing our own locking system for this does not sound very good. Then we would need to handle the locked state on the UI and tell users to try to set their settings again later on. Which can be *very* painful given the all-ecompassing field translatability page where you may have set dozens of things at once and may not remember all the intricacies of your choices to come back and redo.
Any good suggestions?
As for when instances are deleted, field related settings stay around indeed AFAIS from the code.
Comment #56
catchNot convinced that's extreme, it's what I expected this issue to do before I opened the patch, and it's what field instances do too.
This would also simplify tasks like staging partial configuration changes (contrib only but no reason to make that harder).
Also I didn't post this before, but how is a module supposed to ship default configuration for a field instance with translation sync? That's simple enough if it's in one file, but you can't ship part of a file.
Comment #57
Gábor HojtsyWell, so many good reasons indeed.
Comment #58
Gábor HojtsyComment #59
Gábor HojtsyAll right, after more discussion with @alexpott, if we would go to implement each field instance config setting as its own config file then we can just as well make them config entities. Why? We could leverage the dependency system of the config system for deployments, uninstalls, etc. It would make more sense than implementing all that ourselves or leave field content translation sync settings config-synced for fields that don't exist, etc. That sets a pretty dangerous precedent for contrib though where to implement an additional field setting, you would need to implement your own config entity and the number of config entities would explode beyond control.
The only way to get around that in our current system if field instances support pluggable extra configuration like views, which is mostly a container for all kinds of 3rd party configuration. That would let contribs need to add further field settings a possibility to do so as well. Also it would let modules to ship with translation sync for fields to ship them with field settings. Similarly to views, if the plugin is not active for the setting, the embedded setting would just be there and not bothered.
This is the "other way" that I outlined in the issue summary earlier. With the need to split up field sync settings for each instance to its own file, there is a lot more appeal int he plugin approach now.
Retitled the issue for this. Let's figure out if others thing this would be a superior (and feasible) approach to this problem.
Comment #60
effulgentsia CreditAttribution: effulgentsia commentedThe reasoning in #59 makes sense to me. I initially thought creating a plugin system for this would be overkill, compared to just having modules manage everything themselves in their own config files, but the arguments in #55 and #56 to partition such config files on a per-field-instance basis are compelling, thereby making the config entity system compelling, but making every "extra setting" module implement its own config entity type is much more overkill than us creating a plugin system in the existing field instance config entity type. So, yeah, +1 to the new issue title.
Comment #61
yched CreditAttribution: yched commentedI can live with that if people like alex(es) and gabor support it :-)
Views and node types config entities already have this "inner extensibility" feature, #2005434: Let 3rd party modules store extra configuration in EntityDisplay is about adding it to Entity[View|Form]Display too (although in a slightly different way - there, each "component" would need an 'extra_config' entry), so I'm fine with stating that field instances are a valid case too.
A few thoughts though :
Maybe that's not really different from "some EntityViewDisplay includes settings for a formatter provided by a module that doesn't exist", which can already happen currently ?
Gut feeling says "hell no" of course, but not sure which actual rationale we can publicize, and which guideline we can provide to help decide for future cases.
Comment #62
yched CreditAttribution: yched commentedAlso : the current specific case of translation_sync happens to be an "instance-level" extra setting, but if it's worth doing for field_instance_config entities, then the same reasoning also applies to field_config entities...
Comment #63
catchJust a note if we do plugins here, it means we'll have the same issues as #2212081: Remove views optional handler handling for optional stuff that ships in defaults.
Comment #64
effulgentsia CreditAttribution: effulgentsia commentedFrom what I can tell, Views is the only one that actually has it, and only for displays: i.e., "display extenders". But AFAIK, Views doesn't have "field extenders", "sort extenders", etc.
I'm unclear to what degree node types have it. In NodeType.php, all I see is
@todo Pluginify.
above the $settings declaration, with no issue link. And while node.schema.yml contains anode.settings.node
, I don't see any other schema file with anode.settings.SOMETHING_ELSE
.Aside from NodeType and Views discussed above, here are the config entity types we have, by extensibility level:
No extensibility (i.e., no plugins): Breakpoint, BreakpointGroup, (Contact)Category, CustomBlockType, DateFormat, EntityFormMode, EntityViewMode, Language, Menu, RdfMapping, ResponsiveImageMapping, (User)Role, ShortcutSet, Vocabulary
Have plugins, but no cross-plugin extensibility: Action, Block, Editor, FilterFormat, ImageStyle, Migration, SearchPage, Tour
Proposed, but not yet in HEAD, cross-plugin extensibility: EntityFormDisplay, EntityViewDisplay, FieldConfig, FieldInstanceConfig
So, to restate yched's question, do any of the "no extensibility" ones need extensibility, and do any of the middle category ones need cross-plugin extensibility (e.g., should a module be able to add cross-image-effect settings)?
If for D8, the answer is just node types, views displays, entity displays, and fields, then I think that's ok, being as how Drupal's a CMS, and I think those represent the most important building blocks of content management. I don't think there's any clear line though. We could probably come up with use cases for extending all the other ones too, so maybe for each one, there will eventually be a contrib project that subclasses it to add that horizontal extensibility.
Retitling the issue, since we're actually discussing a different kind of extensibility than plugins. Maybe we'll implement it as a plugin type (similar to Views display extenders), but given how few examples we currently have in core that solve this cross-plugin extensibility problem, I don't know that the solution necessarily must be a plugin type.
Comment #65
alexpottI've been working implementing a FieldInstanceExtraSettings plugin type and using it to define a FieldSync plugin in the content translation module - this is all working fine BUT hit a brick wall when I realised that we have a problem for non configurable fields like node title since we have nowhere to store this configuration.
Anyone got any ideas?
Talked @catch, @Gábor Hojtsy and @Berdir in IRC
So the upshot is we're going to create a new config entity to store these extra settings - one that will work for base and configurable fields.
Comment #66
effulgentsia CreditAttribution: effulgentsia commentedHow can we avoid a circular dependency chain between \Drupal\Core\Field and field.module? Right now, the scope of field.module is to enable fields to be added to entities via configuration. Expanding that scope to also include the enhancing of code-defined fields with "extra" configuration seems ok in concept, but IMO, ideally:
- FieldDefinition wouldn't be aware of that.
- baseFieldDefinitions() implementations could still invoke FieldDefinition::create() rather than needing some factory that could swap out FieldDefinition with a subclass from field.module (perhaps some people here disagree with this being a valid goal?).
Comment #67
effulgentsia CreditAttribution: effulgentsia commentedThinking about this a bit more, is it ok in concept? Currently, field.info.yml defines the module as required, but I think we're close to being able to change that, so for a site that only needs code-defined fields, it could be turned off. Seems silly for content_translation.module to then require field.module as a dependency, if all you want on your site is to translate node titles and other code-defined fields.
Which then gets me thinking that to solve this, we'd need a new module, like field_extra_configuration.module (better name TBD). Which makes me want to step back and ask, do we really want such a module in core at this point? Or should we just make content_translation.module handle its own needs with its own config entity, and let contrib figure out a common dependency module that field_validation, field_access, etc. could use or not as they see fit, much like how Entity API evolved in D7 contrib?
Comment #68
alexpottWell i was thinking of creating this in Core and not in the field module.
Comment #69
yched CreditAttribution: yched commented@alexpott : But don't config entities still currently need to be owned by a module ?
Other than that, I'm a bit nervous about adding "support configuration of extra settings for non-configurable fields" in the mix.
- I can't change the 'label' or 'max value' of a base field, because those happen to be native properties that live directly in the FieldDefinition, but I can change 'some_random_3rd_foo_bar' because it's a 3rd party setting ?
Weird situation where extra settings end up being more flexible than native properties, it's actually more handy if a property about a field lives outside its FieldDefinition :-).
- UI impact needs some thought.
Field UI "Manage fields" screen is currently based on the fairly simple notion that non-configurable fields are, well, not configurable :-), and it thus does not list base fields.
Would that screen now be supposed to host the admin forms for those 3rd party settings on base fields ? Coz that's a hell of a lot of rows to add to that admin table on even a bare 'minimal' install, even before you start adding your own custom fields..
I guess that could live to live in a collapsible/collapsed area in the page ?
Or are we talking about providing a ready-to-use / easy-to-load storage container only, with the actual UI left for each module adding 3rd party settings ? (like content_translation or, IIRC, field_permissions do, with dedicated, by-topic pages covering all fields in one page)
Comment #70
Gábor HojtsyWell, for display and form they already have settings. And for translatability they already have field settings. It sounds very plausible to me that field validation or field permissions would expose settings for base fields alike. Weird or not, users are not necessarily aware what base fields are and why would they be limited in terms of translatability, access or validation?
Comment #71
xjmLooks like the issue summary has not been updated for the new direction of the the issue. Can we get an update explaining what the new, broader problem that we're trying to solve is? An example code snippet for the translation synch case would also make it easier to understand.
Comment #72
xjmComment #73
alexpottUpdated issue summary to outline approach agreed discussed by @fago, @plach, and @effulgentsia.
Also this issue should be postponed on #2143291: Clarify handling of field translatability since that moves configurable field translatability config from the FieldConfig to FieldInstanceConfig - which is where content_translation actually assumes it is.
Comment #74
plachThis is a generic discussion about the (entity) field system, let's move it to the proper place so more people can have a look to it.
Comment #75
effulgentsia CreditAttribution: effulgentsia commentedComment #76
plachAdditionally the
EntityManager
should be responsible for instantiating a bundle definition with its overrides, without exposing the cloning process to API consumers. They would just ask a bundle definition which would be instantiated as needed.Comment #77
plachComment #78
jessebeach CreditAttribution: jessebeach commentedComment #79
effulgentsia CreditAttribution: effulgentsia commentedI opened #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields to split out that part of the work. That would then leave this issue with only needing to deal with the plugin-related stuff needed to make 'translation_sync' and contrib use cases work. While this adds to the beta blocker count, I think the split will lead to the issues being completed more efficiently. If you disagree or have concerns about that, please say so.
Comment #80
plachIn my understanding the solution we discussed in Austin did not involve plugins for third-party modules such as Content Translation, but I may have lost some pieces.
@alexpott:
Am I missing something?
Comment #81
effulgentsia CreditAttribution: effulgentsia commentedIt occurs to me that this issue doesn't need to be blocked on either #2143291: Clarify handling of field translatability or #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields. I reworked the summary accordingly, based on the module-as-key rather than plugin-as-key approach alluded to in #80.
Comment #82
xjmPer discussion with @alexpott, let's wait to do this until #2143291: Clarify handling of field translatability is in.
Comment #83
effulgentsia CreditAttribution: effulgentsia commentedI'm fine with leaving this postponed if no one wants to work on this until #2143291: Clarify handling of field translatability is in. However, I see no technical reason for that dependency order. There's no reason this issue can't add a thirdPartySettings property to FieldInstanceConfig and move translation_sync into there, regardless of that issue's fixing of translatable. Translation_sync and translatable are entirely different properties. Or, if there's potential for reroll conflicts with that issue's changes to CT code that's related to translation_sync, then this issue can at least start with a patch that adds thirdPartySettings, and then once #2143291: Clarify handling of field translatability lands, we can iterate this patch to use it for translation_sync. Mentioning this here in case Gábor or someone else wants to start on that, based on #2283977-11: Create a new ConfigEntity type for storing bundle-specific customizations of base fields.
Comment #84
alexpott#83 I guess you're right but it all feels quite tangled up - I'd like both #2143291: Clarify handling of field translatability and #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields done before working on this - so we can make sure that what we do for base fields and configurable fields is the same thing.
Comment #85
xjmComment #86
pwolanin CreditAttribution: pwolanin commentedFrom the issue summary, it's not clear why this is a beta blocker or critical.
Comment #87
Gábor Hojtsy@pwolanin: hopefully my latest update helped?
Comment #88
pwolanin CreditAttribution: pwolanin commented@Gabor - sorry, but not really. Is this going to be an API change that will break existing sites and/or require a re-install?
Comment #89
Gábor Hojtsy@pwolanin: I think we expect it to be an API change / reinstall worth yes. With no concrete solution yet it is impossible to tell 100%. As the issue summary says, 3rd party settings should not just alter in their random stuff, either a plugin container is needed or a 3rd party config file, both of which are significantly different than what we have now.
Comment #90
a.ross CreditAttribution: a.ross commentedBesides the fact that this issue is listed as a beta blocker (only one of 2 remaining), it is also postponed due to #2143291: Clarify handling of field translatability, as per #82. However that issue has actually been fixed a while ago, so shouldn't this be set to active?
Comment #91
andypostComment #92
xjmAs far as I know, the patch in this issue is completely obsolete, so setting to active.
@a.ross, it was blocked on #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields (as per the issue summary), which has only just been fixed today.
Comment #93
a.ross CreditAttribution: a.ross commentedOh well, that is a happy coincidence :)
Comment #94
effulgentsia CreditAttribution: effulgentsia commentedHere's a start. It's incomplete (it adds methods to interfaces without adding corresponding implementations, and it doesn't adjust any UI forms to have a 'third_party_settings' group), but I'm hoping it helps clarify what needs to be done to whoever wants to pick this up and drive it further. You can also see the patch that landed in #2005434: Let 3rd party modules store extra configuration in EntityDisplay for additional guidance on implementation, since this issue is about applying a similar pattern as what was done there for EntityDisplay, but to do it here for FieldConfig.
Comment #95
alexpottDiscussed the following with @xjm:
Discovered #2324121: NodeType's settings array was meant to be able to store information from mutliple modules today. I think we should be doing something generic with this type of third party addition to configuration entities. (The plugin system already handles this for things that are plugins, but these settings are not.) In general, we need a solution for modules that either form alter their own simple settings into the admin form (e.g. the NodeTypeForm) or provide a separate form.
The simple solution is to have an array where third parties can dump information. This array should be keyed by the module name that does the storing. We should use an interface to provide methods to access such settings and use this interface to automate the addition of configuration dependencies.
Interestingly, node type's settings property is already supposed to work this way but the schema is incorrect. And node_uninstall_modules() actually assumes this is the way it is working.
effulgentsia's starter patch maps to this beautifully, but we need to break out FieldConfigInterface::getThirdPartySetting()/FieldDefinion::getThirdPartySetting() into another interface. I will get started on this tomorrow.
Plus we need to get #2260457: Allow config entities to remove dependent configuration keys when dependencies are deleted due to module uninstall done so that uninstalling content_translation does not become impossible.
[Edit: for tone]
Comment #96
yched CreditAttribution: yched commentedGreat initial work in #94, and +1 to #95.
@alexpott: do you think this should be a basic feature of ConfigEntityBase for all config entities, or some common supporting code available as opt-in (in a trait or something) for entity types that want to provide the feature ?
I don't think I get what this refers to ?
Comment #97
xjmOops, that confusion's my fault. Storing extra configuration from plugins on (e.g.) Views is managed by the plugin system. But that solution doesn't apply in this case. My understanding of the point that @alexpott was trying to make is that these settings do not make sense to implement as plugins--way overkill, they don't have the same UI needs, etc.--that we need a different solution for storing the extra configuration than we've used for plugins. And then the rest of the comment describes that solution.
Comment #98
yched CreditAttribution: yched commented@xjm: oh, in the sense that those various extra things would be added through a plugins bag, like a view has filters ?
Yeah, I don't think that would be a good fit here :-)
Comment #99
yched CreditAttribution: yched commentedRegarding the "generic solution" : still +1 on principle, but the case of entity displays is different, as those don't have 1 set of extra settings for the config entity, but 1 for each "field component" present in the display.
Comment #100
alexpottSo the patch attached implements
ThirdPartySettingsInterface
and provides a general implementation for config entities to use withThirdPartySettingsTrait
. Config dependency detection is baked intoConfigEntityBase::calculateDependencies()
. This all leveraged byFieldConfigBase
to allow modules to store additional information on each field.Content_translation
then makes use of all this to store translation sync settings.I don't think FieldDefinition should implement third party settings - if you want to use them just check if the object implements
ThirdPartySettingsInterface
and away you go.This patch stores translation sync settings in field instances like so:
Comment #101
alexpottToo many patches uploaded.
Comment #103
dawehnerWon't third party settings have any kind of dependencies on top of just the module? I could imagine that the third party setting is based upon another config entity.
What about actually implementing the interface ... :)
This line would fail if the third party setting is NULL, which might happen.
Comment #104
alexpott@dawehner #103:
Comment #105
Gábor HojtsyFully updated issue summary, retitled based on the significance of the changes.
Comment #106
Gábor HojtsyAlso deleted the absolutely outdated change notice and wrote https://www.drupal.org/node/2326151.
Comment #107
plachTagging for the rocketship.
Comment #108
Gábor HojtsyFrom #103/#104, I cannot exactly gather if point 3 should be improved or not :) Other than that, what is outstanding? Any concerns? Reviews please!
Comment #109
swentel CreditAttribution: swentel commented@Gabor: I think a simple check to see if $this->thirdPartySettingKey is really an array is fine, after that I think this is ready to go.
Comment #110
Gábor HojtsySo I think you don't want the error to be swallowed, eg. you try to set a value, and no error is sent but silently ignored would not be good. Also the "array" is used in all methods, so we would need to ensure in all of them that it is in array. As opposed to a string, object, integer or whatnot. So since we need to check in every method, we better move the checking itself to a method.
So eventually, we get a lot more docs about @throws, and a lot more method calls. :) There you go.
1. Do we want to subclass this from a generic config exception to our own?
2. Do we want to extend unit test coverage to ensure the exceptions are thrown?
Comment #111
Gábor HojtsyExpanded unit tests.
Comment #112
Gábor HojtsyAnd dedicated exception. I'm pretty confident @alexpott would have asked for it :)
Comment #113
bojanz CreditAttribution: bojanz commentedWill it be possible to translate the settings added this way?
Or is that not planned / out of the scope of this patch?
Comment #114
yched CreditAttribution: yched commentedHm - the last additions feel a bit bloat/overkill.
IMO we could downright hardcode the name of the property used to store the 3rd party settings to 'third_party_settings', have ThirdPartySettingsTrait declare $third_party_settings = array(), and be done with it...
That would mean changing the entry name in node.type.*.yml from 'settings' to 'third_party_settings', but well, that's what they are. Since there is specific logic, behavior and API related to "being the property that stores settings in the name of other modules", we might as well make that clear by using consistent property names in the various yaml files...
Comment #115
yched CreditAttribution: yched commented@bojanz: yes, the point of this issue is to provide a storage that's compatible with the notion of config schemas, so that those settings can be translatable.
Comment #116
Gábor Hojtsy@yched: all right, scratch that then :D I agree that baking in the property name is better than splintering everything with exceptions and such. And baking in the property in the trait will disallow defining the same property on the using class, so accidental conflicts are ruled out. I guess we an decide to accept this one or continue from #112.
Diff between #100 and #116 since the work inbetween went to trash.
Comment #118
swentel CreditAttribution: swentel commentedThe thing is that those are not third party settings strictly speaking, since they are from the node module ? What should happen with that then ? :)
Comment #119
BerdirBack to just settings.preview and so on?
What do we want to convert here and what in follow-up issues? per-node-type settings of menu_ui would be a perfect example for this I think and might help to point out limitations or other stuff that we need to do? Did not read anything yet, what about defaults for example? And how will the form/editing work exactly, how much do they have to do themself?
Comment #120
Gábor Hojtsy@Berdir: Well, this feature is designed for settings where the type, UI, etc. are not controlled by the module managing the entity, so the third party module would provide the form altered fields, save into the entity edited in the form using the API provided here, provide its element schema for the settings, etc.
Comment #121
BerdirYes, but especially for node types, it is a very common pattern to extend the default node type form and add per node type settings there. The core example there are the menu_ui settings for node types (which menus are available to create menu links in and so on). Right now, that is saved in a separate config file.
And I think it would be a nice example to see how that will work, as menu_ui will then need a way to update it's setting on $node_type between building it and saving it. We can probably use #entity_builders for that (see EntityForm::buildEntity()), but there will possibly be other questions too, for example menu currently implements node_type_insert() and creates a corresponding file there (which has some problems for default config, as it overrides it).
We can do that in a follow-up, but then we might need API changes there, so we could just include it here, the patch is relatively small still and the menu_ui example shouldn't be that complicated.
Comment #122
yched CreditAttribution: yched commentedWell, what the node.type.*yml "settings" entry contains (i.e does it also contain the native settings controlled by node.module itself ?) is not really related to how we name that entry ('settings' or 'third_party_settings') , right ? :-)
IMO the model implied by the feature & API introduced here is that yes, the native properties do not live in the same entry than the 3rd party settings.
Stritly speaking, the native node type properties do not need to be grouped into a top-level entry, they can directly live at the top level, they're native, thus known, predictable and non-extensible. Why would 'description' be more top-levelish than 'preview' ?
It's only the 3rd party settings that *need* to be hosted under a dedicated entry with dedicated logic. Consistently naming that entry 'third_party_settings' is fine IMO, and more predictable.
So we'd have:
?
(not sure why some stuff is currently nested in 'options', BTW)
But all this is more a topic for the issue where we move NodeType to ThirdPartySettingsTrait ? I.e #2324121: NodeType's settings array was meant to be able to store information from mutliple modules in the current issue split ?
Comment #123
Gábor Hojtsy@Berdir: yeah the idea was not to resolve all the third party settings use cases but to provide a way to store them and solve the most pressing one which actually currently violates CMI in this issue. Menu items as you describe do not violate how CMI is supposed to work (schemas, ownership, etc). There are more use cases for extensible config like blocks.
Comment #124
BerdirI'm fine with doing node type stuff in the follow-up issue, sorry for derailing.
But if we even remotely follow the suggestions made by yched, with moving existing settings around, then that must happen before beta too. It is already beta target, but we might want to consider blocking it?
Which also makes sense for the reason that I pointed out (having a second implementation to see different use cases before we freeze the API). Would IMHO be very sad to have this common API and then node_types have their own custom implementation that's not even used. But that's the risk of these last-minute changes :)
Comment #125
sunIntegrationSettings[Trait]
('integration_settings'
property name) would be a better name. Not really shorter, but more concise and accurate. Easier to use in communication, sans risk of wonky shorthands à la "3rd party settings".The original idea of bundling node's own settings into
'settings'
(in #111715: Convert node/content types into configuration) was to supply a consistent API for developers (DX) when dealing with content type settings; i.e., if it's not a base property, then it's always a business logic setting that is always owned by a module. Also, since Node module uses its own integration API for its own settings, it's implicitly known to work correctly. Perhaps that makes sense, perhaps it doesn't, but that's the backstory.I agree that we should strive for the "one final solution for all" here. Module/API-specific inconsistencies in this area would result in a painful DX. Beta is approaching, so this is going to be the last chance for unifying things.
To that extent, note that the entire pattern shares a lot of similarities with the configuration of filter plugins in text formats. Personally, my hope was and still is that we'd standardize all of these cases onto that pattern — i.e., in the case of node type settings, we'd have an
EntityIntegrationPluginInterface
, which defines the settings, holds the administrative configuration form, as well as the Entity API hook/event methods and entity form/display integration code. Such plugins may apply to multiple entity types (instead of being node-specific). As in the filter/format constellation, the configuration is embedded into the entity type config.Comment #126
Gábor Hojtsy@Berdir: I would not be surprised if we find a couple more use cases later on, other than field and node type also.
@sun: using a full-on plugin system was deemed too much for these settings above.
What do other think of the IntegrationSettings name? It sounds a bit off to me. Like what is NOT an integration setting? The machine name of the field integrates with content storage, etc. for example. Cardinality settings integrate with how data storage may be generated.
Comment #127
benjy CreditAttribution: benjy commentedYeah I agree, I quite like the third party settings previously suggested.
We previously had sticky/promoted etc under options and they've been moved to BaseFieldOverride in #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields. Lets just move revision to a top level key as well?
Comment #128
andypostActually this is a "extra modules' settings" or "outbound/external" but @yched forced this to "third party" #2005434-59: Let 3rd party modules store extra configuration in EntityDisplay
- entity is defined by provider
- some other provider (extension) wanna integrate some data into the entity
So what is "third party"? - it's not a third party, it's second party or external intergrator
Comment #129
Gábor Hojtsy@andypost: Well, it is third party in terms of added from the outside to the relation of the config store and the entity, which are the two parties otherwise managing the config settings. This is a common use of the word third party.
Comment #130
yched CreditAttribution: yched commentedThe comment is slightly inaccurate, this is not removing the setting (was removed above), but the provider.
Other "use" statements for traits in core leave an empty line after the "class" declaration.
When #2005434: Let 3rd party modules store extra configuration in EntityDisplay added "third party settings" to EntityDisplays, it also added explicit hook_field_formatter_third_party_settings_form() / hook_field_widget_third_party_settings_form() hooks, with field_ui's DisplayOverview / FormDisplayOverview taking care of declaring the 'third_party_settings' form element, populating it with the hook results, and saving it in the property.
I guess it was done because doing direct form alters on the DisplayOverview forms to find the "formatter settings (sub)form" would be kind of tedious (those forms are highly multistep / ajax-driven).
But it also makes sense strictly speaking - the "official form" for the entity controls inputs for the all the top-level entity properties it's going to save, and just happens to delegate the actual content of some of those to other parties.
If a config entity declares itself as "extensible" by implementing ThirdPartySettingsinterface, arguably it's the job of its form to provide the corresponding UI mounting points ?
Not too sure myself, opinions welcome.
Yeah... FieldDefinitionInterface does not implement ThirdPartySettingsInterface, only FieldConfigInterface does.
So any code willing to access third party settings for a field will need this instanceof check :-(
DX--, but no better proposal, that's a consequence of having both FDI and FCI, that's another discussion...
Comment #131
Gábor Hojtsy@yched: resolving 1 and 2. About 3, I am very vary of introducing more hooks here, if we are to go down that route, we can just as well make them plugins. But then again we ruled out plugins because we don't want that complexity. But if we are to reproduce what the plugins would do by other more obscure means, then what's better?
Comment #132
yched CreditAttribution: yched commentedWell, the difference with plugins is that plugins (of a given plugin type) corespond to well-defined "jobs" ("being a text filter"), expressed by a clearly defined interface, whose methods are definite tasks called at specific points by some surrounding system (e.g FilterInterface::process() being called by the filter system when it makes sense for the filter system)
There is no "job" to describe for the stuff we're doing here. There's no telling when or where each of the "3rt party modules" that add settings will make use of those settings. content_translation uses the 'translation_sync' setting in a method call triggered in content_translation_entity_presave(), for some other "setting" it's going to be somewhere else...
This is just about storing "companion" values along with the config entity type, not about formalizing what use is going to be made of those values. Plugins are not the right tool.
Comment #133
Gábor HojtsyLooking at existing use cases we have for plugins, all that a condition does for example (eg. the language condition) is it presents a config form, produces the config value based on the form, provides default values for the config and summarizes current settings (so far all things we may want to do for third party config settings) AND evaluating the condition itself (which is the only condition specific behaviour so to speak). At least having a coherent interface for providing the config form, default config, etc. would be much more D8-like a solution as opposed to adding a few more special cased form alter hooks that people need to manually figure out. We are selling Drupal 8 with a promise that you don't need to figure our X unrelated looking hooks to implement something.
Anyhow, the critical bug being resolved here is not that the form cannot be conveniently altered, altering the form is standard Drupal practice and it was/is altered before/after the patch appropriately, Drupal 8 did not make it less appropriate to modify forms. The problem we are solving here is altering **the configuration** is not a supported scenario in D8 because then there is no clear ownership of the persisted configuration, we cannot track typing of that info, dependencies, etc.
Comment #134
effulgentsia CreditAttribution: effulgentsia commentedI agree with this. In the case of #2005434: Let 3rd party modules store extra configuration in EntityDisplay, we had an existing hook_field_formatter_settings_form_alter() that we replaced with hook_field_formatter_third_party_settings_form(), because in that case, the third party settings are attached to a plugin (formatters), and code that deals with plugins shouldn't need to know where within the complete $form structure their portion is. In this issue's case though, we are attaching third party settings to a config entity, and we have not yet instituted generalized patterns in Drupal for extending entity forms through anything other than hook_form_alter(). Maybe we should, but that seems like a separate issue to me: one that doesn't need to be critical or beta deadlined.
In relation to UI mounting points though, what I do wonder is:
Are we sure we want all 3rd party modules to have their settings grouped together in the $form structure, and separate from $form['instance']['settings']? Maybe so, in which case, let's add the
$form['instance']['third_party_settings']
grouping element inFieldInstanceEditForm::buildForm()
and decide on what weight we want for it (e.g., 11 if we want it to be below $form['instance']['settings']). Alternatively, maybe each module should create its own sibling of $form['instance']['settings'] (e.g., $form['instance']['content_translation_settings']) so that each one can make its own decision of its weight relative to $form['instance']['settings'] and other things in $form['instance']? If we do that, we'll need to set#parents
on that element toarray('instance', 'third_party_settings', 'content_translation')
to get it into the right part of $form_state['values'], but that's easy enough.Additionally, we still have a @todo in HEAD in _content_translation_form_language_content_settings_form_alter() that's linking to this issue:
I think we can remove that
if
check now, or is there a reason to punt that to a follow up?Other than the above two questions, I think this patch is ready to go.
Comment #135
yched CreditAttribution: yched commented+1 on that. It should be the entity form's responsibility to create the container for the "third party settings" entry, and place it in the form (weight, etc...). Then, having 3rd party modules add their elements in that container with a simple form_alter() is fine.
Comment #136
fagoagreed with #135 and that a good way to alter entity forms should be its own issue.
+ public function setThirdPartySetting($module, $key, $value);
Reading this, it's obvious that the 3rd party has to be a module. I'm wondering whether "setModuleSettings" would be a good name then, but it might be less obvious what "module settings" ought to be.
Comment #137
effulgentsia CreditAttribution: effulgentsia commentedRe #125/#136: The nomenclature of setThirdPartySetting() and friends is already in HEAD within Drupal\Core\Field\PluginSettingsInterface. What I like about the name is it clearly means "all things that are not the owner thing". While it's not explicit that "thing" = "module", that gets clarified in the signature. Not saying that there aren't better names out there, but wondering how much we want to refine it, given that it's already in HEAD.
Comment #138
effulgentsia CreditAttribution: effulgentsia commentedAddresses #134/#135.
Comment #139
yched CreditAttribution: yched commentedLooks good to me. RTBC if green.
Comment #140
plachShouldn't we hide checkboxes altogether if the field does not support third party settings?
(also: space before closing parenthesis)
Comment #141
Gábor HojtsyYeah that makes a ton of sense.
Comment #142
fagoTrue.
oh - I was not aware this is already in HEAD, so renaming this should be its own issue then. Given the above reasoning I do not see better options anyway though.
Comment #143
Dries CreditAttribution: Dries commentedCommitted to 8.x. Only one beta blocker left! Thanks for the hard work everyone.
Comment #145
XanoIs there a reason
ThirdPartySettingsTrait
does not camelcase its property according the coding standards other than that we may not want camelcasing in our YAML files and that's what we would end up with with the default way of mapping config entity properties to storage?Comment #146
BerdirComment #147
Gábor Hojtsy@Xano: I am not aware of other reasons.
Comment #148
effulgentsia CreditAttribution: effulgentsia commented@Xano: AFAICT, we retain lowercase/underscore pattern for all our other multiword properties that come from YAML. Examples:
CommentType::target_entity_type_id
,Editor::image_upload
,FieldStorageConfig::entity_type
, etc.Comment #149
XanoI understand. Is this documented anywhere? All I can find in the coding standards is that class properties must be camelcased. We currently violate our coding standards in one part of our code base in order to 'fix' something in another part for which there are no standards yet.
Comment #150
bojanz CreditAttribution: bojanz commentedI don't see why we actually avoid camel case in yaml.
I'm camel casing yaml properties in Commerce 2.x because it feels less wrong than breaking the coding standard (and consistency with the rest of the code)
Comment #151
vijaycs85+1 to follow up for camelcast trait property. It looks like a minor error than anything intentional.
Comment #152
slashrsm CreditAttribution: slashrsm commentedIsn't that very common pattern all around the core codebase? If we standardize let's do it everywhere not just this trait.
Comment #153
BerdirExactly, this didn't introduce anything new, just following the common pattern for config entities, where almost everything is lowercase, with a few exceptions.
Keep in mind that this wouldn't just affect normal config entity properties, but also every kind of plugin configuration that is stored in config entities, from blocks over fields-stuff to views. I don't see how we would change all that?
I'm not sure where, but it *was* documented that fields for entities that are stored in the database do not follow the camelCase standard, and that happened before there the config/content entity split.
Comment #154
Gábor HojtsyWhere do we use camelCase in config entities in core?
Comment #155
XanoLet's continue this in #2335287: [policy, no patch] Class properties don't always follow coding standards.
Comment #157
XanoFollow-up: #2345097: Fix ThirdPartySettingsTrait property capitalization.
Comment #158
marcvangendFollow-up: #2345879: Enhance ThirdPartySettingsTrait with a get-all-settings method