- EntityNG Field class has the applyDefaultValue() method.
- Configurable fields still assign their default values through field.module's hook_entity_create(), and a series of helper functions:
field_populate_default_values(), field_get_default_value().

Attached patch unifies the handling of default values for base and config fields:
- removes field_entity_create(), field_populate_default_values(), field_get_default_value()
- adds FieldDefinitionInterface::getFieldDefaultValue(), and implements it in field API's Field and FieldInstance config entities;
for Field : empty
for FieldInstance: contains the previous logic from field_get_default_value()
- splits applyDefaultValue() into a helper getDefaultValue()
- ConfigField overrides getDefaultValue() to return $instance->getFieldDefaultValue()

Those last two points can be simplified once base field definitions are FieldDefinitionInterface objects (#2047229: Make use of classes for entity field and data definitions). applyDefaultValue() can directly call $this->definition->getFieldDefaultValue()

API changes:
- field_populate_default_values() is removed - was just a helper function, not likely to have standalone uses
- field_get_default_value() --> FieldDefinitionInterface::getFieldDefaultValue()
- dynamic 'default_value_function' callbacks for configurable fields change signatures
before: callback(EntityInterface $entity, Field $field, FieldInstance $instance, $langcode)
after: callback(EntityInterface $entity, FieldDefinitionInterface $field_definition)

Files: 
CommentFileSizeAuthor
#36 default_value_unify-2050801-36.patch9.89 KByched
PASSED: [[SimpleTest]]: [MySQL] 58,317 pass(es).
[ View ]
#36 interdiff.txt2.1 KByched
#35 default_value_unify-2050801-35.patch8.64 KByched
PASSED: [[SimpleTest]]: [MySQL] 58,303 pass(es).
[ View ]
#35 interdiff.txt816 bytesyched
#32 default_value_unify-2050801-32.patch8.53 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 58,237 pass(es).
[ View ]
#30 default_value_unify-2050801-30.patch8.62 KByched
PASSED: [[SimpleTest]]: [MySQL] 58,174 pass(es).
[ View ]
#31 interdiff.txt741 bytesyched
#29 default_value_unify-2050801-29.patch8.46 KByched
PASSED: [[SimpleTest]]: [MySQL] 57,949 pass(es).
[ View ]
#29 interdiff.txt1.04 KByched
#16 default_value_unify-2050801-16.patch8.18 KByched
PASSED: [[SimpleTest]]: [MySQL] 57,963 pass(es).
[ View ]
#16 interdiff.txt551 bytesyched
#14 default_value_unify-2050801-14.patch7.64 KByched
FAILED: [[SimpleTest]]: [MySQL] 57,655 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#14 interdiff.txt2.37 KByched
#13 default_value_unify-2050801-13.patch7.61 KByched
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#13 interdiff.txt2.18 KByched
#11 default_value_unify-2050801-11.patch8.1 KByched
FAILED: [[SimpleTest]]: [MySQL] 57,603 pass(es), 22 fail(s), and 8 exception(s).
[ View ]
#11 interdiff.txt5.13 KByched
#1 field_default_value-2050801-1.patch12.12 KByched
FAILED: [[SimpleTest]]: [MySQL] 57,218 pass(es), 19 fail(s), and 1 exception(s).
[ View ]

Comments

yched’s picture

Status:Active» Needs review
StatusFileSize
new12.12 KB
FAILED: [[SimpleTest]]: [MySQL] 57,218 pass(es), 19 fail(s), and 1 exception(s).
[ View ]

Patch.

yched’s picture

Title:Unify handling of default field values» Unify handling of default values between base and configurable fields

Status:Needs review» Needs work

The last submitted patch, field_default_value-2050801-1.patch, failed testing.

yched’s picture

The test fails show that on a regular field (no default value configured) :

$entity = entity_create('entity_test', array());
$entity->field_email = 'test@example.com';
$entity->save();

$entity = entity_load('entity_test', $entity->id());
$entity->field_email[0]->value === 'test@example.com' // <-- this is FALSE.
$entity->field_email[0]->value === NULL // <-- this is TRUE.

Help welcome :-)

andypost’s picture

