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

  1. 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.
  2. Add common implementation for this in ThirdPartySettingsTrait, so config entities can take a common implementation for easier developer experience.
  3. 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.
  4. Add all modules that provide third party settings as dependencies to the configuration entity dependency list.
  5. 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.

CommentFileSizeAuthor
#141 interdiff.txt1.47 KBGábor Hojtsy
#141 2224761.141.patch30.54 KBGábor Hojtsy
#138 interdiff.txt2.57 KBeffulgentsia
#138 2224761.138.patch30.05 KBeffulgentsia
#131 interdiff.txt1.79 KBGábor Hojtsy
#131 2224761.131.patch27.83 KBGábor Hojtsy
#116 2224761.116.patch27.84 KBGábor Hojtsy
#116 interdiff-100-to-116.patch5.93 KBGábor Hojtsy
#112 2224761.112.patch32.63 KBGábor Hojtsy
#112 interdiff.txt4.82 KBGábor Hojtsy
#111 interdiff.txt4.86 KBGábor Hojtsy
#111 2224761.110.patch30.25 KBGábor Hojtsy
#110 2224761.110.patch30.25 KBGábor Hojtsy
#110 interdiff.txt4.86 KBGábor Hojtsy
#100 2224761.98.patch28.54 KBalexpott
#100 94-98-interdiff.txt23.61 KBalexpott
#100 2224761.98.patch27.32 KBalexpott
#100 94-98-interdiff.txt21.85 KBalexpott
#94 2224761-94.patch11.24 KBeffulgentsia
#45 2224761.44.patch14.47 KBalexpott
#45 38-44-interdiff.txt5.88 KBalexpott
#38 36-38-interdiff.txt1.27 KBalexpott
#38 2224761.38.patch13.19 KBalexpott
#36 2224761.36.patch14.46 KBalexpott
#36 33-36-interdiff.txt11.9 KBalexpott
#33 content_translation-2224761-33.patch12.26 KBxjm
#29 2224761-29-content_Translation-translation_sync_config.patch12.26 KBmlncn
#23 2224761-23-content_translation-translation_sync_config.patch11.79 KBeffulgentsia
#22 interdiff.txt7.08 KBpfrenssen
#22 2224761-22-content_translation-translation_sync_config.patch11.8 KBpfrenssen
#15 interdiff.txt1.47 KBpfrenssen
#15 2224761-15-content_translation-translation_sync_config.patch8.94 KBpfrenssen
#12 interdiff.txt7.07 KBpfrenssen
#12 2224761-12-content_translation-translation_sync_config.patch8.65 KBpfrenssen
#5 2224761-5-content_translation-translation_sync_config.patch7.01 KBpfrenssen
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

Issue tags: +Field API
jessebeach’s picture

Issue summary: View changes
jessebeach’s picture

Status: Postponed » Active

Unpostponed!

pfrenssen’s picture

Assigned: Unassigned » pfrenssen
pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Active » Needs review
FileSize
7.01 KB

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

