Move field/instance/widget/formatter settings out of annotation / plugin definition

Based on #2005434: Let 3rd party modules store extra configuration in EntityDisplay #11 - #13, this should have been tagged as a beta blocker, as it impacts both the config schemata data model and the configurable field API.

Initial patch converting field formatters .. this will break the tests we have for field formatter settings alter, so should we comment this out and bring those back in #2005434: Let 3rd party modules store extra configuration in EntityDisplay ? Could use a go on the approach .. :)

Code is in 2136197-settings-annotations-remove branch

Right, I copy/pasted from views just to see how fast I could get this into a patch. I'm going to fix the test failures first (which are not formatter-alter related) and then come up with a better name, maybe hasSettings (boolean) and settings() ? Suggestions welcome.

Start on widgets as well

Annotations--

NodeConditionTest fails due to #2170471: ContextAwarePluginBase compromised after commit of Core PluginBase

Fixed ManageDisplayTest and PictureFormatter

Removed settings annotation from TextSummaryOrTrimmedFormatter

NodeConditionTest passes if I apply the patch from #2170471-18: ContextAwarePluginBase compromised after commit of Core PluginBase, so here's a combined patch to see what testbot thinks.

@longwave: Thanks for working on this! Any chance you could push to our field api sandbox - http://drupal.org/node/1736366 - In branch 2136197-settings-annotations-remove - I just gave you access, that would be awesome!

Done - the small fixes I made in #20 and #21 are in the sandbox now. I haven't pushed the NodeConditionTest fix as that belongs to the other issue.

If this issue provides implict test coverage and the other issue is apparently blocked on having test coverage (and has been for a long time), then it might be easier to just merge it in here and move on. Otherwise you need to make that other issue a critical beta blocker, as it blocks one and come up with an arbitrary test for it or delay test coverage on this issue.

Ok, renamed the defineOptions to settings() which makes more sense, especially in light of moving the field and instance settings of field types in methods too.
Hope I got them all. Working on the field types now, which seems to be a little trickier.

1. +++ b/core/lib/Drupal/Core/Field/Annotation/FieldFormatter.php
   --- a/core/lib/Drupal/Core/Field/FormatterPluginManager.php
   +++ b/core/lib/Drupal/Core/Field/FormatterPluginManager.php
   
   +++ b/core/lib/Drupal/Core/Field/PluginSettingsInterface.php
   --- a/core/lib/Drupal/Core/Field/WidgetPluginManager.php
   +++ b/core/lib/Drupal/Core/Field/WidgetPluginManager.php
   

   Just an idea of a follow up: We could provide a base class for field module plugins managers.

2. +++ b/core/lib/Drupal/Core/Field/PluginSettingsInterface.php
   @@ -15,6 +15,22 @@
   +   * Defines the settings for this plugin.
   +   *
   +   * @return array
   +   *   The settings of this plugin.
   

   It would be great if we describe how settings look like. Yeah an array, so probably key => default_value ?

