Reconsider support for field-level & instance-level settings with the same name ?

I agree with #1, but you can also call $this->getInstance() instead of checking whether getFieldDefinition() happens to be an instance. Isn't that why ConfigFieldItemInterface has getInstance() as part of its interface? It's exactly the functionality of providing settings forms that makes a field type a "configurable" field type, and for that purpose, and that purpose only, it makes sense to act on the "instance" and "field" config entities directly, rather than via getFieldDefinition().

Even if base fields stay non-configurable and we never display settings forms for them, we might come to a point where Field UI displays a non-editable summary of their settings (like for widgets & formatters). Then the method in charge of displaying this summary output would have the same issues.

Wouldn't Field UI need to choose whether to display the summary as a net summary, or as a partitioned summary of field and instance? If it chooses the former, getFieldDefinition() would be what it needs to act on, for both configurable and nonconfigurable. If it chooses the latter, then it would be meaningless to do that for a nonconfigurable field, so it should only do so for configurable fields, and therefore, safely act on getInstance() and getInstance()->getField().

Ok. Do you think it makes sense to add a parameter to getFieldSetting(s)(), like $setting_source, that could be one of 3 constants: SETTING_SOURCE_DEFINITION (default), SETTING_SOURCE_INSTANCE, SETTING_SOURCE_FIELD? With those constants defined in ConfigFieldItemInterface and the logic in #1 implemented within ConfigFieldItemBase::getFieldSetting(s)()?

An "instance only" mode seems like it would need to support code to spill within Field/FieldInstance::getFieldSettings()

I was thinking $field->settings and $instance->settings would work so long as those are public properties of the config entities. And if we ever make them non-public, we'll need to add method accessors (separate from getFieldSettings()) for it anyway. There's a conceptual difference between "the settings stored in my config entity's yaml file" and "the total effective settings for the field item I'm defining" that we shouldn't remove.

The default behavior + one "field only" option should be enough ?

What this is saying is that a field type in generating an instance settings form could always treat a configured setting for the field as a default for the instance. For example, if an image field's 'default_image' is configured to something at the field level, then on the instance settings form for a new instance, we want that as the default. Which then also means, on saving the instance, that would get saved to the instance's yaml file, and a future change of the field-level 'default_image' wouldn't propagate to the existing instances. I think that's pretty reasonable, but I don't know if we have any Field API history for that being undesirable. If we're ok with this behavior though in all cases where field and instance have the same setting key, then yeah, a boolean instead of 3 constants makes sense.

How's this?

I think it's no-go because getFieldSettings($source) is hard to understand for developers.
Expected argument is $settings_key not the settings storage...

