Spin off from #1975112: Config schema mechanisms cannot reflect alterable config trees and #2130811-17: Use config schema in saving and validating configuration form to ensure data is consistent and correct .

Currently, the collection of available "settings" for a field/instance/widget/formatter are part of field type/widget/formatter plugin definition (in annotations), and are thus alterable like the rest of the plugin definition.
In D7, a lot of contrib modules have used that to add new "settings" of their own, that thus get conveniently saved and loaded along field/instance/widget/formatter definitions.
In current core D8, content_translation does the same to add its 'translation_sync' field setting, applicable to all field types.

Problem: there is no way to provide config schema for this (detailed explanation / discussion in #1975112: Config schema mechanisms cannot reflect alterable config trees).
This was discussed back in Portland, and this is now a major issue because #2130811: Use config schema in saving and validating configuration form to ensure data is consistent and correct relies heavily on config schema info.

Summary of the discussion in Portland:
- A config entity cannot hold properties it doesn't know about, because then it cannot provide a schema for them. The rule is : "if you have additional settings of your own to store about my config entity, store them yourselves in your own config records for which you can provide a schema".
- More specifically, the config schema for the 'settings' part of a field/instance/widget/formatter configuration is statically provided by the module providing the field type/widget/formatter, and the schema mechanism doesn't let any other 3rd party code provide additional schema info about it. We cannot ask *all* field type modules to provide schema info about the 'translation_sync' property that *might* be present in field.field.*.yml CMI files in case c_t.module is enabled.
- Those "extra settings" are in fact not settings for the plugin anyway, since the plugin class won't suddenly and magically know and use them. They are actually settings for 3rd party code (typically implementations of hooks fired when after the plugin did its job), and storing them in 'settings' is only a convenient way to have free storage and loading. But we just can't support that in D8 anymore.
[edit: #2005434: Let 3rd party modules store extra configuration in EntityDisplay is here to bring this functionnality back for widgets & formatter settings in EntityDisplay config entities, in a structured way that allows config schema - see over there why it makes sense for widgets & formatters]

Consequences:
- The set of supported field/instance/formatter/widget settings, should *not* be alterable, and we should probably remove them from the plugin definitions (that's what makes them alterable) and into static methods in the plugin classes (that's how Views always did it).
- 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.

Coding happens in 2136197-settings-annotations-remove branch

CommentFileSizeAuthor
#133 2136197-follow-up.patch794 bytesandypost
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,726 pass(es). View
#130 interdiff.txt1.02 KBswentel
#130 remove-settings-annotation-2136197-130.patch99.18 KBswentel
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,728 pass(es). View
#125 remove-settings-annotation-2136197-123.patch100.19 KBjessebeach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#125 interdiff.txt866 bytesjessebeach
#122 interdiff.txt555 bytesjessebeach
#122 remove-settings-annotation-2136197-122.patch100.22 KBjessebeach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#121 remove-settings-annotation-2136197-120.patch100.26 KBjessebeach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#121 interdiff.txt46.08 KBjessebeach
#118 2136197-settings-annotations-remove-andypost-118.patch100.69 KBandypost
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#118 interdiff.txt2.35 KBandypost
#116 interdiff.txt11.39 KBswentel
#116 2136197-116.patch100.25 KBswentel
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,757 pass(es), 0 fail(s), and 8 exception(s). View
#113 2136197-settings-annotations-remove-andypost-113.patch98.24 KBandypost
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,675 pass(es). View
#113 interdiff.txt977 bytesandypost
#111 2136197-settings-annotations-remove-andypost-111.patch98.1 KBandypost
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,764 pass(es). View
#107 remove-settings-annotation-2136197-107.patch98.13 KBjessebeach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,650 pass(es). View
#107 interdiff.txt1.35 KBjessebeach
#99 interdiff.txt1.03 KBjessebeach
#99 remove-settings-annotations-2136197-97.patch97.46 KBjessebeach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,492 pass(es). View
#99 content_translation_settings.png35.35 KBjessebeach
#99 no-patch.png23.96 KBjessebeach
#99 with-patch.png20.71 KBjessebeach
#93 2136197-settings-annotations-remove-andypost-93.patch96.93 KBandypost
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,522 pass(es). View
#86 content-translation.png57.92 KBjessebeach
#78 2136197-settings-ct-remove.patch95.92 KBandypost
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,417 pass(es). View
#78 interdiff.txt943 bytesandypost
#76 2136197-settings-annotations-remove-andypost-76.patch95 KBandypost
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,751 pass(es), 15 fail(s), and 14 exception(s). View
#76 interdiff.txt2.09 KBandypost
#70 2136197-settings-annotations-remove-andypost-69.patch93.55 KBandypost
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,695 pass(es), 16 fail(s), and 16 exception(s). View
#70 interdiff.txt7.94 KBandypost
#66 2136197-settings-annotations-remove-wf-66.patch49.3 KBandypost
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,429 pass(es). View
#66 2136197-settings-annotations-remove-andypost-66.patch88.13 KBandypost
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,822 pass(es), 23 fail(s), and 25 exception(s). View
#66 interdiff.txt26.94 KBandypost
#64 2136197-settings-annotations-remove-andypost-64.patch68.99 KBandypost
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,950 pass(es), 90 fail(s), and 69 exception(s). View
#64 interdiff.txt2.43 KBandypost
#60 2136197-settings-annotations-remove-andypost-60.patch67.89 KBandypost
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#60 interdiff.txt19.69 KBandypost
#60 interdiff-fix-test.txt847 bytesandypost
#57 2136197-settings-annotations-remove-andypost-57.patch48.8 KBandypost
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,337 pass(es), 1 fail(s), and 0 exception(s). View
#57 interdiff.txt2.02 KBandypost
#56 interdiff.txt1.09 KBandypost
#51 2136197-field-settings-50.patch47.81 KBandypost
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,415 pass(es). View
#49 2136197-field-settings-49.patch48.09 KBandypost
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,343 pass(es), 1 fail(s), and 0 exception(s). View
#49 interdiff.txt1.9 KBandypost
#43 2136197-field-settings-42.patch46.78 KBandypost
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,422 pass(es), 1 fail(s), and 2 exception(s). View
#43 interdiff.txt3.91 KBandypost
#41 settings-out-of-annotations-2136197-41.patch42.87 KBjessebeach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,325 pass(es), 15 fail(s), and 18 exception(s). View
#41 interdiff.txt532 bytesjessebeach
#39 2136197-39-interdiff.txt1.85 KBBerdir
#39 2136197-39.patch48.19 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,379 pass(es), 9 fail(s), and 1 exception(s). View
#28 2136197-28.patch47.51 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 64,600 pass(es), 1 fail(s), and 0 exception(s). View
#24 2136197-field-settings-21-with-2170471-18.patch43.77 KBlongwave
PASSED: [[SimpleTest]]: [MySQL] 64,435 pass(es). View
#21 interdiff.txt625 byteslongwave
#21 2136197-field-settings-21.patch42.76 KBlongwave
FAILED: [[SimpleTest]]: [MySQL] 64,439 pass(es), 1 fail(s), and 0 exception(s). View
#20 interdiff.txt2.52 KBlongwave
#20 2136197-field-settings-20.patch42.15 KBlongwave
FAILED: [[SimpleTest]]: [MySQL] 64,403 pass(es), 1 fail(s), and 0 exception(s). View
#17 2136197-17.patch39.63 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 64,350 pass(es), 1 fail(s), and 5 exception(s). View
#15 2136197-15.patch39.39 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 64,355 pass(es), 2 fail(s), and 9 exception(s). View
#13 2136197-13.patch39.59 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#11 2136197-11.patch20.92 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 63,150 pass(es), 649 fail(s), and 371 exception(s). View
#6 2136197-6.patch20.27 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 64,226 pass(es), 122 fail(s), and 121 exception(s). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

yched’s picture

Note: For "extra settings" for widgets and formatters specifically, the "store them yourself" mantra is tricky because of the "per view mode" & "default display" logic specific to entity displays. So having those "extra settings" stored in the EntityDisplay somehow would really be a bonus. It just can't be in "settings", and needs to be in a separate entry, clustered by 'providing module', so that we can route to the correct schema info.

This is what #2005434: Let 3rd party modules store extra configuration in EntityDisplay is for.

yched’s picture

@alexpott: not fully sure this will solve #2130811: Use config schema in saving and validating configuration form to ensure data is consistent and correct though.

In the scenario described above, c_t's 'translation_sync' property would still be form_altered into the "Field edit form", it would just be saved to a separate c_t config file instead of field.field.*.yml. Would #2130811: Use config schema in saving and validating configuration form to ensure data is consistent and correct still be able to masssage submitted values according to the correct schema in this case ?

yched’s picture

Issue summary: View changes
yched’s picture

xjm’s picture

Priority: Normal » Critical
Issue tags: +beta blocker

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.

swentel’s picture

Status: Active » Needs review
FileSize
20.27 KB
FAILED: [[SimpleTest]]: [MySQL] 64,226 pass(es), 122 fail(s), and 121 exception(s). View

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

swentel’s picture

Issue summary: View changes
yched’s picture

Yay, thanks for picking this up !

At this point in the cycle, though, I'm not too hot on renaming "settings" to "options" - we have a whole collextion of methods named xxSettings() / xxSetting() all over the place for widgets, formatters, field types...

Status: Needs review » Needs work

The last submitted patch, 6: 2136197-6.patch, failed testing.

swentel’s picture

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.

swentel’s picture

Status: Needs work » Needs review
FileSize
20.92 KB
FAILED: [[SimpleTest]]: [MySQL] 63,150 pass(es), 649 fail(s), and 371 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 11: 2136197-11.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
39.59 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Start on widgets as well

Status: Needs review » Needs work

The last submitted patch, 13: 2136197-13.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
39.39 KB
FAILED: [[SimpleTest]]: [MySQL] 64,355 pass(es), 2 fail(s), and 9 exception(s). View

Annotations--

Status: Needs review » Needs work

The last submitted patch, 15: 2136197-15.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
39.63 KB
FAILED: [[SimpleTest]]: [MySQL] 64,350 pass(es), 1 fail(s), and 5 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 17: 2136197-17.patch, failed testing.

longwave’s picture

longwave’s picture

Status: Needs work » Needs review
FileSize
42.15 KB
FAILED: [[SimpleTest]]: [MySQL] 64,403 pass(es), 1 fail(s), and 0 exception(s). View
2.52 KB

Fixed ManageDisplayTest and PictureFormatter

longwave’s picture

FileSize
42.76 KB
FAILED: [[SimpleTest]]: [MySQL] 64,439 pass(es), 1 fail(s), and 0 exception(s). View
625 bytes

Removed settings annotation from TextSummaryOrTrimmedFormatter

The last submitted patch, 20: 2136197-field-settings-20.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 21: 2136197-field-settings-21.patch, failed testing.

longwave’s picture

Status: Needs work » Needs review
FileSize
43.77 KB
PASSED: [[SimpleTest]]: [MySQL] 64,435 pass(es). View

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.

swentel’s picture

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

longwave’s picture

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.

Berdir’s picture

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.

swentel’s picture

FileSize
47.51 KB
FAILED: [[SimpleTest]]: [MySQL] 64,600 pass(es), 1 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 28: 2136197-28.patch, failed testing.

yched’s picture

Issue summary: View changes
yched’s picture

dawehner’s picture

  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 ?

Wim Leers’s picture

  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?

Berdir’s picture

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.

yched’s picture

@Wim #33. 4 :
For the specific case of widgets / formatters, [Widget|Formatter]PluginManager::getInstance() specifically avoids failing when the requested plugin is unknown, and uses the 'default_widget' instead.
That behavior was introduced in CCK D6 to avoid white pages just because the module providing the wdget has been removed. Unlike the case of "the field type is missing", which is serious and we go great lengths to avoid, we try to consider "widget / formatter missing" as a non breaking condition.

I guess that could be reconsidered, but that's probably outside the scope of that issue.

Wim Leers’s picture

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

effulgentsia’s picture

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

martin107’s picture

Issue tags: +Needs reroll

Patch no longer applies.

Berdir’s picture

Status: Needs work » Needs review
FileSize
48.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,379 pass(es), 9 fail(s), and 1 exception(s). View
1.85 KB

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.

Status: Needs review » Needs work

The last submitted patch, 39: 2136197-39.patch, failed testing.

jessebeach’s picture

Status: Needs work » Needs review
FileSize
532 bytes
42.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,325 pass(es), 15 fail(s), and 18 exception(s). View

Rerolled. Rejected hunk in the interdiff.

Status: Needs review » Needs work

The last submitted patch, 41: settings-out-of-annotations-2136197-41.patch, failed testing.

andypost’s picture

FileSize
3.91 KB
46.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,422 pass(es), 1 fail(s), and 2 exception(s). View
andypost’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

Status: Needs review » Needs work

The last submitted patch, 43: 2136197-field-settings-42.patch, failed testing.

jessebeach’s picture

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

jessebeach’s picture

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
longwave’s picture

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
1.9 KB
48.09 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,343 pass(es), 1 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 49: 2136197-field-settings-49.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
1.9 KB
47.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,415 pass(es). View

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

jessebeach’s picture

Status: Needs review » Needs work

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] => 
        )

)
jessebeach’s picture

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?