1. +++ b/core/lib/Drupal/Core/Field/PluginSettingsBase.php
   @@ -61,15 +76,19 @@ protected function mergeDefaults() {
        $definition = $this->getPluginDefinition();
   -    return $definition['settings'];
   +    if (!empty($plugin_definition['class'])) {
   

   s/$plugin_definition/$definition/

   Maybe that's even causing the one test failure? :)

2. +++ b/core/lib/Drupal/Core/Field/PluginSettingsInterface.php
   @@ -15,6 +15,22 @@
   +   * Defines the settings for this plugin.
   ...
   +   *   The settings of this plugin.
   

   "for" vs. "of", should be consistent.

3. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
   @@ -28,15 +28,21 @@
   +    $settings['pager_id'] = 0;
   

   I think a brief comment explaining why this is zero would make sense. That wasn't possible with annotations, but now it is.

4. +++ b/core/modules/system/tests/modules/entity_test/entity_test.install
   @@ -32,7 +32,7 @@ function entity_test_install() {
   -      ->setComponent('field_test_text', array('type' => 'text_text'))
   +      ->setComponent('field_test_text', array('type' => 'text_textfield'))
   

   Wow, good catch. Any idea why this didn't trigger an exception or a fatal error?

   Shouldn't we open an issue to ensure that it *will* trigger an exception/fatal error?

That and similar fixes are in #2178795: Allow DiscoveryInterface::getDefinition() to throw an exception for an invalid plugin too I think, which are fixed there because otherwise tests are failing.

#35: That helps. I forgot about that. Never mind then, let's not derail this :)

+++ b/core/lib/Drupal/Core/Field/PluginSettingsBase.php
@@ -31,7 +32,21 @@
+  public static function settings() {
+    return array();
+  }
...
@@ -61,15 +76,19 @@ protected function mergeDefaults() {
   public function getDefaultSettings() {
     $definition = $this->getPluginDefinition();
-    return $definition['settings'];
+    if (!empty($plugin_definition['class'])) {
+      $plugin_class = DefaultFactory::getPluginClass($this->getPluginId(), $definition);
+      return $plugin_class::settings();
+    }
+    return array();

Why do we need a static settings() method and a nonstatic getDefaultSettings() method? Would it make more sense to just make getDefaultSettings() static and use that as our method name?

Patch no longer applies.

Re-roll.

3. There is no reason, 0 is the default value for a pager ID and that's what we set.

Tried to address the other reviews, agreed that getDefaultMethods() seems unecessary, removed that and called static::settings() directly. defaultSettings() would be a better method but PhpStorm didn't want to rename it and I wanted to see how this goes before I go through it manually.

Rerolled. Rejected hunk in the interdiff.

And new hunk after #2218199: Move email and number field types to \Drupal\Core\Field, remove modules

1 fail? Oh, we can totally get this green today! I'll race you andypost!

So are we not pulling settings out of annotation for fields? For instance FieldItemBase still has a getSettings method that pulls settings from an annotation, such as with DecimalItem.

Here is the list of Classes that still have settings in annotations:

core/lib/Drupal/Core/Field/Plugin/Field/FieldType/DecimalItem.php
core/lib/Drupal/Core/Field/Plugin/Field/FieldType/StringItem.php
core/lib/Drupal/Core/Field/Plugin/Field/FieldType/UriItem.php
core/lib/Drupal/Core/Field/Plugin/Field/FieldType/UuidItem.php
core/modules/comment/lib/Drupal/comment/Plugin/Field/FieldType/CommentItem.php
core/modules/datetime/lib/Drupal/datetime/Plugin/Field/FieldType/DateTimeItem.php
core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Plugin/Field/FieldType/ShapeItem.php
core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Plugin/Field/FieldType/TestItem.php
core/modules/file/lib/Drupal/file/Plugin/Field/FieldType/FileItem.php
core/modules/filter/lib/Drupal/filter/Plugin/Filter/FilterHtml.php
core/modules/filter/lib/Drupal/filter/Plugin/Filter/FilterUrl.php
core/modules/image/lib/Drupal/image/Plugin/Field/FieldType/ImageItem.php
core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListBooleanItem.php
core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListFloatItem.php
core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListIntegerItem.php
core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListTextItem.php
core/modules/responsive_image/lib/Drupal/responsive_image/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Field/FieldType/TaxonomyTermReferenceItem.php
core/modules/text/lib/Drupal/text/Plugin/Field/FieldType/TextItem.php

I don't think the FieldType classes were ever attempted so far in this patch; in #28 swentel said they were working on this but since then #28 has only been repeatedly rerolled to keep up with HEAD. The Filter plugins are not relevant to this issue and ResponsiveImageFormatter looks like the only FieldFormatter that has been missed at present.

Test Drupal\field_ui\Tests\ManageDisplayTest still broken because settings from display are "leaks" into plugin
So hasSettings() is true...
This bug also re-producable in UI (change formatter for body field to 'Trimmed' and save display, then changing the formatter back to 'Default' will show a "wheel")

I found hasSettings() useless because it's much easy to check self::settings() or $plugin->settings().
Also this method does not guarantee that plugin settings are not merged as all above patches shows.

Not sure we need to convert field types here, maybe follow-up?

pushed 2136197-settings-annotations-remove-andypost branch

Here is the bleed andypost mentions in #49. I'm investigating where the bleed is coming from.

TestFieldNoSettingsFormatter::settings:
Array
(
)

FormatterPluginManager::getDefaultSettings:
Array
(
    [0] => Drupal\field_test\Plugin\Field\FieldFormatter\TestFieldNoSettingsFormatter
    [1] => Array
        (
        )

)

PluginSettingsBase::hasSettings:
Array
(
    [0] => field_no_settings
    [1] => Array
        (
            [field_empty_setting] => non empty setting
            [field_test_formatter_settings_form_alter] => 
        )

)

The patch in #51 passes the ManageDisplayTest, but it makes me nervous to just rip out a method that should work. Are we masking a deeper problem?

When the plugin instance is created in DisplayOverviewBase, the $display_options for the plugin instance are polluted with the options from the previous display:

$plugin = $this->getPlugin($field_definition, $display_options);

Array (
   [type] => field_no_settings
   [weight] => -3
   [settings] => Array
   (
     [field_empty_setting] => non empty setting
     [field_test_formatter_settings_form_alter] => 
   )
  [label] => above
)

I'm looking at whether the logic in the DisplayOverviewBase::buildFieldRow has a bug.

This looks really suspect.

if (isset($form_state['plugin_settings'][$field_name])) {
  $display_options['settings'] = $form_state['plugin_settings'][$field_name];
}

It's not masking the problem, it's related to #2005434: Let 3rd party modules store extra configuration in EntityDisplay
Display holds settings from previous formatter, so probably the proper fix for that would be "proper merge"

Let's see how this affects core, the test fixed too

Awesome, #57 addresses the issue I raised in #54.

Fix broken test
Starting to convert field-type settings

60: 2136197-settings-annotations-remove-andypost-60.patch queued for re-testing.

wrong patch

Converted instance settings to instanceSettings except content_translation module hack - module needs new storage for it's properties:

/**
 * Implements hook_field_info_alter().
 */
function content_translation_field_info_alter(&$info) {
  foreach ($info as &$field_type_info) {
    // By default no column has to be synchronized.
    $field_type_info['settings'] += array('translation_sync' => FALSE);
    // Synchronization can be enabled per instance.
    $field_type_info['instance_settings'] += array('translation_sync' => FALSE);
  }
}

Also attaching patch with conversion for widget and formatter only (*wf)
All pushed to 2136197-settings-annotations-remove-andypost branch

+++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListIntegerItem.php
@@ -19,11 +19,7 @@
- *   default_formatter = "list_default",
...
+ *   default_formatter = "list_default"

Minor, but Annotations now support trailing commas, so that could be left alone.

Also minor but I actually liked the hasSettings() method. If we make it use $this->settings() instead of $this->settings it should work regardless of the display problem here, right?

Also, writing that, I wonder why we're going with settings() as method name instead of getSettings()?

Because getSettings() already exists as an instance method. I think it should be defaultSettings(). We're apparently leaving out the get for static methods, see schema()/propertyDefinitions().

Fix 2 tests and cleanup #68.1
I do not see a reason in hasSettings() that calls static method and doing the same

All left tests are cause by content translations except EntityFieldTest:
there's some strange code \Drupal\Core\TypedData\TypedDataManager::getPropertyInstance()

EDIT getSettings() is tricky - it merges defaults with field and instance level settings

Next steps:
- clean-up config schema
- implement storage for content translation
- fix tests

Currently content translation module uses config storage for all it's definitions so probably better to add field properties (settings) translation to current one
content_translation_get|set_config() wrappers seems works

I found the cause of EntityFieldTest failures - the settings is not plain associative array, it's values could be nested arrays.
Suppose better to file new issue for that.

diff --git a/core/lib/Drupal/Core/TypedData/TypedDataManager.php b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
index 88d63ec..7ddc532 100644
--- a/core/lib/Drupal/Core/TypedData/TypedDataManager.php
+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
@@ -259,6 +259,13 @@ public function getPropertyInstance(TypedDataInterface $object, $property_name,
     }
     $key = $definition->getDataType();
     if ($settings = $definition->getSettings()) {
+      // Do not throw notices if settings values is array.
+      foreach ($settings as $k => $v) {
+        if (is_array($v)) {
+          debug($settings, "key: $k");
+          $settings[$k] = '';
+        }
+      }
       $key .= ':' . implode(',', $settings);
     }
     $key .= ':' . $object->getPropertyPath() . '.';

EDIT At least ER instance settings has handler_settings that value is array

The proper fix in this context, only content translation left here to fix

let's see how this removal affects tests

Filed 2 follow-ups:
#2221577: Fix assumption that field settings is not a nested array
#2221559: Unify target_bundle vs target_bundles settings for entity reference fields

Draft change notice for this issue: https://drupal.org/node/2221691

Patch just needs some work to clean-up git grep translation_sync remains and reviews ;)

+++ b/core/modules/content_translation/content_translation.module
@@ -790,18 +790,6 @@ function content_translation_form_field_ui_field_instance_edit_form_alter(array
- * Implements hook_field_info_alter().
- */
-function content_translation_field_info_alter(&$info) {
-  foreach ($info as &$field_type_info) {
-    // By default no column has to be synchronized.
-    $field_type_info['settings'] += array('translation_sync' => FALSE);
-    // Synchronization can be enabled per instance.
-    $field_type_info['instance_settings'] += array('translation_sync' => FALSE);
-  }
-}
-
-/**

Regarding this change andypost noted: "I found that content translation stores its data in own yml"

I don't see how content translation in hooking into the settings of a FieldItem, so I'd like to get some input from folks working on multi-lingual about this change.

For instance in content_translation_field_sync_widget the following code is run:

$translation_sync = $field->getSetting('translation_sync');

Because translation_sync is no longer set through the info alter, this key won't return a value.

I think you might have uncovered a gap in the tests for translation syncing.

So, everything about the patch except for the content_translations looks good. Regarding content_translations, this is from the issue summary:

In the case of content_translation, it means 'translation_sync' should not be an altered-in field setting, but a separate piece of config, associated to a given [entity_type, bundle, field_name], tracked and managed by content_translation whichever way it sees fit.

We can't leave this to a follow-up unfortunately, since it will introduce a regression.

about #82-83 - currently content translation duplicates its settings:
1) actual settings from /admin/config/regional/content-language are stored in \Drupal::config('content_translation.settings') for each entity, bundle, field, field property
2) the data from 1 is duplicated into 'translation_sync' property of each configurable field and instance

Patch #78 shows that we can remove the initialization of this setting but this still applies in content_translation_save_settings():

          foreach ($bundle_settings['columns'] as $field_name => $column_settings) {
            $instance = field_info_instance($entity_type, $field_name, $bundle);
            if ($instance->isTranslatable()) {
              $instance->settings['translation_sync'] = $column_settings;
            }
            // If the field does not have translatable enabled we need to reset
            // the sync settings to their defaults.
            else {
              unset($instance->settings['translation_sync']);
            }
            $instance->save();
          }

The only question here for me - if we remove this optimization in favor of relying on global config what is a performance impact?

I really think the translation_sync tests have a gap in coverage. The field instance setting is being used to track whether a field is translatable.

If I'm right, I should be able to write a test that fails under the conditions I surmise exist. Trying that.

Ok, I think I know what we need to do. content_translation.module has a function content_translation_save_settings. This function is called when the form language_content_settings_form at /admin/config/regional/content-language is submitted.

At this point, the settings for translation_sync are injected into each field instance in the bundles that have been enabled for translation.

// Store whether fields have translation enabled or not.
if (!empty($bundle_settings['columns'])) {
  foreach ($bundle_settings['columns'] as $field_name => $column_settings) {
    $instance = field_info_instance($entity_type, $field_name, $bundle);
    if ($instance->isTranslatable()) {
      $instance->settings['translation_sync'] = $column_settings;
    }
    // If the field does not have translatable enabled we need to reset
    // the sync settings to their defaults.
    else {
      unset($instance->settings['translation_sync']);
    }
    $instance->save();
  }
}

Using the Article image field as an example, this results in the field.instance.node.article.field_image.yml file being updated to include translation_sync settings

id: node.article.field_image
...
settings:
  file_directory: field/image
  file_extensions: 'png gif jpg jpeg'
  max_filesize: ''
  max_resolution: ''
  min_resolution: ''
  alt_field: true
  title_field: false
  alt_field_required: false
  title_field_required: false
  default_image:
    fid: null
    alt: ''
    title: ''
    width: null
    height: null
  handler: default
  translation_sync:
    alt: alt
    title: title
    file: 0
field_type: image

Rather than storing this translation_sync information in the field instance config, we need to save it in a config file that the content_translation module controls and associate the configs to the corresponding field through entity_type, bundle and field_name.

The existing tests are still valid because we won't be altering the behavior of translation syncing.

We might be able to use @FieldFormatter plugins as an analogical model. If we create, let's say, @TranslationSync plugins, we'd have to create a type per variation in fields. We'd probably be able to group field_types in the @TranslationSync plugins if the fields have similar translatable properties, for example, just a text field.

I think this is the pattern we have in core to "extend" a field's settings and behavior.

There's a special issue for that #2143291: Clarify handling of field translatability

For the scope of the issue the removal of content_translation_field_info_alter() is enough, imo.

Suppose only @plach can drop a light on c_t magic

I still think that public static settings() should be defaultSettings(), no? :)

hm, probably...
but blocks plugins are using defaultConfiguration()

so defaultInstanceSettings() vs defaultInstanceConfiguration()

renamed all:
1) settings() ->defaultSettings()
2) instanceSettings() -> defauktInstanceSettings()

about #82-83 - currently content translation duplicates its settings:
1) actual settings from /admin/config/regional/content-language are stored in \Drupal::config('content_translation.settings') for each entity, bundle, field, field property
2) the data from 1 is duplicated into 'translation_sync' property of each configurable field and instance

This is not quite accurate. The information stored in content_translation.settings.yml indicates whether a field is translatable, not its properties that should be synced.

content_translations.settings.yml:

node:
  article:
    content_translation:
      enabled: true
      fields:
        title: true
        body: true
        comment: true
        field_image: true
        field_tags: true

field.instance.node.article.field_image.yml

settings:
  ...
  translation_sync:
    alt: alt
    title: title
    file: 0

What might be duplicated is how the translation_sync settings get into a field instance settings file. This is why I'm still hesitant to rip out content_translation_field_info_alter as #78 did.

Yes, so we still need some field&instance related storage for "extra" data like #2005434: Let 3rd party modules store extra configuration in EntityDisplay

1) Field item propertyDefinitions() could "mark" properties as translatable, as entity base fields doing.
2) otoh this data is used only by \Drupal\content_translation\FieldTranslationSynchronizer::synchronizeFields() so could be easily moved to some other storage
3) Let's think about contrib that wanna store some "own-extra-meta-data" on fields and instances

Let's think about contrib that wanna store some "own-extra-meta-data" on fields and instances

I think you're spot-on andypost. There's going to be a beta-blocker followup to this issue. I think we can get this one closed with the code changes you've already written without regressions, but we won't be able to close this hole in this issue...it's just too big and the patch in this issue and already quite large.

the NodeType config entity does include a one-off mechanism to allow 3rd-party-added properties

Suppose you mean User data that stored additions in "user.data" service (see \Drupal\user\Entity\User::preSave())
But NodeType has strict node.settings.node schema definition (at least menu stores own "menu.entity.node.$type" yml see menu_parent_options())

After some more testing, I got to the bottom of my doubt. It turns out that content_translation_field_info_alter is invoked in order to add the setting translation_sync to new fields on a node. Without this hook implementation, new fields don't get this setting. I took some screenshots that prove this.

The problem is hook_field_info_alter is invoked during the process to collect a fields definitions, which no longer include settings, so the attempt in content_translation_field_info_alter to merge an array with an existing settings array lead to an undefined index error.

What we need is a hook that gets invoked after definitions and settings have been merged but before the config entity is saved. In this case we can use hook_entity_presave. content_translation already implements this hook to synchronize translatable fields before save. Now we ensure the settings are there as well. This does not resolve that we're still saving a third-party settings for a field in its config file, thus mucking with its schema. But this has always been the case. At least now we can (if the tests pass! I ran a couple locally that pass) move forward on this issue. This was the only reservation I had about it.

"extra settings about a field / instance" do not have the same indirection constraint though, and it's fairly straightforward to store 3rd party settings about [entity_type].[field_name] or [entity_type].[bundle].[field_name] in a custom CMI record managed by the 3rd party.

andypost, this suggestion by yched seems promising. It means we wouldn't have a schema associated with the field information. It would just be blob data, right?

#99 looks good, I'd left removal of translation_sync to follow-up because it's not strictly in the scope of the patch.

Seems @yched agree on that:

"Let 3rd party modules store extra configuration in FieldConfig / FieldInstanceConfig" isn't as critical IMO.

Anyway we need to clean-up schema files about field and instance settings for knows field types.

EDIT no need to change schema that still needed to save config of the field and instance

Again, sorry for not replying earlier, it's really hard for me to find time for core these days.

I think you guys properly addressed the issue so far (thanks @jessebeach for the thorough research work :) and the changes pertaining CT in the patch look reasonable to me.

Just one more thing about the removal of the CT keys from the various annotations: @yched's summary makes some convincing points about the need to do that, so I won't argue about this, but the reason those settings are attached to field annotations is that they are specific to every field-type. CT cannot automatically provide that information for every field-type contrib may provide, so if we want to remove them we need to introduce a new way for field-type-defining modules to integrate with CT and provide it the information it needs. I guess this is what @jessebeach was referring to in #87 with @TranslationSync plugins. Those might be overkill given the kind of information we need to provide, but at very least a basic _info() hook would be needed.

We may want to consider a generic field info hook: extending field annotations would be consistent with what we are already doing for entities: annotation + info() hook + info_alter() hook.

To be more clear:

• what in HEAD are instance settings can definitely be tracked by CT in its own configuration
• what in HEAD are field settings are not configuration but definition and should ideally live in the annotation data or some kind of extension of it.

CT cannot automatically provide that information for every field-type contrib may provide
@plach Why? because of unpredictable field properties or "field_collumns"?

Exactly :)

Actually, since CT has no need to special-case Field UI fields we could just leverage entity field info.

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.php
@@ -66,7 +66,7 @@ protected function defineOptions() {
-   *   \Drupal\views\Plugin\Block\ViewsBlock::settings().
+   *   \Drupal\views\Plugin\Block\ViewsBlock::defaultSettings().

Hmm, this seems unrelated. It also should probably be defaultConfiguration()

We should also remove the 'Widget' settings from the annotation class - and add it to the change notice.

This looks sane to me so far. The question is whether it's allowed to open 2 new beta blocker issues ? :)

Discussed with @jessebeach and @swentel. Based on what @swentel described, I think it's definitely appropriate to split this issue into three parts to make it more reviewable and unblock other work, so @swentel is filing two followup parts for it as per #102.

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.php
@@ -66,7 +66,7 @@ protected function defineOptions() {
-   *   \Drupal\views\Plugin\Block\ViewsBlock::settings().
+   *   \Drupal\views\Plugin\Block\ViewsBlock::defaultSettings().

Hmm, this seems unrelated. It also should probably be defaultConfiguration()

It should be defaultConfiguration. This code here isn't fantastic, but it's what existed before this patch and this isn't the place to refactor it.

We should also remove the 'Widget' settings from the annotation class - and add it to the change notice.

Removed and the changed notice is updated to reflect this change.

Follow ups filed:

• #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration
• #2224779: Move 'column_groups' out of settings in a dedicated annotation

(hopefully clear enough titles)

Ok, let's do this then ?

Re-roll after #2191709: Remove the "configurable" flag on field types (pushed to sandbox)
Also, we can't use hook_field_info_alter() before #2221577: Fix assumption that field settings is not a nested array so #99 content_translation_entity_presave() is nice workaround

@todo added

New patch, addressed all. Some comments

1. Well, that would be massive change for this patch. Don't have a strong opinion, but I'd rather do that in follow up if you feel strongly about it ;)
3. Looks weird indeed, reverted it, we'll see if this still turns back green.
4. I did a quick check and to me this seems fine, unless I'm missing something

4) fixed, @swentel see https://drupal.org/node/1354#types
Also merge after #2169983: Move type-specific logic from ListItemBase to the appropriate subclass

Will try to fix 1) if green

I've got #1 fixed already. Merging in....

+++ b/core/lib/Drupal/Core/Field/PluginSettingsBase.php
@@ -63,7 +63,7 @@ public function getSetting($key) {
+    // Use only a settings defined by plugin.

We should just remove this line

Refactored the settings functions' bodies to return an array of settings with the defaults merged in, as per #1 in #114.

Also removed the line mentioned by swentel in #120.

Annnnnd, now the line mentioned in #120 is removed.

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/ConfigurableEntityReferenceItem.php
@@ -0,0 +1,10 @@
+-    // The instance settings 'handler_settings.target_bundles' used for this.
++    // The instance setting 'handler_settings.target_bundles' is used for this.

oops

Otherwise looking great.

@yched, I'll let you pull the trigger on this.

andypost found an unconverted settings annotation in ListBooleanItem. I've refactored it to the array() + defaults() format suggested in #114/#1.

A rejection file slipped in. Still RTBC though ! :)

Committed 7f61620 and pushed to 8.x. Thanks!

One more place found

Ah right, good catch.

Committed 4c109a5 and pushed to 8.x. Thanks!