plach’s picture

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Tests/ContentTranslationSyncImageTest.php
@@ -69,14 +69,31 @@ protected function setupTestFields() {
+        'enabled' => TRUE,

Everything looks good aside from this line. Is it really needed?

pfrenssen’s picture

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

plach’s picture

Status: Needs review » Reviewed & tested by the community

Ok, thanks :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

We'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:

translation_sync:
  - field_image
  - field_title

The current structure is:

field_image:
  translation_sync: true
field_tags:
  translation_sync: false
field_title:
  translation_sync: true

I don't think fields which have a value of false should be in the file.

plach’s picture

The changes proposed by @alexpott make sense to me, sorry for missing the schema issue: I am not really into config yet...

pfrenssen’s picture

Assigned: Unassigned » pfrenssen
pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
8.65 KB
7.07 KB

Addressed the remarks.

yched’s picture

Status: Needs review » Needs work

A 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

translation_sync:
  - node
    - article
      - field_image
      - field_title
    - page
      - field_title
  - some_other_entity_type
    - ...
pfrenssen’s picture

Assigned: Unassigned » pfrenssen

It even needs to go one level deeper, as the translation sync is managed on field column group level. Example for an image field:

translation_sync:
  node:
    article:
      field_image:
        - alt
        - title

Luckily it's already implemented like this, it's just the config schema that is wrong.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
8.94 KB
1.47 KB

Fixed the config schema and added the default config file that I forgot to add in the previous patch.

Status: Needs review » Needs work

The last submitted patch, 15: 2224761-15-content_translation-translation_sync_config.patch, failed testing.

pfrenssen’s picture

I 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:

content_translation.field_settings:
  translation_sync:
    node:
      article:
        field_image:
          - alt
          - title

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:

# content_translation.field_settings.{entity type}.{bundle}.{field}
content_translation.field_settings.*.*.*:
  translation_sync:
    - alt
    - title

Here are some other config schemas that use this pattern:

  • field.instance.*.*.*
  • entity.form_display.*.*.*
alexpott’s picture

+++ b/core/modules/content_translation/config/schema/content_translation.field_settings.schema.yml
@@ -0,0 +1,8 @@
+  type: sequence
+  label: 'Fields that have translation synchronisation enabled'
+  sequence:
+    - type: string
+      label: 'Field group column'

This 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

content_translation.field_settings:
  type: mapping
  label: 'Content translation field settings'
  mapping:
    translation_sync:
      type: sequence
      label: 'Field translation sync settings'
      sequence:
        - type: sequence
          label: 'Entity type'
          sequence:
            - type: sequence
              label: 'Bundle'
              sequence:
                - type: sequence
                  label: 'Field'
                  sequence:
                    type: string
                    label: 'Column'
pfrenssen’s picture

Assigned: Unassigned » pfrenssen

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

yched’s picture

  1. +++ b/core/modules/content_translation/content_translation.admin.inc
    @@ -36,12 +36,18 @@ function content_translation_field_sync_widget(FieldDefinitionInterface $field)
    +    $translation_sync = \Drupal::config('content_translation.field_settings')->get('translation_sync.' . $field->id()) ?: array();
    

    This still assumes a translation_sync property directly keyed by field name instead of by [entity type, bundle, field name].

  2. +++ b/core/modules/content_translation/content_translation.module
    @@ -751,29 +751,29 @@ function content_translation_form_field_ui_field_edit_form_submit($form, array &
    +  $key = 'translation_sync.' . $form_state['instance']->id();
    

    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.

pfrenssen’s picture

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

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
11.8 KB
7.08 KB

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

effulgentsia’s picture

effulgentsia’s picture

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

effulgentsia’s picture

Per #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?

effulgentsia’s picture

Status: Needs review » Needs work

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

alexpott’s picture

We need to change this code too...

/**
 * Implements hook_install().
 */
function content_translation_install() {
  // Assign a fairly low weight to ensure our implementation of
  // hook_module_implements_alter() is run among the last ones.
  module_set_weight('content_translation', 10);
  \Drupal::service('language_negotiator')->saveConfiguration(Language::TYPE_CONTENT, array(LanguageNegotiationUrl::METHOD_ID => 0));

  $config_names = \Drupal::configFactory()->listAll('field.field.');
  foreach ($config_names as $name) {
    \Drupal::config($name)
      ->set('settings.translation_sync', FALSE)
      ->save();
  }
  $config_names = \Drupal::configFactory()->listAll('field.instance.');
  foreach ($config_names as $name) {
    \Drupal::config($name)
      ->set('settings.translation_sync', FALSE)
      ->save();
  }
}
mlncn’s picture

Assigned: Unassigned » mlncn
Issue summary: View changes
mlncn’s picture

Status: Needs work » Needs review
FileSize
12.26 KB

Reroll + 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?

Status: Needs review » Needs work

The last submitted patch, 29: 2224761-29-content_Translation-translation_sync_config.patch, failed testing.

xjm’s picture

Issue tags: +Entity Field API
xjm’s picture

Adding some tags to get more eyes in here. :)

xjm’s picture

Assigned: mlncn » Unassigned
Status: Needs work » Needs review
FileSize
12.26 KB

Going 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. :)

xjm’s picture

Status: Needs review » Needs work

NW for #26 and #27 now that the bot has picked it up.

YesCT’s picture

alexpott’s picture

Status: Needs work » Needs review
FileSize
11.9 KB
14.46 KB

