This was originally found in #2209981: ConfigurableEntityReferenceItem should not mess with the field schema and is similar in nature to #2235125: Use DataDefinition::addConstraint() instead of ::setConstraints().
It also causes #2472443: BFD::createFromFieldStorageDefinition() creates a broken field definition (no field-level settings)

Problem/Motivation

DataDefinition::setSettings() is used all over core to set multiple settings at once. However, it currently removes default settings if those are not explicitly specified.

Proposed resolution

As per #36 :

TypedData doesn't have a notion of "default settings", it can be argued that TypedData\DataDefinition::setSettings($settings) overwriting existing settings is fair - it's a setter after all.

But fields do have this notion of default (field-level / storage-level) settings, and a FieldDefinition that doesn't have an entry for an "official" setting is invalid / broken. There is no unsetting a field setting, the moment a setting is defined, consuming code assumes it is set in any runtime field definition.

So the patch leaves the generic TypedData's DataDefinition::setSettings() untouched, and instead modifies the method in field / field storage definitions (BaseFieldDefinition / FieldConfigBase / FieldStorageConfig) so that they merge rather than replace.

The current patch also replaces a bunch of usages of setSettings() with repeated calls to setSetting() as (arguably) this is more readable and was necessary for previous iterations of this patch. This can be removed in case people feel strongly.

API changes

The setSettings($settings) method in BaseFieldDefinition / FieldConfigBase / FieldStorageConfig no longer overwrites the current settings with the new ones, but rather merges the new ones into the old ones, to ensure no setting goes missing.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug : a valid API call can produce invalid data structures (field definitions that lack some settings and produce notices when consuming code tries to access them)
Issue priority Major because ... (not sure ?)
Disruption None ? I don't think there can be code that relies on the current "overwrite, leaving missing settings" behavior, since it can only produce invalid definition structures
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

Status: Active » Needs review

Here we go.

I went with approach 1 for now. Let's see.

tstoeckler’s picture

Oops.

tstoeckler’s picture

Status: Needs review » Needs work

The last submitted patch, 2: 2236903-remove-set-settings.patch, failed testing.

Jalandhar’s picture

Status: Needs work » Needs review
FileSize
9.18 KB

Rerolled patch.

Status: Needs review » Needs work

The last submitted patch, 5: 2236903-remove-set-settings-5.patch, failed testing.

yched’s picture

setSettings() overrides all settings and, thus, removes any default settings (if they are not passed into setSettings())

That seems like a bug. setSettings() should merge the incoming array into the defaults.

FWIW, I'm +1 on using setSettings() in Data / Field definitions...

tstoeckler’s picture

Re #7: I'm fine with that, but then there would be no (simple) way to do what setSettings() does right now. I still think we should standardize on setSetting() in baseFieldDefinitions(), though. I guess this is arguable, but IMO it improves legibility.

andypost’s picture

otoh we should left ability to override all settings at once

yched’s picture

@andypost #9

we should left ability to override all settings at once

We don't want to provide the ability to remove settings that the rest of the runtime code assume is present.
Thus, setSettings() should always merge with default values. $this->settings should always be an array with entries for all known settings.

@tstoeckler #8

but then there would be no (simple) way to do what setSettings() does right now

Not sure what you mean ?

tstoeckler’s picture

@yched I meant the same thing as @andypost in #9. But you make a compelling argument. I will update the patch per #10.

Or maybe @Jalandhar you want to tackle that, since you already re-rolled the patch?! :-) Thanks for that, by the way, that was very helpful!

sun’s picture

Title: Remove DataDefinition::setSettings() » DataDefinition::setSettings() removes other settings (unintended)
Category: Task » Bug report
Issue tags: +DX (Developer Experience), +API clean-up, +beta target

I can see how merging defaults + existing may not work correctly at all times, leaving you with a undefined "magic" behavior.

Due to that, I think this can be classified as a bug, and we should remove the method entirely.

The two referenced bug reports in the OP give sufficient proof that the method is misunderstood and its usage only introduces major bugs.

tstoeckler’s picture

Well I don't really care, TBH. We have two options:

1. Fix setSettings() to foreach over setSetting() itself.

2. Remove setSettings() and force people to foreach themselves.

