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
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 |
Comment | File | Size | Author |
---|---|---|---|
#59 | 2236903-fix_Field_setSettings-52.patch | 14.79 KB | yched |
#52 | interdiff.txt | 6.25 KB | yched |
#52 | 2236903-fix_Field_setSettings-52.patch | 14.79 KB | yched |
| |||
#52 | 2236903-fix_Field_setSettings-52-test_only.patch | 4.89 KB | yched |
#49 | 2236903-49.patch | 10.99 KB | swentel |
Comments
Comment #1
tstoecklerHere we go.
I went with approach 1 for now. Let's see.
Comment #2
tstoecklerOops.
Comment #3
tstoeckler2: 2236903-remove-set-settings.patch queued for re-testing.
Comment #5
Jalandhar CreditAttribution: Jalandhar commentedRerolled patch.
Comment #7
yched CreditAttribution: yched commentedThat seems like a bug. setSettings() should merge the incoming array into the defaults.
FWIW, I'm +1 on using setSettings() in Data / Field definitions...
Comment #8
tstoecklerRe #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.
Comment #9
andypostotoh we should left ability to override all settings at once
Comment #10
yched CreditAttribution: yched commented@andypost #9
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
Not sure what you mean ?
Comment #11
tstoeckler@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!
Comment #12
sunI 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.
Comment #13
tstoecklerWell 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.
Comment #14
yched CreditAttribution: yched commentedDisagree 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.
Comment #15
fagoMerging in defaults seems reasonable. #ad12: When would this not work?
Comment #16
tstoecklerHere we go. This leaves
setSettings()
in place.Comment #17
sunIf it's about merging settings, then the method name should be
mergeSettings()
instead ofsetSettings()
.Merging can always yield unexpected behavior depending on your expectations; e.g.:
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.Comment #18
yched CreditAttribution: yched commentedAt 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.
Comment #19
tstoeckler16: 2236903-16-set-settings-dangerous.patch queued for re-testing.
Comment #20
tstoecklerOK, does anyone want to RTBC?
Comment #21
tstoecklerUpdated 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.
Comment #22
yched CreditAttribution: yched commented$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() :è-)
Comment #23
fagoPlease 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
Comment #24
tstoecklerYou mean with documentation on getSettings()? Sure, that sounds like a good idea.
Comment #27
tstoecklerAlthough 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 toFALSE
.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.
Comment #28
jhedstromComment #29
tstoecklerMust have been a context change because I had no merge conflict.
Comment #30
andypostComment #31
amateescu CreditAttribution: amateescu commentedLooks like this hunk is no longer needed in the patch.
Comment #32
fagoI 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.
There is nothing like default settings in typed data. So this documentation needs to stay down.
Comment #33
yched CreditAttribution: yched commentedI 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 :
Comment #34
tstoecklerRe #31: You're right, it's not needed. I still think we should promote
setSetting()
oversetSettings()
regardless. Especially because people will copy-paste a lot of these things from core I think it makes sense to be consistent here, andsetSetting()
is perfectly fit that. I can remove that hunk, though, if you want.@fago That would mean the following:
I really can't imagine that to be what we want and I don't see how that can be considered idempotent.
Comment #35
yched CreditAttribution: yched commentedBumped 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.
Comment #36
yched CreditAttribution: yched commentedDiscussed 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)
Comment #37
yched CreditAttribution: yched commentedAlso, bumping to "major" because of #2472443: BFD::createFromFieldStorageDefinition() creates a broken field definition (no field-level settings)
Comment #38
yched CreditAttribution: yched commented#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 ?
Comment #39
yched CreditAttribution: yched commented#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 ?
Comment #40
andypostIn 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
Comment #41
Jalandhar CreditAttribution: Jalandhar commentedPatch #39 needs to be rerolled.
Comment #42
andypostand nit
Comment #43
nlisgo CreditAttribution: nlisgo commentedRe-roll and addressed whitespace error mentioned in #42.
Comment #44
andypostGreat!
Comment #45
fagoyep, it does, thanks. Trying to appropriately re-title the issue.
Also took a look at the patch, looks great. I second the RTBC.
Comment #46
yched CreditAttribution: yched commentedThanks @fago !
Updated the IS for the current approach (limited to field definitions rather than the generic DataDefinition), and added a beta evaluation.
Comment #47
alexpottWe 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.Comment #48
yched CreditAttribution: yched commented@alexpott :
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.
Comment #49
swentel CreditAttribution: swentel commentedSimple test. Test only patch is also the interdiff with the patch.
Comment #52
yched CreditAttribution: yched commentedThanks @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.
Comment #53
swentel CreditAttribution: swentel commentedThat's some mighty test coverage, RTBC if green :)
Comment #57
yched CreditAttribution: yched commented"CI error" - dunno what that means ?
There's no "retest" link, does it get retested automatically ?
Comment #58
yched CreditAttribution: yched commentedComment #59
yched CreditAttribution: yched commentedReuploading just in case
Comment #62
yched CreditAttribution: yched commentedOh sigh
Comment #63
swentel CreditAttribution: swentel commentedYes!
Comment #64
alexpottDiscussed 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!
Comment #67
BerdirSomething 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).