Merge content_translation.settings and content_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.

Status: Needs review » Needs work

The last submitted patch, 36: 2224761.36.patch, failed testing.

alexpott’s picture

FileSize
13.19 KB
1.27 KB

Weird... new patch.

This patch results in content_translation.settings like:

node:
  article:
    content_translation:
      enabled: true
      fields:
        title: true
        uid: true
        status: true
        created: true
        changed: true
        promote: true
        sticky: true
        log: true
        body: true
        comment: true
        field_image: true
        field_tags: true
    translation_sync:
      field_image:
        - file
  page:
    content_translation:
      enabled: true
      fields:
        title: true
        uid: true
        status: true
        created: true
        changed: true
        promote: true
        sticky: true
        log: true
        body: true
alexpott’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 38: 2224761.38.patch, failed testing.

vijaycs85’s picture

To fix the schema part of config in #38, we got #2245721: Add missing configuration schema in language component

alexpott’s picture

@vijaycs85 yes but we're altering the schema here so let's get it done here.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
12.9 KB
2.46 KB

Ok, lets try this one.

alexpott’s picture

Issue summary: View changes

re: #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.

alexpott’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
5.88 KB
14.47 KB

Very weird d.o bugs...

Anyhow new patch which has a valid schema but a superfluous key.

vijaycs85’s picture

Schema in #45 looks good to me.

alexpott’s picture

Created #2248709: The root type of a configuration object can not be a sequence to address the superfluous key issue (is possible)

xjm’s picture

38: 2224761.38.patch queued for re-testing.

alexpott’s picture

Status: Needs work » Needs review

bots are back

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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

Gábor Hojtsy’s picture

BTW I looked at concerns expressed by @yched above, and those seem to be all taken into account and implemented as suggested also.

Gábor Hojtsy’s picture

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

Created a draft change record at https://drupal.org/node/2252851 too :)

catch’s picture

Status: Reviewed & tested by the community » Needs review

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

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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

catch’s picture

Not sure how best to resolve that, splitting for each field instance into its own config would be rather extreme.

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

Gábor Hojtsy’s picture

Well, so many good reasons indeed.

Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy
Gábor Hojtsy’s picture

Title: Translation sync should be stored in own storage instead of injected in field instance settings » Field instances should support pluggable extra configuration for use cases like translation sync
Assigned: Gábor Hojtsy » Unassigned

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