jessebeach’s picture

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.

jessebeach’s picture

This looks really suspect.

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

FileSize
1.09 KB

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"

andypost’s picture

Status: Needs work » Needs review
FileSize
2.02 KB
48.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,337 pass(es), 1 fail(s), and 0 exception(s). View

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

jessebeach’s picture

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

Status: Needs review » Needs work

The last submitted patch, 57: 2136197-settings-annotations-remove-andypost-57.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
847 bytes
19.69 KB
67.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Fix broken test
Starting to convert field-type settings

Status: Needs review » Needs work

The last submitted patch, 60: 2136197-settings-annotations-remove-andypost-60.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 60: 2136197-settings-annotations-remove-andypost-60.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
2.43 KB
68.99 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,950 pass(es), 90 fail(s), and 69 exception(s). View

wrong patch

Status: Needs review » Needs work

The last submitted patch, 64: 2136197-settings-annotations-remove-andypost-64.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
26.94 KB
88.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,822 pass(es), 23 fail(s), and 25 exception(s). View
49.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,429 pass(es). View

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

The last submitted patch, 66: 2136197-settings-annotations-remove-andypost-66.patch, failed testing.

tstoeckler’s picture

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

Berdir’s picture

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

andypost’s picture

FileSize
7.94 KB
93.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,695 pass(es), 16 fail(s), and 16 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 70: 2136197-settings-annotations-remove-andypost-69.patch, failed testing.

