- 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)
Comment | File | Size | Author |
---|---|---|---|
#36 | default_value_unify-2050801-36.patch | 9.89 KB | yched |
#36 | interdiff.txt | 2.1 KB | yched |
#35 | default_value_unify-2050801-35.patch | 8.64 KB | yched |
#35 | interdiff.txt | 816 bytes | yched |
#32 | default_value_unify-2050801-32.patch | 8.53 KB | amateescu |
Comments
Comment #1
yched CreditAttribution: yched commentedPatch.
Comment #2
yched CreditAttribution: yched commentedComment #4
yched CreditAttribution: yched commentedThe test fails show that on a regular field (no default value configured) :
Help welcome :-)
Comment #5
andypostwhy the signatures are different?
Comment #6
yched CreditAttribution: yched commentedbecause passing $field and $instance assumes "configurable field", and we try to limit the places where we do that, in favor of the generic FieldDefinitionInterface
Comment #7
yched CreditAttribution: yched commentedThis 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
Comment #8
yched CreditAttribution: yched commentedAnd 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".
Comment #9
webchickIt 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.
Comment #10
yched CreditAttribution: yched commented@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
Comment #11
yched CreditAttribution: yched commentedRestricted as per #10.
Moved the 'default_value_callback' changes in #2060907: Let "default_value_callback' functions work with FieldDefinitionInterface (postponed on this one)
Comment #13
yched CreditAttribution: yched commentedShould fix a bunch of fails.
Comment #14
yched CreditAttribution: yched commentedDamn, scratch #13, reversed logic in last minute refactor...
Start again, interdiff is against #11.
Comment #16
yched CreditAttribution: yched commentedSo, problem is:
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 :-(
Comment #17
amateescu CreditAttribution: amateescu commentedDid you try this?
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';
?Comment #18
yched CreditAttribution: yched commentedNope, $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
Comment #19
yched CreditAttribution: yched commentedWe're good now, this is ready for reviews :-)
Comment #20
amateescu CreditAttribution: amateescu commentedThat 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
Comment #21
BerdirNo, $entity->$field = 'whatever' works, that's not part of the BC decorator.
Comment #22
amateescu CreditAttribution: amateescu commentedOk, my bad.
Comment #23
BerdirHad 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.
Comment #24
yched CreditAttribution: yched commentedHah, 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...
Comment #25
klausiNo, 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().
Why does this only affect newly created entities? Can't I also call that on an existing entity with existing field values?
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?
Comment #26
Berdir@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.
Comment #27
yched CreditAttribution: yched commentedTrue, 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.
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().
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.
Comment #28
yched CreditAttribution: yched commentedOne phpdoc adjustment is needed as per #27, but leaving to "needs review" meanwhile
Comment #29
yched CreditAttribution: yched commentedUpdated the phpdoc for FieldDefinitionInterafce::getFieldDefaultValue(), as per @klausi's remark in #25.
Comment #30
yched CreditAttribution: yched commentedAdded 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 ? :-)
Comment #31
yched CreditAttribution: yched commentedForgot to attach the interdiff
Comment #32
amateescu CreditAttribution: amateescu commentedRerolled 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.
Comment #33
yched CreditAttribution: yched commented#32: default_value_unify-2050801-32.patch queued for re-testing.
Comment #34
catchIs there any value to is_null() over !isset() here? If not could we just use !isset()?
The rest looks good to me.
Comment #35
yched CreditAttribution: yched commentedis_null() is more directly in line with the logic of the test, but it could be !isset() if preferred. Added a more explicit comment.
Comment #36
yched CreditAttribution: yched commentedNow 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.
Comment #37
catchCommitted/pushed to 8.x, thanks!
Will need a change notice.
Comment #38
yched CreditAttribution: yched commentedAdded https://drupal.org/node/2070839, and updated the existing https://drupal.org/node/2031221
Comment #39.0
(not verified) CreditAttribution: commenteddescribe simplification