alexpott: GaborHojtsy: https://drupal.org/node/2224761 - now question is should the EntityTranslation settings be a config entity - considering it is the 0 to N usecase
[2:40pm] Druplicon: https://drupal.org/node/2224761 => Translation sync should be stored in own storage instead of injected in field instance settings #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration => 57 comments, 12 IRC mentions
[2:41pm] GaborHojtsy: alexpott: yeah I’ve been looking at doing the manual config management code adapting what we have now
[2:41pm] GaborHojtsy: alexpott: and it will have dependencies on the other entities, etc.
[2:41pm] GaborHojtsy: alexpott: on the module
[2:41pm] GaborHojtsy: alexpott: and so on
[2:41pm] GaborHojtsy: alexpott: if we handle it with the config dependency system, we don’t need to code for the deletion, etc.
[2:42pm] GaborHojtsy: alexpott: the ultimate hard question though is the name length
[2:42pm] GaborHojtsy: alexpott: this will have entity name, budle and field name combined with content_translation.sync as prefix or something along those lines
[2:42pm] alexpott: GaborHojtsy: yes but the only place where we are using the dependency system is uninstall atm
[2:43pm] alexpott: GaborHojtsy: content_translation.sync is not the much bigger then field.instance
[2:43pm] GaborHojtsy: hum
[2:44pm] GaborHojtsy: yeah I’m fine with “not that much bigger” :D
[2:45pm] alexpott: GaborHojtsy: well entity type is 64 and bundle is 32 and field name is 32
[2:45pm] GaborHojtsy: ah
[2:45pm] alexpott: GaborHojtsy: so I think you have plenty of space
[2:46pm] alexpott: GaborHojtsy: the dependency system is a good reason to make it a config entity
[2:48pm] GaborHojtsy: alexpott: yeah that is what I was thinking
[2:53pm] alexpott: GaborHojtsy: although it does mean the config entities are going to mushroom.
[2:53pm] GaborHojtsy: yeah this will set an interesting precedent for contrib
[2:53pm] alexpott: GaborHojtsy: in some ways this calls into question the whole approach
[2:54pm] GaborHojtsy: alexpott: well, unless fields provide plugabble additional settings
[2:54pm] alexpott: GaborHojtsy: what we really need is a way for modules to plug into the field system
[2:54pm] alexpott: GaborHojtsy: great minds
[2:54pm] GaborHojtsy: alexpott: yeah I put this into the issue summary earlier :)
[2:55pm] GaborHojtsy: alexpott: since arbitrary altering config is not possible (due to schema :D) either standalone related config needs to be done like we do now
[2:55pm] GaborHojtsy: alexpott: or pluggable settings, like views
[2:55pm] alexpott: GaborHojtsy: I think we should do the latter
[2:56pm] alexpott: GaborHojtsy: this way the configuration and deployment of it always goes hand in hand
[2:56pm] alexpott: GaborHojtsy: and the translation sync config has no meaning without the field instance
[2:57pm] alexpott: GaborHojtsy: we just need to convince TimP to work on it and we'll have a patch asap :)
[3:02pm] GaborHojtsy: alexpott: yeah I think for some reason the field people shied away from that
[3:02pm] GaborHojtsy: alexpott: maybe they thought it would be too heavy handed
[3:02pm] alexpott: GaborHojtsy: well it is less heavy handed than a gazillion config entities
[3:02pm] GaborHojtsy: alexpott: that would clearly be more extensible for other types of settings without exploding the number of config files/entities
[3:02pm] GaborHojtsy: yeah
[3:03pm] GaborHojtsy: alexpott: any conerns with me posting this discussion on the issue?
[3:03pm] alexpott: GaborHojtsy: none :)

Retitled the issue for this. Let's figure out if others thing this would be a superior (and feasible) approach to this problem.

effulgentsia’s picture

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

yched’s picture

I 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 :

  • This means that you cannot ship "translation sync" or other 3rd party settings in config/install independently of the host config entity itself : I can't ship 'extra_setting_foo' for a given field_bar in my config/install if I'm not the one that ships field_bar itself. Too bad, live with it.
  • This pattern popping up here and there probably means we should have a solid unified API for this, that takes care of the nice subtleties like :
    • cleaning up the extra settings added by a module when that module gets uninstalled (initially, and before config schema considerations, the cleanup aspect was one of the main reason for the "store your own settings yourself" motto)
    • reversedly, not sure what to do with the case of "some config/install/field.instance.xxxxx.yml includes 'extra properties' for a module that is currently not installed, and thus has a part for which we can't generate a schema".
      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 ?
  • Where is the line ? ;-) Why don't we just bake this in all config entities natively ?
    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.
yched’s picture

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

catch’s picture

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

effulgentsia’s picture

Title: Field instances should support pluggable extra configuration for use cases like translation sync » Field instances should support cross-field-type, extensible, extra configuration for use cases like translation sync

Views and node types config entities already have this "inner extensibility" feature

From 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 a node.settings.node, I don't see any other schema file with a node.settings.SOMETHING_ELSE.

Where is the line ? ;-) Why don't we just bake this in all config entities natively ?

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

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.

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.

alexpott’s picture

I'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