+++ b/core/modules/datetime/datetime.moduleundefined
@@ -264,29 +265,22 @@ function datetime_field_load($entity_type, $entities, $field, $instances, $langc
+function datetime_default_value(EntityInterface $entity, FieldDefinitionInterface $field_definition) {

+++ b/core/modules/field/tests/modules/field_test/field_test.field.incundefined
@@ -194,7 +194,7 @@ function field_test_widget_multiple_validate($element, &$form_state) {
+function field_test_default_value() {

+++ b/core/modules/system/tests/modules/entity_test/entity_test.moduleundefined
@@ -475,8 +476,8 @@ function entity_test_entity_test_mul_translation_delete(EntityInterface $transla
+function entity_test_field_default_value(EntityInterface $entity, FieldDefinitionInterface $field_definition) {

why the signatures are different?

yched’s picture

because passing $field and $instance assumes "configurable field", and we try to limit the places where we do that, in favor of the generic FieldDefinitionInterface

yched’s picture

This belongs to a larger plan to reorganize the handling of "default vaalues" for configurable fields : #2056405: Let field types provide their support for "default values" and associated input UI

yched’s picture

Priority:Normal» Critical

And critical, because, along with the issue linked above, this will be needed to fix "field CMI files contain local numeric entity ids and break on deploy".

webchick’s picture

Issue tags:+Approved API change

It seems like there's a lot of other stuff happening here besides fixing the critical; e.g. removing procedural wrappers, etc. Could we just focus this one issue on the bug at hand, and discuss those procedural wrappers in a more holistic way in a separate issue?

However, the API changes needed to directly fix the critical issue here are approved.

yched’s picture

@webchick
- There's no way field_populate_default_values() can stay, but I really can't imagine a use for it in contrib, it's really a helper function.
- I guess field_get_default_value() could be kept around as a wrapper around the new code
- the change of signature for 'default value callback' functions is about making most code work with the generic FieldDefinitionInterface rather than field.module's $field / $instance ConfigEntities. Can probably be considered in separate different patch.

I'll try to reroll with the above shortly

yched’s picture

Status:Needs work» Needs review
StatusFileSize
new5.13 KB
new8.1 KB
FAILED: [[SimpleTest]]: [MySQL] 57,603 pass(es), 22 fail(s), and 8 exception(s).
[ View ]

Restricted as per #10.
Moved the 'default_value_callback' changes in #2060907: Let "default_value_callback' functions work with FieldDefinitionInterface (postponed on this one)

Status:Needs review» Needs work

The last submitted patch, default_value_unify-2050801-11.patch, failed testing.

yched’s picture

Status:Needs work» Needs review
StatusFileSize
new2.18 KB
new7.61 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Should fix a bunch of fails.

yched’s picture

StatusFileSize
new2.37 KB
new7.64 KB
FAILED: [[SimpleTest]]: [MySQL] 57,655 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Damn, scratch #13, reversed logic in last minute refactor...
Start again, interdiff is against #11.

Status:Needs review» Needs work

The last submitted patch, default_value_unify-2050801-14.patch, failed testing.

yched’s picture

StatusFileSize
new551 bytes
new8.18 KB
PASSED: [[SimpleTest]]: [MySQL] 57,963 pass(es).
[ View ]

So, problem is:

$entity = entity_create('entity_test', array());
$entity->field_email = 'test@example.com';
$entity->save(); --> Saves no value for field_email

By the time save() reaches field_attach_insert() & field storage, the 'test@example.com' has been wiped.
This happens in EntityNG::updateOriginalValues(), that does a filterEmptyValues().
EmailItem::isEmpty() doesn't check the right stuff, and the value that has been placed is not seen, so the item is removed.

Attached patch fixes EmailItem::isEmpty().

After 2+ hrs on this, I still have no idea why this is triggered by the patch and doesn't happen in HEAD. The flow of field/item/property assignment is pretty inextricable :-(

amateescu’s picture

Status:Needs work» Needs review

Did you try this?

    $value = $this->get('value')->getValue();
    return $value === NULL || $value === '';

Edit: Also, I'm not sure $entity->field_email = 'test@example.com'; is supposed to work, shouldn't that be $entity->field_email->value = 'test@example.com';?

yched’s picture

Nope, $entity->field_email = 'test@example.com' is correct (EmailItemTest passes in HEAD).
There's a magic part that considers that the value is assigned to the first property if no property is specified.

$value = $this->get('value')->getValue(); / $value = $this->value are equivalent

yched’s picture

We're good now, this is ready for reviews :-)

amateescu’s picture

Issue tags:+sprint

Nope, $entity->field_email = 'test@example.com' is correct (EmailItemTest passes in HEAD).
There's a magic part that considers that the value is assigned to the first property if no property is specified.

That magic is the BC decorator, so IMO that test is also broken in HEAD right now (this can be verified by commenting those three lines and running the test locally).

With that said, I'm not versed enough in the EntityNG stuff to have a helpful review here, so putting the sprint tag to hopefully lure in some typed data guys

Berdir’s picture

No, $entity->$field = 'whatever' works, that's not part of the BC decorator.

amateescu’s picture

Ok, my bad.

Berdir’s picture

Had some debugging fun with this as well.

i haven't been able to *exactly* pin this down but what happens is basically this:

FieldItemBase/Map maintains $this->properties and $this->values. For performance reasons, it tries to avoid instantiating properties except when it's necessary ($this->field->value just returns the vaue, $this->field[0]->get('value')->getValue() instantiates the property and then converts it into a value again). So, relying on $this->values is wrong, as it is only there if the property isn't.

1. Field item object is created. values and properties is empty.
2. applyDefaultValue() is called, it creates the property to able to call the default value stuff on it.
3. setValue() is called, as the property exists, it unsets $this->values and sets the value on the instantiated property
(4. Now, only in 8.x, something accesses the BC decorator and changes the value, which causes the BC decorator to unset the whole field object, including the instantiated property.)
5. During invokeFieldItemPrepareCache(), we call the field again with get()->getTranslatedField()
6a) in 8.x: This causes it to be re-created based on the values on the entity object. Now, the property is not instantiated and the value is in $this->values. The isEmpty() call returns FALSE.
6b) with this patch: The field object is still there, not re-created, the property of the field item is therefore also still there and $this->values is empty. The isEmpty() call returns TRUE.

In short, the fix is correct and we'd have noticed this from the beginning if it weren't for that crazy BC stuff. Haven't reviewed the other part of the patch in detail, but looks ok to me as well, didn't see something bad.

yched’s picture

(4. Now, only in 8.x, something accesses the BC decorator and changes the value, which causes the BC decorator to unset the whole field object, including the instantiated property.)

Hah, yes, field_populate_default_values(), removed by the patch, does a getBCEntity() in HEAD.
Nice detective work!

The complexity of the "properties as objects" part of entityNG scares the sh* out of me...

klausi’s picture

Status:Needs review» Needs work
+++ b/core/lib/Drupal/Core/Entity/Field/Field.php
@@ -224,17 +224,30 @@ public function defaultAccess($operation = 'view', AccountInterface $account = N
+   * Returns the default value for the field.
+   *
+   * @return array
+   *   The default value for the field.
+   */
+  protected function getDefaultValue() {
+    if (isset($this->definition['settings']['default_value'])) {
+      return $this->definition['settings']['default_value'];
+    }
+  }

No, this will not always return an array, the default value for the uid author field on nodes is for example just 0. Do we even need this helper function? I would say just check the settings in applyDefaultValue().

+++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinitionInterface.php
@@ -156,9 +158,21 @@ public function getFieldCardinality();
+  /**
+   * Returns the default value for the field in a newly created entity.
+   *
+   * @param \Drupal\Core\Entity\EntityInterface $entity
+   *   The entity being created.
+   *
+   * @return array
+   *   The default value for the field, as an array keyed by delta and by
+   *   property names.
+   */
+  public function getFieldDefaultValue(EntityInterface $entity);

Why does this only affect newly created entities? Can't I also call that on an existing entity with existing field values?

+++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/EmailItem.php
@@ -52,6 +52,6 @@ public function getPropertyDefinitions() {
   public function isEmpty() {
-    return !isset($this->values['value']) || $this->values['value'] === '';
+    return $this->value === NULL || $this->value === '';
   }

Yay, you also hit that bug in EmailItem :-) Slightly different implementation proposed in #2002158: Convert form validation of comments to entity validation that first takes a look at the values array. Not sure which one is better?

Berdir’s picture

@klausi: I think this is the correct fix, Code should either use $this->$something (and with that __get()) or $this->get($something) (that however forces a primitive typed data object to be created), not $this->values directly. That should probably be better documented and maybe the code in __get() should be moved to a method so you don't have to rely on magic methods in your own class? Not sure how to name it though, we already have get() and getValue() and so on.

yched’s picture

No, this will not always return an array, the default value for the uid author field on nodes is for example just 0.

True, that's what tricked me initially and caused the fails in #11. I fixed the calling code but forgot to update the phpdoc.
Will update the patch asap.

Do we even need this helper function? I would say just check the settings in applyDefaultValue().

No, because how we fetch the "default value" is different for "base fields" and "configurable fields". Handling those differences should be the job of FieldefinitionInterface::getFieldDefaultValue() (introduced by this patch), but we currently have no FieldefinitionInterface object for base fields - so we need Field::getDefaultValue() so that base fields and configurable fields can have a different implementation.

As mentioned in the OP, when #2047229: Make use of classes for entity field and data definitions is fixed, then this helper can be removed, and applyDefaultValue() can simply call $field_definition->getFieldDefaultValue().

Why does this only affect newly created entities? Can't I also call that on an existing entity with existing field values?

Because that's the very definition of "default values" :-) : only come into play for freshly created entities.
This has been the case for "configurable fields" since CCK D5, and this is also the case for EntityNG's handling of default values as introduced in #1777956: Provide a way to define default values for entity fields: applyDefaultValue() only runs on entity_create(), not on entity_load().
We're very much not changing that, that would be a completely different concept.

yched’s picture

Status:Needs work» Needs review

One phpdoc adjustment is needed as per #27, but leaving to "needs review" meanwhile

yched’s picture

StatusFileSize
new1.04 KB
new8.46 KB
PASSED: [[SimpleTest]]: [MySQL] 57,949 pass(es).
[ View ]

Updated the phpdoc for FieldDefinitionInterafce::getFieldDefaultValue(), as per @klausi's remark in #25.

yched’s picture

StatusFileSize
new8.62 KB
PASSED: [[SimpleTest]]: [MySQL] 58,174 pass(es).
[ View ]

Added a @todo note to clarify that Field::getDefaultValue() is temporary before FieldDefinitionInterface objects are available for base fields (second item in comment #27).

This patch should really be ready now - its also fairly straightforward, simply leveraging the concept and API that was put in place by #1777956: Provide a way to define default values for entity fields. Anyone up for RTBC ? :-)

yched’s picture

StatusFileSize
new741 bytes

Forgot to attach the interdiff

amateescu’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new8.53 KB
PASSED: [[SimpleTest]]: [MySQL] 58,237 pass(es).
[ View ]

Rerolled after #1847002: Move entity type classes from Drupal\$provider\Plugin\Core\Entity to Drupal\$provider\Entity and also rtbc-ing as I can't see any other problem in the patch.

yched’s picture

#32: default_value_unify-2050801-32.patch queued for re-testing.

catch’s picture

+++ b/core/lib/Drupal/Core/Entity/Field/Field.php
@@ -224,17 +224,33 @@ public function defaultAccess($operation = 'view', AccountInterface $account = N
+    if (is_null($value) || (is_array($value) && empty($value))) {

Is there any value to is_null() over !isset() here? If not could we just use !isset()?

The rest looks good to me.

yched’s picture

StatusFileSize
new816 bytes
new8.64 KB
PASSED: [[SimpleTest]]: [MySQL] 58,303 pass(es).
[ View ]

is_null() is more directly in line with the logic of the test, but it could be !isset() if preferred. Added a more explicit comment.

yched’s picture

StatusFileSize
new2.1 KB
new9.89 KB
PASSED: [[SimpleTest]]: [MySQL] 58,317 pass(es).
[ View ]

Now that #2067127: Reorganize the contents of field.* files, committed earlier today, has moved @deprecated functions to field.deprecated.inc, this patch should move field_get_default_value() over there too, since it deprecates it.

catch’s picture

Title:Unify handling of default values between base and configurable fields» Change notice: Unify handling of default values between base and configurable fields
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Committed/pushed to 8.x, thanks!

Will need a change notice.

yched’s picture

Title:Change notice: Unify handling of default values between base and configurable fields» Unify handling of default values between base and configurable fields
Status:Active» Fixed
Issue tags:-Needs change record, -sprint

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

Anonymous’s picture

Issue summary:View changes

describe simplification