Problem
While working on #2226197: Introduce FieldStorageDefinitionInterface in the Entity Field API we figured the current default value methods/API is a bit confusing, as depending whom you ask you might get different answers what the default value of a field is. That is as the default value can be specified in the field definition object, but field type specific defaults and/or field-type specific interpreting of the default value lives in the field item list class. E.g a problem right now is that $field_definition->getDefaultValue() of a date field could give you 'NOW', but that's not a valid value for a date field.
From a logical point of view it would make most sense to me to be able to ask the field definition about the default value (while you have to pass the entity for which you want to get it). The field type may specify some type-specific logic that generates a default value per field type, but for concrete field instances you might want to override it. So the field definition is the one, which should know and be our public API.
Also, it makes sense to me to be able to ask the definition object about it as getting the default value of an item should not necessarily require having already an item.
I do not know whether it makes a lot of sense to pass the entity object to $field_definiton->getDefaultValue() though - why would one generat a default value of a field dependent on another field? (Not sure about changing that, just wondering).
Proposed solution
As the field item class is the class field type specific adapts go, I'd propose moving the methods for defining custom default value providing logic per field type and the form (which would have to become static). This makes a better DX for people providing field types as well and is inline with what we are doing for e.g. property definitions (field definition asks the field item class about the field type specific defaults).
So the current flow is:
$item->getDefaultValue() -> call parent -> get $field_definition->getDefaultValue() which incorporates the default_value_function and/or the default_value setting -> adapt value -> return
The new one would be:
$field_definition->getDefaultValue() -> if no default value function is there call item::defaultValue($field_definition), which reads the default value setting (and optionally processes it as date field does) -> return
Then, to customize default values you can
- set the default value setting (if the field type implements it, what is best practice and done in the base class)
- have a field type specific default implemented in the field item class
- provide a default value function as callback to a specific field definition, which then takes over
- customize the field definition class (for base field usages) to a provide a custom function across multiple usages as #1979260: Automatically populate the author default value with the current user proposes. (There would be no good place to put the callback else, so the class seems reasonable.)
API changes
- FieldDefinitionInterface::getDefaultValue() and FieldDefinitionInterface::getDefaultValueCallback() added
- FieldDefinition::setSetting('default_value', 'foo') is replaced by FieldDefinition::setDefaultValue('foo'). We could easily add BC support for that if want to.
Comment | File | Size | Author |
---|---|---|---|
#40 | field-default-value-2226267-followup-40.patch | 1.36 KB | yched |
#38 | field-default-value-2226267-followup-36.patch | 684 bytes | yched |
#32 | interdiff-24-32.txt | 1.06 KB | xjm |
#32 | field-default-value-2226267-32.patch | 33.55 KB | xjm |
#24 | d8_field_default_value-24.patch | 33.37 KB | Berdir |
Comments
Comment #1
fagoSo here is a preliminary, not working patch moving some stuff around to explain/discuss things.
Comment #2
fagoForgot a double-colon ;)
Comment #3
fagoComment #4
yched CreditAttribution: yched commentedStill need to read and digest the proposal, but:
- Current patch in #1979260: Automatically populate the author default value with the current user uses a dedicated FieldDefinition class to be able to override FieldDefinition::getDefaultValue()
- Latest patch in #1966436: Default *content* entity languages are not set for entities created with the API switches in a different FieldItem class for 'language' in language_field_info_alter() in order to be able to override FieldItem::applyDefaultValue().
So yeah, looks like we really need a generic solution to allow more flexibility on defult values...
Comment #5
yched CreditAttribution: yched commentedOverall, agreed with the plan in the OP, except for the very last part :
I'd still tend to discourage that - using a custom FieldDefinition subclass just to be able to put a static method on it ? A static method can live on any random helper class. If I have to add a new class anyway, I'd rather encourage an agnostic helper class rather than a CustomFieldDefinition class that just puzzles people when they see $field['foo'] = new CustomFieldDefinition(); in baseFieldDefinition().
("- Oh, what does it do ? How is it different from the FieldDefinition everyone else uses ? - Never mind, it's just FieldDefinition with an extra static method")
Comment #6
yched CreditAttribution: yched commentedThose were placed in FieldItemList because they do operate at the level of the list: they handle the default value *of the list*, i.e. a list of multiple values, keyed by delta, used as default values.
Putting this logic at the level of the Item does make it a bit easier to override for a field type (which is a not a frequent use case), but it's still conceptually wrong IMO ?
Hm, so does static defaultValue() live on the FieldItemList or on the FieldItem ?
Unrelated ?
Comment #7
fago1. true, it's conceptually the right place. So yeah, it should probably stay there then.
2.: I thought on The FieldItem (so that was wrong there), however I think it should go wherever 1) goes. The default values is for the whole field item list as well so if the form is on the list class the static should be also imho.
3. yes, sry.
I don't feel strong about going with CustomFieldDefinition classes, so if this bothers you let's just leave it out for now. But where should #1979260: Automatically populate the author default value with the current user put the callback implementation then? The owner/author field is not related to a certain entity type, so entity classes don't work? Have a custom class just for that, but only use it for the callback?
Comment #8
yched CreditAttribution: yched commentedA "default value" callback that returns the current user --> maybe be a static method on the User entity type class ? Or on the class that supports the current_user service ? (sry, can't look at code just right now)
Comment #9
fagoThe class is
AccountProxy
not sure it fits there. Also, the user entity seems to be arbitrary. :/Comment #10
yched CreditAttribution: yched commented"the user entity seems to be arbitrary"
Well, it returns the current user, so I wouldn't exactly call it arbitrary :-)
Well, if no existing class looks like a good fit, then it's a choice between :
1) add a new helper class in user.module just to put that helper method
2) add a new CustomFieldDefinition class just to put that helper method.
Not much difference :-) But as stated above, IMO:
2) opens the door to multiplying ad-hoc CustomFieldDefinition classes whose behavior is unclear.
1) means the definition I see in baseFieldDefinition() remains "familiar", with a clear mention that it uses a custom "default value" callback in a helper class somewhere. The "unknown" part is not "what does this whole CustomFieldDefinition do ?" but is limited to "what does this default value callback do ?", and the method name will likely give enough hints about that.
Variant : switch our approach from "specify a random default value callback / method" to "specify a DefaultValueProvider class, that needs to implement DefaultValueProviderInterface" :
- avoids the "one-off random class" effect - the callback becomes a real "thing", an implementation of a known interface
- DefaultValueProviderInterface lets us properly document the expected shape of the callback (arguments, return value...). We currently have no good place to document that.
The more I think of it, the more I find this is a cleaner approach, actually. Isolated callbacks are a thing of the past, they should be interfaces...
Comment #11
bforchhammer CreditAttribution: bforchhammer commentedTo add to the discussion: in #2111443: Show a warning when configuring form displays when a field is hidden and has no default value. we found that we need a way to determine whether a field has a default value without having access to an entity; at the moment that can only be done by creating a new entity, running
FieldDefinitionInterface::getDefaultValue()
and checking the property value afterwards (which seems like a lot of overhead); alternatively we can check manually whether the field definition has a non-empty 'default_value' setting or a callable 'default_value_function'. Both approaches don't seem like the best way to do things.Switching to a DefaultValueProviderInterface/DefaultValueProvider approach sounds like a good idea to me; any progress on that front?
Comment #12
yched CreditAttribution: yched commentedAs @swentel said in #2111443: Show a warning when configuring form displays when a field is hidden and has no default value., at some point we had a check in field_ui to display a message when "hiding" a field that was both required and without a default value. Can't seem to find that code now, not sure why/when it was removed.
Whatever happens over there, this IMO reinforces the fact that we need a unified, dedicated API for "default values" on FieldDefinitionInterface, not the current "dedicated properties on config fields vs a 'setting' in base fields".
Comment #13
Elijah LynnComment #14
fagoAgreed. Attached patch implements that as well as the suggested flow improvements to make the field definition the primary API (see #2278485: [META] Promote field definitions methods to a primary API).
Also marking this as beta target as it changes how you define the default value for a field.
Comment #15
fagoComment #16
fagoComment #19
fagoFixed the tests and moved to a separate entity type such that it does not influence other test case anymore.
Comment #21
fagoI really suck at multi-tasking.
Comment #22
xjmThanks @fago. Assigning this one to @yched for review as well.
Comment #23
yched CreditAttribution: yched commentedYay. Patch looks good :-)
- Great to finally move the signature of "default value callbacks" in line with "EntityNG". I tried to do that back in #2050801: Unify handling of default values between base and configurable fields, but it was frowned upon because of code freeze :-)
Closing #2060907: Let "default_value_callback' functions work with FieldDefinitionInterface as a dupe.
- I still think moving "default value callbacks" to a dedicated interface would be a good archiecture move, but that's outside the scope of this issue here
Would RTBC, but needs a reroll.
Comment #24
BerdirRe-roll.
Comment #25
yched CreditAttribution: yched commentedThanks !
(wow, we get testbot results in less than 18 minutes now ? Mind blown)
Comment #26
BerdirYeah, @jthorson set up some monster-servers and runs the tests with --concurrency=32 o.0
Comment #27
alexpottWhy are these not on the FieldDefinitionInterface?
very minor nit alert: If you are going to reroll - we're missing an @return
Comment #28
bforchhammer CreditAttribution: bforchhammer commentedCould this patch also add a method for checking whether a non-empty default value has been defined? (e.g.
hasDefaultValue()
). That's going to be needed for #2111443: Show a warning when configuring form displays when a field is hidden and has no default value.; see comments #11, #12.Comment #29
xjmI guess because the original setters were also not on the interface? There are numerous other setters that are on
FieldDefinition
but not on the interface. The whole class was added in #1994140: Unify entity field access and Field API access:Since the scope of "setters not on the FieldDefinitionInterface" is bigger than just these two, maybe makes sense to discuss in a followup?
I'll reroll this for the other two suggestions.
Comment #30
BerdirYes, the interface currently explicitly does not add setter methods, because we wanted to be flexible how they are configured as static base fields and configurable fields implement the same interface.
We also have #2143297: setters on FieldDefinitionInterface to change/discuss this, so let's stay consistent here.
Comment #31
xjmThanks @Berdir. :)
Comment #32
xjm@bforchhammer re:
Looking at the patch, this also seems out of scope. I'd suggest filing a separate issue. Should be a nice tiny patch.
Here's just the docs fix (which BTW is also an issue in HEAD anyway); leaving at RTBC since the other feedback has been addressed. This issue is soft-blocking a beta blocker (#2238085: [regression] options_allowed_values() signature doesn't allow for Views filter configuration) so would be good to keep it in scope and get it in.
Comment #33
alexpottCommitted 4831e2e and pushed to 8.x. Thanks!
Comment #35
tstoecklerJust saw this issue in the commitlog. Awesome work, this makes a ton of sense.
One thing is strange, however: Because we check for
isset($this->definition['default_value_callback'])
instead ofempty()
it's currently not possible to unset a default value callback using the API.Should there be a separate
unsetDefaultValueCallback()
?Comment #36
yched CreditAttribution: yched commented#35 makes sense. Followup patch attached.
@bforchhammer #28 : hasDefaultValue() would make sense as well. Didn't make it in here, so IMO #2111443: Show a warning when configuring form displays when a field is hidden and has no default value. can add it if it needs it ?
Comment #37
bforchhammer CreditAttribution: bforchhammer commented@yched #36: follow-up patch missing? :)
@xjm, @yched: Yes,
hasDefaultValue()
can be handled in #2111443: Show a warning when configuring form displays when a field is hidden and has no default value.; it's actually already part of the patch over there, just needs a re-roll I think.Comment #38
yched CreditAttribution: yched commentedEr, right.
Comment #39
tstoecklerThis looks great, but I think the PHPDoc for
setDefatulValueCallback()
should be adapted as well.The type-hint should be
callable|null
(it'sstring|array
right now), and there should be a line that passingNULL
removes a previously set default value callback.Comment #40
yched CreditAttribution: yched commentedUpdated the doc.
Comment #41
tstoecklerAwesome, thanks!
Comment #43
catchCommitted/pushed to 8.x, thanks!
Comment #45
fagofigured that this patch did not cover all existing default value implementations, see #2318605: uuid, created, changed, language default value implementations need to be updated