15:16 alexpott
catch, GaborHojtsy: I've been working on the translation sync issue - converting it to a plugin for a FieldInstance
15:16 alexpott
catch, GaborHojtsy: as in a FieldInstanceExtraSettings plugin
15:17 alexpott
catch, GaborHojtsy: but the brick wall I've just run into is the fact that non configurable fields are covered by translation sync
15:18 alexpott
catch, GaborHojtsy: either of you got any good ideas?
15:20 catch
alexpott: no good ideas, but this feels like fundamentally the same problem as https://drupal.org/node/2005434
15:20 Druplicon
https://drupal.org/node/2005434 => Let 3rd party modules store extra configuration in EntityDisplay [
#2005434]
=> 33 comments, 10 IRC mentions
15:21 alexpott
catch: well at least entity displays always have a config file
15:22 heyrocker has joined (~heyrocker@2601:7:2200:8e8:1580:e7d0:b193:90ca)
15:22 alexpott
catch: so we have a place to store extra settings - for fields like node title what to do?
15:25 GaborHojtsy
alexpott: yeah we need some kind of storage for that config and ideally it would be per field instance too
15:25 GaborHojtsy
alexpott: even if they are “non-configurable” they have widget/display settings as well :) now those are not on the field instance but ont he entity display/form
15:26 GaborHojtsy
alexpott: but translatability is on the instance or at least it is supposed to be to be more precise, there are some issues :D
15:27 catch
alexpott: would this also be a problem if it used config entities and not plugins for this?
15:28 alexpott
catch: nope because then each field would have its own config entity - but then each contrib module that wants to do something similar will have a config entity :(
15:28 catch
alexpott: are there that many?
15:29 alexpott
catch: well any module that wants to add a custom setting to any field instance would need to do this
15:30 catch
alexpott: I can't think of any offhand.
15:32 alexpott
berdir: so we're discussing https://drupal.org/node/2224761
15:32 Druplicon
https://drupal.org/node/2224761 => Field instances should support cross-field-type, extensible, extra configuration for use cases like translation sync [
#2224761]
=> 64 comments, 22 IRC mentions
15:33 berdir
uff, ok.. :)
15:33 alexpott
berdir: I've hit a brick wall because the translation_sync covers both non configurable and configurable fields
15:33 alexpott
berdir: so we can't store this on the field instance
15:34 alexpott
berdir: catch is wondering if we should create a FieldTranslation (or something) config entity to store this.
15:35 alexpott
berdir: I'm concerned that we'll get too many config entities because contrib will just have to do the same thing. catch can't think of any contrib modules that would need to do this. Can you?
15:35 alexpott
berdir: or do you have any better ideas
15:39 GaborHojtsy
alexpott: catch: I think the moving of form/display stuff to their own systems greatly reduced the chance other modules would need to add more settings on fields… anything related to the field settings themselves like defaults, validation, etc. would be candidates there
15:39 GaborHojtsy
does not claim he knows the related contrib field
15:40 berdir
alexpott: as far as "nobody else has that problem", this is exactly the same problem as menu settings for node types I think?
15:41 berdir
not the config vs. base field complexity, but at least the "additional settings for a config entity" part
15:42 aloyr_ has joined (~aloyr@174.46.30.194)
15:42 alexpott
berdir: well the additional settings stuff can be solved through using plugins - the work i've done on this already shows that it'd work.
15:42 berdir
k
15:42 GaborHojtsy
berdir: yeah that one is a shared problem with views, entity displays/forms, node types, etc. :)
15:42 GaborHojtsy
berdir: the problem here is base fields
15:42 GaborHojtsy
berdir: because they have no config entity to plug into
15:43 berdir
so it's specific about the fields with are a mix of config and code-defined fields..
15:43 aloyr_ has left ()
15:43 alexpott
berdir: yep
15:44 alexpott
GaborHojtsy, catch, berdir: I'm guessing https://drupal.org/project/field_validation would use the same solution as we come up with.
15:44 catch
alexpott: I think that + translation it's OK to force them to make new config entities.
15:44 berdir
can't name any examples right now, but I think there are some modules that extend field settings, yeah, that or maybe field permissions, not sure if base fields are relevant for that..
15:45 catch
I can see field permissions wanting to do things like view permission on author.
15:47 GaborHojtsy
catch: so then that is one new config entity per field instance per such module so 3 additional config entities per field instance if you use field validtion, translation and field permissions, which is not that far fetcheed a combination
15:47 GaborHojtsy
in total 4 config entities for each configurable field
15:48 alexpott
catch, berdir, GaborHojtsy: So anything that wants to extend a field needs to do this. Maybe we should have a field extender config entity - which has a plugin manager which anything can plug in to.
15:48 catch
Field validation only needs that on actual validation (+ the UI)
15:48 catch
So it woudldn't need to load the entity on view.
15:49 alexpott
catch: and translation sync is not needed on view either
15:49 catch
So only field permissions then?
15:50 catch
the config entity really doesn't feel like that much overhead.
15:52 catch
alexpott: do you mean one extra config entity,then the plugin implementation is on that?
15:52 alexpott
catch: yep
15:53 catch
alexpott: is this optional if nothing needs it?
15:53 alexpott
catch: yep
15:53 alexpott
catch: no plugins -> trigger an entity delete
15:53 catch
alexpott: well would we try to load it all the time is what I meant.
15:53 alexpott
catch: method on FieldDefiintion to get the entity
15:53 alexpott
catch: only when needed
15:54 catch
alexpott: that seems OK.
15:56 alexpott
catch: so it'll be called field.extra_settings.<entity type>.<bundle>.<field name>
15:56 alexpott
berdir: any objections to this approach?
15:57 berdir
nope.. and I certainly have no better ideas :)

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.