The above patches do 2. @yched wants 1. so I'm fine with it.

When building up field definitions I certainly like setSetting() more than setSettings() in terms of readability, but if people feel strongly that we shouldn't do that, I could live with ripping that part out of the patch.

Thanks for the issue rename.

yched’s picture

Disagree with #12, I see no reason why setSettings() should be removed.
Merging with defaults is the way it works for widgets and formatters, I'd like to keep field types consistent please.

fago’s picture

Merging in defaults seems reasonable. #ad12: When would this not work?

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
3.29 KB
8.27 KB

Here we go. This leaves setSettings() in place.

sun’s picture

If it's about merging settings, then the method name should be mergeSettings() instead of setSettings().

Merging can always yield unexpected behavior depending on your expectations; e.g.:

->mergeSettings(array(
  'foo' => array(
    'stale' => 'child',
    'bar' => 'baz',
  ),
))
->mergeSettings(array(
  'foo' => array(
    'bar' => 'zulu',
  ),
))

Whether you intended to leave that stale child behind or not depends on your use-case (and whether you're aware of that stale child key to begin with). Contrary to that, setSetting() will explicitly replace 'foo' with a new value, sans merge.

yched’s picture

At any rate, we're not doing deep merging - that would be assuming stuff about the semantics of each setting, which we don't know about.

As stated earlier, the goal is to ensure all settings (i.e 1st level keys) are present for runtime code that expects they are there. You cannot "unset" a setting, there always has to be a value - be it the default value, or an explicit NULL.

tstoeckler’s picture

tstoeckler’s picture

OK, does anyone want to RTBC?

tstoeckler’s picture

Issue summary: View changes

Updated the issue summary to reflect the new approach.

I don't think we need a change notice as I can't imagine anyone being explicitly aware of the current behavior.

yched’s picture

+    foreach ($settings as $setting_name => $setting) {
+      $this->definition['settings'][$setting_name] = $setting;
+    }

$this->definition['settings'] = $settings + $this->definition['settings']; would have the same effect ?

Also, this patch fixes setSettings() to behave as it should, so no need for all the s/setSettings()/setSetting() ?
Encouraging multiple individual setSetting() calls over one single setSettings() in baseFieldDefinitions() implementations in core is one thing, but it's not related to actually fixing setSettings() :è-)

fago’s picture

Please see the related #2116341: Apply defaults to definition objects - with the approach used over there the problem would be gone also. The approach over there is to have a $apply_defaults boolean on the getter, defaulting to TRUE.

Having getSettings() returning the defaults by default is good behaviour, however this change makes setSettings() + getSettings() not idempotent, i.e. you don't get what you set. That's awkward and not documented behaviour, thus I think we should either

  • clarify that defaults are mixed in,
  • call it addSettings() or mergeSettings(),
  • or have the $apply_defaults boolean on the getter
tstoeckler’s picture

clarify that defaults are mixed in

You mean with documentation on getSettings()? Sure, that sounds like a good idea.

Status: Needs review » Needs work

The last submitted patch, 16: 2236903-16-set-settings-dangerous.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
Issue tags: +DrupalWTF
FileSize
5.92 KB
6.41 KB

Although the usage of setSettings() has decreased over time this is still a pretty bad bug.

@fago Is this sufficient in terms of documentation? I really don't see any need to litter our API with a $apply_defaults boolean, when we really have no use-case for setting that to FALSE.

No real interdiff because there was a merge file. I just accepted all the changes from 8.0.x and the *.txt file shows what I did from there.

jhedstrom’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
tstoeckler’s picture

Status: Needs work » Needs review
Issue tags: -beta target, -Needs reroll
FileSize
6.45 KB

Must have been a context change because I had no merge conflict.

andypost’s picture

Status: Needs review » Reviewed & tested by the community
amateescu’s picture

+++ b/core/modules/system/src/Tests/Entity/EntityFieldTest.php
@@ -686,10 +686,8 @@ public function testEntityConstraintValidation() {
-      ->setSettings(array(
-        'target_type' => 'node',
-        'target_bundle' => 'article',
-      ));
+      ->setSetting('target_type', 'node')
+      ->setSetting('target_bundle', 'article');

Looks like this hunk is no longer needed in the patch.

fago’s picture

Status: Reviewed & tested by the community » Needs work

I don't think it's a good idea to make it keep everything that's not passed to setSettings() - this is really not the usually set semantic. We can apply defaults to the passed settings, but everything else not passed should be left out imo. If that's required, we could add an addSettings() method - or you just call setSetting() for each setting.

+++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
@@ -224,14 +224,36 @@ public function getSettings() {
+   * Note that this does not override or remove the default settings declared by
+   * the data type, even if not all possible settings are supplied. For example:

There is nothing like default settings in typed data. So this documentation needs to stay down.

yched’s picture

We can apply defaults to the passed settings, but everything else not passed should be left out imo

I don't get what "applying defaults to the passed settings" would be ? If they are passed, it means they are passed with a value, and there is no need to apply defaults ?

Also, see #18 :

As stated earlier, the goal is to ensure all settings (i.e 1st level keys) are present for runtime code that expects they are there. You cannot "unset" a setting, there always has to be a value - be it the default value, or an explicit NULL.

tstoeckler’s picture

Re #31: You're right, it's not needed. I still think we should promote setSetting() over setSettings() regardless. Especially because people will copy-paste a lot of these things from core I think it makes sense to be consistent here, and setSetting() is perfectly fit that. I can remove that hunk, though, if you want.

@fago That would mean the following:

// Let's say the default settings are:
FieldItem::defaultSettings() === [
  'fruit' => 'apple',
  'season' => 'winter',
];

// And the current settings are:
$field_definition->getSettings() === [
  'fruit' => 'banana',
  'season' => 'summer',
];

// And then I want to change the fruit, and decide to use setSettings()
// because I saw that method in core.
$field_definition->setSettings(['fruit' => 'cherry']);

// Then I've inadvertantly also changed the 'season' setting without knowing it.
$field_definition->getSetting('season') === 'winter';

I really can't imagine that to be what we want and I don't see how that can be considered idempotent.

yched’s picture

Bumped on this on BFD::createFromFieldStorageDefinition() :

BaseFieldDefinition::createFromFieldStorageDefinition() creates a BFD that misses default values of field-level settings. This causes notices in code that tries to access them down the line. See #2472443: BFD::createFromFieldStorageDefinition() creates a broken field definition (no field-level settings), which I opened before @amateescu reminded me of that issue here.

yched’s picture

Discussed with @fago and @Berdir at DDD :

We concluded that, since TypedData doesn't have a notion of "default settings", it can be argued that TypedData\DataDefinition::setSettings($settings) overwriting existing settings is fair - it's a setter after all.

But fields do have this notion of default (field-level / storage-level) settings, and a FieldDefinition that doesn't have an entry for an "official" setting is invalid / broken. There is no unsetting a field setting.

So, the merge behavior should rather be done in BFD::setSettings() / FieldStorageConfig::setSettings() (#2472443-2: BFD::createFromFieldStorageDefinition() creates a broken field definition (no field-level settings) has a patch that does that)

@tstoeckler : what do you think ?

(Side note : if we go that way, it would need to be done in FieldConfig::setSettings() too, unfortunately that one does not exist yet, there's only a $field_config->setting public property, because #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods didn't land yet - which, er, is my bad :-/
We can simply modify the patch over there so that the setSettings() method it introduces does have that merge behavior)

yched’s picture

yched’s picture

#2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods landed now, so we can deal with this behavior entirely in BFD::setSettings() / FieldStorageConfig::setSettings() without changing the behavior of TypedData\DataDefinition::setSettings()

@tstoeckler / @fago : does that sound good to you ?

yched’s picture

Status: Needs work » Needs review
FileSize
9.67 KB
8.67 KB

#36 Would be something like that.

Fixing that in several child classes instead of DataDefinition(Interface) is indeed a bit more cumbersome in terms of where to put the corresponding doc :-/

However, we need to fix FieldStorageConfig/FieldStorageDefinitionInterface as well, and that one is not a DataDefinition anyway.

@tstoeckler / @fago : what do you think ?

andypost’s picture

In related issue we need to update field settings but it's not clear which method to use for that #2467293: Warning when changing vocabulary machine name

Jalandhar’s picture

Status: Needs review » Needs work

Patch #39 needs to be rerolled.

andypost’s picture

Issue tags: +Need reroll
+++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
@@ -146,16 +146,36 @@ public function getSettings() {
+   * ¶

and nit

nlisgo’s picture

Status: Needs work » Needs review
Issue tags: -Need reroll
FileSize
9.63 KB

Re-roll and addressed whitespace error mentioned in #42.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Great!

fago’s picture

Title: DataDefinition::setSettings() removes other settings (unintended) » setSettings() on field definitions can unintentionally clear default values
Component: typed data system » field system

@tstoeckler / @fago : does that sound good to you ?

yep, it does, thanks. Trying to appropriately re-title the issue.

Also took a look at the patch, looks great. I second the RTBC.

yched’s picture

Issue summary: View changes

Thanks @fago !

Updated the IS for the current approach (limited to field definitions rather than the generic DataDefinition), and added a beta evaluation.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

We don't seem to have test coverage of this behaviour.

To me it's a bit odd to have a method called setSettings that actually merges settings. But that has been discussed here already.

yched’s picture

@alexpott :

To me it's a bit odd to have a method called setSettings that actually merges settings. But that has been discussed here already.

Right, see #36 (summarized in the IS). Field settings are not an arbitrary key/value collection, but a well-defined set of properties that all need to have a value. You cannot say "I'll just set 'foo' and 'bar' and leave the others not set", that's not something that's allowed nor has any sense.

Agreed on the test.

swentel’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.64 KB
10.99 KB

Simple test. Test only patch is also the interdiff with the patch.

The last submitted patch, 49: 2236903-49-test-only.patch, failed testing.

The last submitted patch, 49: 2236903-49-test-only.patch, failed testing.

yched’s picture

Thanks @swentel ! I extended the test a bit, to cover the three classes involved (BaseFieldDefinition, FieldStorageConfig, FieldConfig), and more explicitly track what we're testing (there are multiple settings, we set one, the others are still there - this also tests the "default settings" behavior)

This also uncovered #2327883: Field [storage] config have incomplete settings until they are saved again : FieldConfig doesn't populate default settings until the config is saved (FieldStorageConfig does). We have some unification / cleanup to do there, left a @todo note.

Also, since \Drupal\system\Tests\Entity\EntityFieldTest is already 700 lines long, and those tests here only involves definition structures and not entities, moved them to a separate test class in \Drupal\system\Tests\Field.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

That's some mighty test coverage, RTBC if green :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 52: 2236903-fix_Field_setSettings-52.patch, failed testing.

The last submitted patch, 52: 2236903-fix_Field_setSettings-52-test_only.patch, failed testing.

The last submitted patch, 52: 2236903-fix_Field_setSettings-52.patch, failed testing.

yched’s picture

"CI error" - dunno what that means ?
There's no "retest" link, does it get retested automatically ?

yched’s picture

Status: Needs work » Needs review
yched’s picture

Reuploading just in case

Status: Needs review » Needs work

The last submitted patch, 59: 2236903-fix_Field_setSettings-52.patch, failed testing.

The last submitted patch, 52: 2236903-fix_Field_setSettings-52-test_only.patch, failed testing.

yched’s picture

Status: Needs work » Needs review

Oh sigh

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Yes!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Discussed with @yched, @amateescu and @berdir - we all agreed that this makes working with fields more robust and is a good change and there was never a use0case to unset on the top-level of the settings array.

This fixes a major bug and is BC. Committed a49d543 and pushed to 8.0.x. Thanks!

  • alexpott committed a49d543 on 8.0.x
    Issue #2236903 by yched, tstoeckler, swentel, Jalandhar, nlisgo,...

Status: Fixed » Closed (fixed)

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

Berdir’s picture

Something about FieldSettingsTest is strange, see #2679096: Convert Kernel Tests in Drupal\system\Tests\Entity to phpunit. We can't see a reason why this would assert TRUE but the code adds 'TRUE'. It seems like it just works because simpletest is less strict and it fails with PHPUnit. So I'm changing the expectation to TRUE as well. (As described there, there is no runtime type checking, only when saving and that is also overridden because loading changes it again).