yched’s picture

Cant look at code atm, but don't mix:
- the static method that defines the collection of settings and their default values for a given plugin class
- the accessors for the runtime settings values of a specific instantiated plugin (object). hasSetting() is in that area (although 5.4 syntax sugar makes it slghtly less needed)

(sorry if i'm just adding noise, i'm only following from afar)

andypost’s picture

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

andypost’s picture

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

andypost’s picture

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

andypost’s picture

Status: Needs work » Needs review
FileSize
2.09 KB
95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,751 pass(es), 15 fail(s), and 14 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 76: 2136197-settings-annotations-remove-andypost-76.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
943 bytes
95.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,417 pass(es). View

let's see how this removal affects tests

andypost’s picture

jessebeach’s picture

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

andypost’s picture

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

jessebeach’s picture

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

jessebeach’s picture

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.

andypost’s picture

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?

jessebeach’s picture

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.

jessebeach’s picture

Status: Needs review » Needs work
FileSize
57.92 KB

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.

Screenshot of the language_content_settings_form form

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.

jessebeach’s picture

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.

yched’s picture

You probably want to check with @plach, but I'm not sure why we'd need various sync plugins for various field types ?

andypost’s picture

Assigned: Unassigned » plach
Status: Needs work » Needs review
Related issues: +#2143291: Clarify handling of field translatability

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