effulgentsia’s picture

method on FieldDefiintion to get the entity

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

effulgentsia’s picture

Title: Field instances should support cross-field-type, extensible, extra configuration for use cases like translation sync » Add support for cross-field-type, extensible, extra configuration to for use cases like translation sync

Expanding that scope to also include the enhancing of code-defined fields with "extra" configuration seems ok in concept

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

alexpott’s picture

Well i was thinking of creating this in Core and not in the field module.

yched’s picture

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

Gábor Hojtsy’s picture

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

Well, 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?

xjm’s picture

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

xjm’s picture

Title: Add support for cross-field-type, extensible, extra configuration to for use cases like translation sync » Add support for cross-field-type, extensible, extra configuration
alexpott’s picture

Issue summary: View changes
Status: Needs work » Postponed

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

plach’s picture

Component: content_translation.module » field system

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

effulgentsia’s picture

Issue summary: View changes
plach’s picture

Issue summary: View changes

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

plach’s picture

Issue summary: View changes
jessebeach’s picture

Issue summary: View changes
effulgentsia’s picture

Title: Add support for cross-field-type, extensible, extra configuration » [PP-2] Add support for cross-field-type, extensible, extra configuration
Issue summary: View changes
Related issues: +#2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields

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

plach’s picture

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

effulgentsia’s picture

Title: [PP-2] Add support for cross-field-type, extensible, extra configuration » Add support for cross-field-type, extensible, extra configuration
Issue summary: View changes
Status: Postponed » Active
Issue tags: -Needs issue summary update
Related issues: +#2005434: Let 3rd party modules store extra configuration in EntityDisplay

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

xjm’s picture

Issue summary: View changes
Status: Active » Postponed

Per discussion with @alexpott, let's wait to do this until #2143291: Clarify handling of field translatability is in.

effulgentsia’s picture

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

alexpott’s picture

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

xjm’s picture

Issue summary: View changes
pwolanin’s picture

From the issue summary, it's not clear why this is a beta blocker or critical.

Gábor Hojtsy’s picture

Issue summary: View changes

@pwolanin: hopefully my latest update helped?

pwolanin’s picture

@Gabor - sorry, but not really. Is this going to be an API change that will break existing sites and/or require a re-install?

Gábor Hojtsy’s picture

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

a.ross’s picture

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

andypost’s picture

Status: Postponed » Needs review
xjm’s picture

Status: Needs review » Active

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

a.ross’s picture

Oh well, that is a happy coincidence :)

effulgentsia’s picture

Status: Active » Needs work
FileSize
11.24 KB

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

alexpott’s picture

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

yched’s picture

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

The plugin system already handles this for things that are plugins

I don't think I get what this refers to ?

xjm’s picture

The plugin system already handles this for things that are plugins

I don't think I get what this refers to ?

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

yched’s picture

@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 :-)

yched’s picture

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

alexpott’s picture

So the patch attached implements ThirdPartySettingsInterface and provides a general implementation for config entities to use with ThirdPartySettingsTrait. Config dependency detection is baked into ConfigEntityBase::calculateDependencies(). This all leveraged by FieldConfigBase 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:

third_party_settings:
  content_translation:
    translation_sync:
      title: title
      file: '0'
      alt: '0'
alexpott’s picture