1. +++ b/core/modules/field/lib/Drupal/field/Entity/Field.php
   @@ -561,6 +561,14 @@ public function getBundles() {
   +    $settings = $this->settings + $this->getDefaultSettings();
   +    return $settings;
   
   @@ -576,13 +584,7 @@ public function getFieldType() {
   +    $settings = $this->settings + $this->getDefaultSettings() + $this->getDefaultInstanceSettings();
        return $settings;
   

   no reason in intermediate variable just return $this->settings + ...;

2. +++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigFieldItemBase.php
   @@ -8,12 +8,17 @@
   +  const SETTING_SOURCE_DEFINITION = 0;
   +  const SETTING_SOURCE_FIELD = 1;
   +  const SETTING_SOURCE_INSTANCE = 2;
   
   @@ -45,4 +50,44 @@ public function instanceSettingsForm(array $form, array &$form_state) {
   +  protected function getFieldSettings($source = self::SETTING_SOURCE_DEFINITION) {
   ...
   +    if ($source == self::SETTING_SOURCE_DEFINITION) {
   ...
   +    elseif (!($field_definition instanceof FieldInstanceInterface)) {
   ...
   +    elseif ($source == self::SETTING_SOURCE_FIELD) {
   ...
   +    elseif ($source == self::SETTING_SOURCE_INSTANCE) {
   

   Suppose DX-- here

Would different methods, like getFieldSettingsFromFieldOnly() and getFieldSettingsFromInstanceOnly() be better DX, and if so, any suggestions for better names than that?

Alternatively, we could make every single field type's instanceSettingsForm() method start with:
$settings = $this->getFieldDefinition()->getSettings(), and every settingsForm() method start with $settings = $this->getFieldDefinition()->getField()->getSettings(), and then not need a wrapper method in the base class? But I think we'd then need a comment above that line explaining what we're doing, and we'd be duplicating that for each field type.

What about disallowing the sharing of setting name between field and instance? What are consequences? What would be wrong with that? This will keep think clean and will drop any confusion.

What about disallowing the sharing of setting name between field and instance? What are consequences? What would be wrong with that?

Well, currently, ImageFormatterBase::prepareView() has this code:

$instance = field_info_instance($entity->entityType(), $this->fieldDefinition->getFieldName(), $entity->bundle());
// Use the default for the instance if one is available.
if (!empty($instance['settings']['default_image'])) {
  $fid = array($instance['settings']['default_image']);
}
// Otherwise, use the default for the field.
// Note, that we have to bypass getFieldSetting() as this returns the
// instance-setting default.
elseif (($field = $this->fieldDefinition->getField()) && !empty($field->settings['default_image'])) {
   $fid = array($field->settings['default_image']);
}

That kind of sucks. We should be able to replace all of this with $fid = $this->getFieldSetting('default_image');. Same for any contrib field type that wants to allow a per-instance setting to override a per-field setting.

Here's a patch that I think addresses the DX concerns of #12.2. In other words, for settingsForm() and instanceSettingsForm(), we pass to those functions the settings that that form is responsible for. Everywhere else, getFieldSetting(s)() returns the correctly merged setting(s). Thoughts?

This addresses #12.1. I think that covers all feedback so far, so looking forward to more.

#18: field-type-settings-forms-18.patch queued for re-testing.

Straight reroll.

I disagree with #22. Will write up why in a bit. For now, here's a straight reroll. I'll clean up this patch a bit more, but posting the reroll first, so that I can attach interdiffs with subsequent patches.

This patch removes adding methods to Field[Instance][Interface]. That's out of scope for this issue and detracts from the main point of the patch.

My issue with latest approach is that injecting some $settings into the methods makes the status of the code a bit shaky:
the object still holds a FieldDefinition, but only parts of it should be trusted in those methods, settings should be read from the incoming param.

It's not about trust, it's about meaning. getFieldSettings() is about getting the settings as they should apply to the field data being described by the field definition. It's analogous to getComputedStyle() for working with a DOM element's CSS value in JavaScript. For example, maybe we should add a drupal_alter() step at the end of FieldInstance::getFieldSettings() to allow modules to runtime alter the settings without affecting what's stored in the CMI files. Perhaps we don't need to, since CMI also comes with a context override system, but conceptually, we could add that drupal_alter() and the meaning should still make sense: everything that needs to act on a field data object would just start using those altered settings. However, providing the field or field instance settings forms has nothing to do with that meaning of getFieldSettings(). Instead, the entire purpose of the form is to provide a UI to what is stored in the CMI files. Injecting those settings into the form functions makes sense. Continuing the getComputedStyle() analogy, it would be like a browser plugin that allowed you to directly edit the CSS files of a web page: it would need to present to you the direct values that are in those files, not the net result of getComputedStyle().

There's no logic we can place in there to make "give me the instance setting, if "empty" look in the field" work magically - at least not without reconsidering the auto-population of default values, and I'm frankly not keen on going there only to support what remains an edge case...

That doesn't seem like that big a deal to solve. Surely, we can filter out NULLs either from what we save to the instance's CMI file, or from what is merged in getFieldSettings(). But even if we choose to not do that, and instead drop support for setting inheritance, I still think we should proceed with this patch for the reasons stated above.

ImageItem can even provide a helper getDefaultImage() method that does that and let consuming code be simple.

That's a great workaround if we choose to not support automatic setting inheritance.

Working on #2013679-186: Helper issue for #1953408 (Remove ArrayAccess BC layer) I found that existence of getFieldSettings() is really confusing

• no way to use empty() on setting value, suppose this is a killer of dx
• you can call it on field and instance but result always merged unpredictable settings
• same applies to getFieldSetting()
• a lot of places in core where we have to use ->settings[] to access this by the right way (image and a lot of tests)

Suppose suggested 3rd parameter will bring dx down because most of devs will prefer to access settings directly,
primary for empty() and isset()

So I suggest to get rid of it asap and use plain arrays

Ok, so filed clean-up patch for image module #2087429: Remove deprecated call field_info_instance() in ImageFormatterBase.php

typos

This was bumped to major in 27, but with a proposal that is out of scope. So demoting.