Berdir’s picture

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

andypost’s picture

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

so defaultInstanceSettings() vs defaultInstanceConfiguration()

yched’s picture

- re: "settings" / "configuration"

Yeah, a couple other plugin types use "configuration", some others use "options", the whole of Field API uses "settings" for its various plugin types.
That's unfortunate (I opened an issue like two years ago to try to unify this in advance, can't find it atm, but sadly it didn't go anywhere), but as I wrote in #8 above, "settings" is pretty well rooted in our APIs, and I don't think we want to rename it in this issue, and possibly not in D8 either.

Also, renaming "settings -> configuration" would be pretty confusing in our case, clashes with the "base fields / configurable fields" nomenclature.
*If* we rename/unify with something, "options" would be a better choice IMO. Yet again, not sure why it shouldn't be the other plugin types that unify to "settings" :-)

So yeah, for now +1 on defaultSettings()

- re: translation sync

Note that #2143291: Clarify handling of field translatability is about clarifying the flow of the 'translatable' property on fields / instances, and is not directly related to the translation_sync "setting" that is problematic here.

andypost’s picture

FileSize
96.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,522 pass(es). View

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

jessebeach’s picture

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.

andypost’s picture

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

jessebeach’s picture

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.

yched’s picture

Note: The CMI guideline so far has always been "if you have 3rd party configuration to store about my config entities, store them yourself in your own config records, I'm not extending my config schema for you".

