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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

Status: Active » Needs work
FileSize
6.54 KB

So here is a preliminary, not working patch moving some stuff around to explain/discuss things.

fago’s picture

Title: Improve deafult value handling of fields to be consistent » Improve default value handling of fields to be consistent

Forgot a double-colon ;)

fago’s picture

Issue summary: View changes
yched’s picture

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

yched’s picture

Overall, agreed with the plan in the OP, except for the very last part :

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

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

yched’s picture

  1. +++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
    @@ -240,4 +240,56 @@ public function instanceSettingsForm(array $form, array &$form_state) {
    +  public function defaultValuesForm($field_defintion, array &$form, array &$form_state) {
    ...
    +  public function defaultValuesFormValidate($field_defintion, array $element, array &$form, array &$form_state) {
    ...
    +  public function defaultValuesFormSubmit($field_defintion, array $element, array &$form, array &$form_state) {
    

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

  2. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -328,7 +328,10 @@ public function isDisplayConfigurable($display_context) {
    +    return $entity->get($this->getName())->getDefaultValue();
    
    +++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
    @@ -240,4 +240,56 @@ public function instanceSettingsForm(array $form, array &$form_state) {
    +  public static function defaultValue($field_definition, $entity) {
    +    return array();
    +  }
    
    +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Field/FieldType/ConfigurableEntityReferenceFieldItemList.php
    @@ -17,7 +17,7 @@ class ConfigurableEntityReferenceFieldItemList extends FieldItemList {
    -  protected function getDefaultValue() {
    +  public function defaultValue() {
    

    Hm, so does static defaultValue() live on the FieldItemList or on the FieldItem ?

  3. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayModeInterface.php
    @@ -20,6 +20,6 @@
    -  public function getTargetType();
    +  public function getTargetEntityTypeId();
    

    Unrelated ?

fago’s picture

1. 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?

yched’s picture

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?

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

fago’s picture

The class is AccountProxy not sure it fits there. Also, the user entity seems to be arbitrary. :/

yched’s picture

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

bforchhammer’s picture

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

yched’s picture

As @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".

Elijah Lynn’s picture

fago’s picture

Issue summary: View changes
FileSize
30.7 KB

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

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

fago’s picture

Status: Needs work » Needs review
fago’s picture

Issue tags: +beta target

The last submitted patch, 1: d8_default_value_move.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 14: d8_field_default_value.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
4.23 KB
32.97 KB

Fixed the tests and moved to a separate entity type such that it does not influence other test case anymore.

Status: Needs review » Needs work

The last submitted patch, 19: d8_field_default_value.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
33.24 KB
1.48 KB

I really suck at multi-tasking.

xjm’s picture

Assigned: Unassigned » yched

Thanks @fago. Assigning this one to @yched for review as well.

yched’s picture

Status: Needs review » Needs work

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
33.37 KB

Re-roll.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Thanks !

(wow, we get testbot results in less than 18 minutes now ? Mind blown)

Berdir’s picture

Yeah, @jthorson set up some monster-servers and runs the tests with --concurrency=32 o.0

alexpott’s picture

  1. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -378,8 +378,56 @@ public function isDisplayConfigurable($display_context) {
    +  /**
    +   * Sets a custom default value callback.
    +   *
    +   * If set, the callback overrides any set default value.
    +   *
    +   * @param string|array $callback
    +   *   The callback to invoke for getting the default value. The callback will
    +   *   be invoked with the following arguments:
    +   *   - \Drupal\Core\Entity\ContentEntityInterface $entity
    +   *     The entity being created.
    +   *   - \Drupal\Core\Field\FieldDefinitionInterface $definition
    +   *     The field definition.
    +   *   It should return the default value as documented by
    +   *   \Drupal\Core\Field\FieldDefinitionInterface::getDefaultValue().
    +   *
    +   * @return $this
    +   */
    +  public function setDefaultValueCallback($callback) {
    ...
    +  /**
    +   * Sets a default value.
    +   *
    +   * Note that if a default value callback is set, it will take precedence over
    +   * any value set here.
    +   *
    +   * @param mixed $value
    +   *   The default value in the format as returned by
    +   *   \Drupal\Core\Field\FieldDefinitionInterface::getDefaultValue().
    +   *
    +   * @return $this
    +   */
    +  public function setDefaultValue($value) {
    

    Why are these not on the FieldDefinitionInterface?

  2. +++ b/core/modules/system/tests/modules/entity_test/entity_test.module
    @@ -419,17 +418,13 @@ function entity_test_entity_test_mul_translation_delete(EntityInterface $transla
    + * @param \Drupal\Core\Entity\ContentEntityInterface $entity
    + *   The entity being created.
    + * @param \Drupal\Core\Field\FieldDefinitionInterface $definition
    + *   The field definition.
      */
    -function entity_test_field_default_value(EntityInterface $entity, FieldConfigInterface $field, FieldInstanceConfigInterface $instance, $langcode) {
    -  return array(array('value' => $field->getName() . '_' . $langcode));
    +function entity_test_field_default_value(ContentEntityInterface $entity, FieldDefinitionInterface $definition) {
    +  return array(array('value' => $definition->getName() . '_' . $entity->language()->id));
     }
    

    very minor nit alert: If you are going to reroll - we're missing an @return

bforchhammer’s picture

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

xjm’s picture

Assigned: yched » xjm

Why are these not on the FieldDefinitionInterface?

I 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:

Implementors of hook_entity_field_access_alter() need to be able to reason on a FieldDefinitionInterface object (which unifies base fields and configurable fields). Thus the patch adds an initial implementation of a FieldDefinition class for base fields, that is currently still missing in HEAD.

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.

Berdir’s picture

Yes, 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.

xjm’s picture

Thanks @Berdir. :)

xjm’s picture

Assigned: xjm » Unassigned
FileSize
33.55 KB
1.06 KB

@bforchhammer re:

Could 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: Enforce automatic node title when the Title field is hidden; see comments #11, #12.

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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 4831e2e and pushed to 8.x. Thanks!

  • Commit 4831e2e on 8.x by alexpott:
    Issue #2226267 by fago, xjm, Berdir: Improve default value handling of...
tstoeckler’s picture

Just 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 of empty() it's currently not possible to unset a default value callback using the API.
Should there be a separate unsetDefaultValueCallback()?

yched’s picture

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

bforchhammer’s picture

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

yched’s picture

Status: Fixed » Needs review
FileSize
684 bytes

Er, right.

tstoeckler’s picture

Status: Needs review » Needs work

This looks great, but I think the PHPDoc for setDefatulValueCallback() should be adapted as well.

The type-hint should be callable|null (it's string|array right now), and there should be a line that passing NULL removes a previously set default value callback.

yched’s picture

Status: Needs work » Needs review
FileSize
1.36 KB

Updated the doc.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Awesome, thanks!

  • Commit 234ed4f on 8.x by catch:
    Issue #2226267 by yched: Improve default value handling of fields to be...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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

fago’s picture

figured that this patch did not cover all existing default value implementations, see #2318605: uuid, created, changed, language default value implementations need to be updated