The last submitted patch, 100: 2224761.98.patch, failed testing.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -326,6 +326,13 @@ public function calculateDependencies() {
    +    if ($this instanceof ThirdPartySettingsInterface) {
    +      // Configuration entities need to depend on the providers of any third
    +      // parties that they store the configuration for.
    +      foreach ($this->getThirdPartyProviders() as $provider) {
    +        $this->addDependency('module', $provider);
    +      }
    +    }
    

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

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ThirdPartySettingsTrait.php
    @@ -0,0 +1,93 @@
    + * Provides generic implementation of ThirdPartySettingsInterface.
    ...
    + * @see \Drupal\Core\Config\Entity\ThirdPartySettingsInterface
    ...
    +trait ThirdPartySettingsTrait {
    

    What about actually implementing the interface ... :)

  3. +++ b/core/lib/Drupal/Core/Config/Entity/ThirdPartySettingsTrait.php
    @@ -0,0 +1,93 @@
    +    return array_keys($this->{$this->thirdPartySettingKey});
    

    This line would fail if the third party setting is NULL, which might happen.

alexpott’s picture

@dawehner #103:

  1. Well this is up for the entity that implements the ability for third party settings to implement this is just a generic implementation for the common use case
  2. Traits can't
  3. Well the use cases so far always define it as an empty array. But I guess there is no harm in a bit of defensive coding here.
Gábor Hojtsy’s picture

Title: Add support for cross-field-type, extensible, extra configuration » Add a generic way to add third party configuration on configuration entities and implement for field configuration
Issue summary: View changes
Issue tags: -Needs issue summary update

Fully updated issue summary, retitled based on the significance of the changes.

Gábor Hojtsy’s picture

Also deleted the absolutely outdated change notice and wrote https://www.drupal.org/node/2326151.

plach’s picture

Issue tags: +sprint

Tagging for the rocketship.

Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy

From #103/#104, I cannot exactly gather if point 3 should be improved or not :) Other than that, what is outstanding? Any concerns? Reviews please!

swentel’s picture

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

Gábor Hojtsy’s picture

FileSize
4.86 KB
30.25 KB

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

Gábor Hojtsy’s picture

FileSize
30.25 KB
4.86 KB

Expanded unit tests.

Gábor Hojtsy’s picture

FileSize
4.82 KB
32.63 KB

And dedicated exception. I'm pretty confident @alexpott would have asked for it :)

bojanz’s picture

Will it be possible to translate the settings added this way?
Or is that not planned / out of the scope of this patch?

yched’s picture

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

yched’s picture

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

Gábor Hojtsy’s picture

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

The last submitted patch, 116: interdiff-100-to-116.patch, failed testing.

swentel’s picture

settings:
  node:
    preview: 1
    options:
      revision: false
    submitted: true

The thing is that those are not third party settings strictly speaking, since they are from the node module ? What should happen with that then ? :)

Berdir’s picture

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

Gábor Hojtsy’s picture

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

Berdir’s picture

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

yched’s picture

Well, 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:

type: article
name: Article
preview: 1
options:
  revision: false
submitted: true
third_party_settings:
  some_module:
    some_setting: 'foo'
  some_other_module:
    some_other_setting: 'bar'

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

Gábor Hojtsy’s picture

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

Berdir’s picture

I'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 :)

sun’s picture

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

Gábor Hojtsy’s picture

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

benjy’s picture

What do other think of the IntegrationSettings name? It sounds a bit off to me.

Yeah I agree, I quite like the third party settings previously suggested.

options:
  revision: false

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?

andypost’s picture

Actually 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

Gábor Hojtsy’s picture

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