As explained in the issue summary over there, #2005434: Let 3rd party modules store extra configuration in EntityDisplay was opened because, for the specific case of formatters / widgets in EntityDisplays, it would be very painful for 3rd party code to store "extra settings about a formatter for a field in a $bundle / $view mode" themselves in a dedicated CMI record of their own: they would basically need to reproduce the "use Display for either $view_mode or 'default'" indirection logic encompassed in Entity(Form|View)Display::collectRenderDisplay() - which in most cases would probably end up badly implemented or simply overlooked.

"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. So providing a mechanism to "Let 3rd party modules store extra configuration in FieldConfig / FieldInstanceConfig" isn't as critical IMO.

This being said, IIRC (I'm still away from my coding env), the NodeType config entity does include a one-off mechanism to allow 3rd-party-added properties within the node.type.*.yml CMI records (presumably with some code to cleanup properties when the 3rd party module gets uninstalled ? didn't check, but that would be a bug if not). So if it was done for node types, maybe it could be considered for fields and instances as well...

andypost’s picture

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

jessebeach’s picture

FileSize
20.71 KB
23.96 KB
35.35 KB
97.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,492 pass(es). View
1.03 KB

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.

content_translation settings file showing that both new fields created with and without the patch are marked as being translatable.

Configuration file for a new field on 8.x HEAD showing that the translation_sync setting is present.

Configuration file for a new field showing that the translation_sync setting is not present.

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.

jessebeach’s picture

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

andypost’s picture

#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

plach’s picture

Assigned: plach » Unassigned

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.
andypost’s picture

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

plach’s picture

Exactly :)

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

swentel’s picture

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

xjm’s picture

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.

jessebeach’s picture

FileSize
1.35 KB
98.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,650 pass(es). View
+++ 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.

yched’s picture

c_t adding extra stuff to field type plugin definitions to provide metadata about how a specific field type can leverage "translation sync" sounds perfectly reasonable to me, that's what hook_field_info_alter() is here for ?

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Ok, let's do this then ?

andypost’s picture

FileSize
98.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,764 pass(es). View

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