yched’s picture

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ThirdPartySettingsTrait.php
    @@ -0,0 +1,101 @@
    +    unset($this->third_party_settings[$module][$key]);
    +    // If the third party is no longer storing any information completely remove
    +    // the setting.
    +    if (empty($this->third_party_settings[$module])) {
    +      unset($this->third_party_settings[$module]);
    +    }
    

    The comment is slightly inaccurate, this is not removing the setting (was removed above), but the provider.

  2. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
    @@ -16,6 +17,7 @@
     abstract class FieldConfigBase extends ConfigEntityBase implements FieldConfigInterface {
    +  use ThirdPartySettingsTrait;
    

    Other "use" statements for traits in core leave an empty line after the "class" declaration.

  3. +++ b/core/modules/content_translation/content_translation.module
    +++ b/core/modules/content_translation/content_translation.module
    @@ -642,8 +642,8 @@ function content_translation_form_field_ui_field_instance_edit_form_alter(array
    
    @@ -642,8 +642,8 @@ function content_translation_form_field_ui_field_instance_edit_form_alter(array
    -      $form['instance']['settings']['translation_sync'] = $element;
    -      $form['instance']['settings']['translation_sync']['#weight'] = -10;
    +      $form['instance']['third_party_settings']['content_translation']['translation_sync'] = $element;
    +      $form['instance']['third_party_settings']['content_translation']['translation_sync']['#weight'] = -10;
    

    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.

  4. +++ b/core/modules/content_translation/src/FieldTranslationSynchronizer.php
    @@ -61,7 +63,7 @@ public function synchronizeFields(ContentEntityInterface $entity, $sync_langcode
    -      if ($field_definition->isTranslatable() && !$items->isEmpty() && $translation_sync = $field_definition->getSetting('translation_sync')) {
    +      if ($field_definition instanceof ThirdPartySettingsInterface && $field_definition->isTranslatable() && !$items->isEmpty() && $translation_sync = $field_definition->getThirdPartySetting('content_translation', 'translation_sync')) {
    

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

Gábor Hojtsy’s picture

FileSize
27.83 KB
1.79 KB

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

yched’s picture

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

Gábor Hojtsy’s picture

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

effulgentsia’s picture

Anyhow, the critical bug being resolved here is not that the form cannot be conveniently altered... 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.

I 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:

+++ b/core/modules/content_translation/content_translation.module
@@ -629,8 +629,8 @@ function content_translation_form_field_ui_field_instance_edit_form_alter(array
-      $form['instance']['settings']['translation_sync'] = $element;
-      $form['instance']['settings']['translation_sync']['#weight'] = -10;
+      $form['instance']['third_party_settings']['content_translation']['translation_sync'] = $element;
+      $form['instance']['third_party_settings']['content_translation']['translation_sync']['#weight'] = -10;

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 in FieldInstanceEditForm::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 to array('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:

// @todo Remove this special casing when arbitrary settings can be
//   stored for any field. See https://drupal.org/node/2224761.
if ($definition instanceof FieldInstanceConfigInterface) {
  $column_element = content_translation_field_sync_widget($definition);
  ...
}

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.

yched’s picture

in which case, let's add the $form['instance']['third_party_settings'] grouping element in FieldInstanceEditForm::buildForm() and decide on what weight we want for it (e.g., 11 if we want it to be below $form['instance']['settings']).

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

fago’s picture

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

effulgentsia’s picture

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

effulgentsia’s picture

FileSize
30.05 KB
2.57 KB

Addresses #134/#135.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. RTBC if green.

plach’s picture

+++ b/core/modules/content_translation/content_translation.admin.inc
@@ -36,14 +37,16 @@ function content_translation_field_sync_widget(FieldDefinitionInterface $field)
+    if ($field instanceof ThirdPartySettingsInterface ) {

Shouldn't we hide checkboxes altogether if the field does not support third party settings?
(also: space before closing parenthesis)

Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » Unassigned
FileSize
30.54 KB
1.47 KB

Yeah that makes a ton of sense.

fago’s picture

What I like about the name is it clearly means "all things that are not the owner thing".

True.

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.

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.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Only one beta blocker left! Thanks for the hard work everyone.

  • Dries committed ec2b048 on 8.0.x
    Issue #2224761 by Gábor Hojtsy, alexpott, pfrenssen, effulgentsia, xjm,...
Xano’s picture

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

Berdir’s picture

Issue tags: -sprint
Gábor Hojtsy’s picture

@Xano: I am not aware of other reasons.

effulgentsia’s picture

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

Xano’s picture

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

bojanz’s picture

I 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)

vijaycs85’s picture

+1 to follow up for camelcast trait property. It looks like a minor error than anything intentional.

slashrsm’s picture

Isn't that very common pattern all around the core codebase? If we standardize let's do it everywhere not just this trait.

Berdir’s picture

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

Gábor Hojtsy’s picture

Where do we use camelCase in config entities in core?

Xano’s picture

Status: Fixed » Closed (fixed)

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

Xano’s picture

marcvangend’s picture