yched’s picture

+++ b/core/modules/content_translation/content_translation.module
@@ -790,21 +790,17 @@ function content_translation_form_field_ui_field_instance_edit_form_alter(array
 function content_translation_entity_presave(EntityInterface $entity) {
+  // By default no column has to be synchronized.
+  if ($entity->getEntityTypeId() === 'field_config') {
+    $entity->settings += array('translation_sync' => FALSE);
+  }
+  // Synchronization can be enabled per instance.
+  if ($entity->getEntityTypeId() === 'field_instance_config') {
+    $entity->settings += array('translation_sync' => FALSE);
+  }

Since this really is an ugly WTF, let's please add a @todo comment pointing at #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration :-)

andypost’s picture

FileSize
977 bytes
98.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,675 pass(es). View

@todo added

yched’s picture

Status: Reviewed & tested by the community » Needs work

Actually reviewed the patch - mostly minor remarks, other than that this looks good to go.
Thanks folks !

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/DecimalFormatter.php
    @@ -34,6 +28,18 @@ class DecimalFormatter extends NumericFormatterBase {
    +  public static function defaultSettings() {
    +    $settings = parent::defaultSettings();
    +    $settings['thousand_separator'] = '';
    +    $settings['decimal_separator'] = '.';
    +    $settings['scale'] = 2;
    +    $settings['prefix_suffix'] = TRUE;
    +    return $settings;
    +  }
    

    I would tend to have core encourage a more readable array syntax like:

    return array(
      'thousand_separator' => '',
      'prefix_suffix' => TRUE,
    ) + parent::defaultSettings();
    

    , but maybe it's only me :-)

  2. +++ b/core/lib/Drupal/Core/Field/PluginSettingsBase.php
    @@ -42,11 +49,11 @@ public function getSettings() {
    -    if (!$this->defaultSettingsMerged && !array_key_exists($key, $this->settings)) {
    +    // Merge defaults if that has not been done before.
    +    if (!$this->defaultSettingsMerged) {
    

    Why this change ? Looks unrelated and with potentially nasty side effects ?

  3. +++ b/core/lib/Drupal/Core/Field/PluginSettingsBase.php
    @@ -56,20 +63,13 @@ public function getSetting($key) {
    -    $this->settings += $this->getDefaultSettings();
    +    // Use only settings defined by plugin.
    +    $this->settings = array_intersect_key($this->settings, static::defaultSettings()) + static::defaultSettings();
    

    Not sure why this change either, but at any rate we should avoid calling/computing static::defaultSettings() twice in a single line :-)

  4. +++ b/core/lib/Drupal/Core/Field/PluginSettingsInterface.php
    @@ -34,21 +42,13 @@ public function getSettings();
    -   * @return \Drupal\field\Plugin\PluginSettingsInterface
    +   * @return \Drupal\Core\Field\PluginSettingsInterface
        *   The plugin itself.
    
    @@ -61,7 +61,7 @@ public function setSettings(array $settings);
    -   * @return \Drupal\field\Plugin\PluginSettingsInterface
    +   * @return \Drupal\Core\Field\PluginSettingsInterface
        *   The plugin itself.
    

    Nice catch, but since then we have moved to just "@return $this" ?

  5. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/ConfigurableEntityReferenceItem.php
    @@ -31,6 +31,25 @@ class ConfigurableEntityReferenceItem extends EntityReferenceItem implements All
    +  public static function defaultSettings() {
    +    $settings = parent::defaultSettings();
    +    // The instance settings 'handler_settings.target_bundles' used for this.
    +    unset($settings['target_bundle']);
    

    Grammar looks off in the comment.
    "The target bundle is handled by the 'target_bundles' property in the 'handler_settings' instance setting." ?

  6. +++ b/core/modules/field/lib/Drupal/field/Entity/FieldConfig.php
    @@ -326,7 +326,9 @@ protected function preSaveNew(EntityStorageControllerInterface $storage_controll
    -    $this->settings += $field_type['settings'];
    +     if (isset($field_type['class'])) {
    +       $this->settings += $field_type['class']::defaultSettings();
    +     }
    

    Actually we should just call FieldTypePluginManager::getDefaultSettings() here ?
    (that's what FieldInstanceConfig::preSave() does already when merging default instance settings)

  7. +++ b/core/modules/field/lib/Drupal/field/Entity/FieldConfig.php
    @@ -526,8 +528,13 @@ public function getSettings() {
    +    $settings = array();
    +    $instance_settings = array();
    +    if (isset($field_type_info['class'])) {
    +      $settings = $field_type_info['class']::defaultSettings();
    +      $instance_settings = $field_type_info['class']::defaultInstanceSettings();
    +    }
    

    Same here, call FTPM::getDefaultSettings() / getDefaultInstanceSettings() ?

  8. +++ b/core/modules/field/lib/Drupal/field/Entity/FieldConfig.php
    @@ -535,18 +542,14 @@ public function getSettings() {
         // We assume here that consecutive array_key_exists() is more efficient than
         // calling getSettings() when all we need is a single setting.
         if (array_key_exists($setting_name, $this->settings)) {
           return $this->settings[$setting_name];
         }
    -    elseif (array_key_exists($setting_name, $field_type_info['settings'])) {
    -      return $field_type_info['settings'][$setting_name];
    -    }
    -    elseif (array_key_exists($setting_name, $field_type_info['instance_settings'])) {
    -      return $field_type_info['instance_settings'][$setting_name];
    +    $settings = $this->getSettings();
    +    if (array_key_exists($setting_name, $settings)) {
    +      return $settings[$setting_name];
    

    Comment is now off, there are no "consecutive calls to array_key_exists" anymore.
    --> change to "We assume that one call to array_key_exists() is more efficient than ...." ?

  9. +++ b/core/modules/field/lib/Drupal/field/Tests/CrudTest.php
    @@ -64,7 +64,7 @@ function testCreateField() {
         $field_type = \Drupal::service('plugin.manager.field.field_type')->getDefinition($field_definition['type']);
    -    $this->assertEqual($field_config['settings'], $field_type['settings'], 'Default field settings have been written.');
    +    $this->assertEqual($field_config['settings'], $field_type['class']::defaultSettings(), 'Default field settings have been written.');
    

    No need to fetch $field_type, just use 'plugin.manager.field.field_type'::getDefaultSettings() ?

  10. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.php
    @@ -134,7 +134,7 @@ function testFieldPrepare() {
         $field_type = \Drupal::service('plugin.manager.field.field_type')->getDefinition($field_definition['type']);
    -    $this->assertEqual($field->settings, $field_type['settings'], 'All expected default field settings are present.');
    +    $this->assertEqual($field->settings, $field_type['class']::defaultSettings(), 'All expected default field settings are present.');
    

    same

  11. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.php
    @@ -168,7 +168,7 @@ function testInstancePrepare() {
         $field_type = \Drupal::service('plugin.manager.field.field_type')->getDefinition($field_definition['type']);
    -    $this->assertEqual($instance->settings, $field_type['instance_settings'] , 'All expected instance settings are present.');
    +    $this->assertEqual($instance->settings, $field_type['class']::defaultInstanceSettings() , 'All expected instance settings are present.');
    

    same

  12. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldInstanceCrudTest.php
    @@ -84,7 +84,7 @@ function testCreateFieldInstance() {
    -    $this->assertEqual($config['settings'], $field_type['instance_settings'] , 'Default instance settings have been written.');
    +    $this->assertEqual($config['settings'], $field_type['class']::defaultInstanceSettings() , 'Default instance settings have been written.');
    

    same

  13. +++ b/core/modules/field_ui/lib/Drupal/field_ui/DisplayOverviewBase.php
    @@ -399,8 +399,7 @@ protected function buildFieldRow(FieldDefinitionInterface $field_definition, Ent
    -        $plugin_definition = $plugin->getPluginDefinition();
    -        if ($plugin_definition['settings']) {
    +        if ($plugin::defaultSettings()) {
    

    Again, IMO ideally only the plugin manager would call the static methods on the plugin class, the rest of the code would call the methods on the plugin manager, which are the "official API" to get the default settings of a plugin.

    That's the standard pattern we have for other static definition methods (field properties, field schema...). They are collected by one method somewhere (typically on a manager), and the rest of the code goes through that method.

  14. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageDisplayTest.php
    @@ -225,10 +225,11 @@ function testViewModeCustom() {
    +    $field_test_default_settings = \Drupal::service('plugin.manager.field.formatter')->getDefaultSettings('field_test_default');
    +    $field_test_with_prepare_view_settings = \Drupal::service('plugin.manager.field.formatter')->getDefaultSettings('field_test_with_prepare_view');
    

    Nitpick, but extracting the formatter plugin manager to a separate var moight be more readable.

  15. +++ b/core/modules/file/file.field.inc
    @@ -9,16 +9,6 @@
    - * Implements hook_field_info_alter().
    - *
    - * Cannot annotate in FieldItem plugin the settings.uri_scheme meta data key
    - * with a dynamic value. We need to alter the value here.
    - */
    -function file_field_info_alter(&$info) {
    -  $info['file']['settings']['uri_scheme'] = file_default_scheme();
    -}
    

    Nice :-)

yched’s picture

Also, re @andypost #111:

(yched #110) - the NodeType config entity does include a one-off mechanism to allow 3rd-party-added properties
(andypost #111) - 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())

No, I did mean node.settings, not User data :-)
Notice that node.settings is keyed by module name, thus allowing 3rd party modules to add stuff in there and still provide a config schema.

swentel’s picture

Status: Needs work » Needs review
FileSize
100.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,757 pass(es), 0 fail(s), and 8 exception(s). View
11.39 KB

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

Status: Needs review » Needs work

The last submitted patch, 116: 2136197-116.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
2.35 KB
100.69 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
jessebeach’s picture

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

swentel’s picture

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

jessebeach’s picture

FileSize
46.08 KB
100.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

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.

jessebeach’s picture

FileSize
100.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
555 bytes

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

swentel’s picture

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

The last submitted patch, 121: remove-settings-annotation-2136197-120.patch, failed testing.

jessebeach’s picture

FileSize
866 bytes
100.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

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

The last submitted patch, 122: remove-settings-annotation-2136197-122.patch, failed testing.

The last submitted patch, 118: 2136197-settings-annotations-remove-andypost-118.patch, failed testing.

yched’s picture

Status: Needs review » Reviewed & tested by the community

w00t ! Thanks !
RTBC if green :-)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 125: remove-settings-annotation-2136197-123.patch, failed testing.

swentel’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
99.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,728 pass(es). View
1.02 KB

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • Commit 7f61620 on 8.x by alexpott:
    Issue #2136197 by andypost, jessebeach, swentel, longwave, Berdir: Move...
andypost’s picture

Status: Fixed » Needs review
FileSize
794 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,726 pass(es). View

One more place found

swentel’s picture

Title: Move field/instance/widget/formatter settings out of annotation / plugin definition » Move field/instance/widget/formatter settings out of annotation / plugin definition [followup]
Status: Needs review » Reviewed & tested by the community

Ah right, good catch.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • Commit 4c109a5 on 8.x by alexpott:
    Issue #2136197 followup by andypost: Move field/instance/widget/...
andypost’s picture

Title: Move field/instance/widget/formatter settings out of annotation / plugin definition [followup] » Move field/instance/widget/formatter settings out of annotation / plugin definition
yched’s picture

Adjusted the change notice (https://drupal.org/node/2221691) a bit,
and updated the existing change notices about "(field types | formatters | widgets) as plugins" :
https://drupal.org/node/2064123
https://drupal.org/node/1805846
https://drupal.org/node/1796000

Status: Fixed » Closed (fixed)

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