Split from #2318605: uuid, created, changed, language default value implementations need to be updated.
Problem/Motivation
The current API allows specifying "allowed values" using two different formats:
- a literal, in which case it will be assigned to the first property of the first item.
- a numerically indexed array of items, each item being a property/value array.- NULL or array() for no default value
I.e you can do either one of:
$base_field_definition->setDefaultValue(42);
$base_field_definition->setDefaultValue(array(
array('value' => 42),
array('value' => 43),
));
That $default_value gets passed as is (litteral or array) to FIL::processDefaultValue(), which is supposed to "massage" it (e.g transform 'NOW' into the current timestamp).
Problem: all current implementations (including the ones added in #2318605: uuid, created, changed, language default value implementations need to be updated) fail to akcknoledge that polymorphism: they all assume one of the two possible formats, and break if they receive the other.
Proposed resolution
We can't expect each FIL::processDefaultValue() to do the legwork of supporting that polymorphism.
We should normalize the $default_value present in the Definition to the array format before calling FIL::processDefaultValue() on it, so that implementations can do their massaging on a predictable format.
This could be done either:
1) in the base implementation of FieldItemList::processDefaultValues(),
Pros:
- easy...
Cons:
- only works if child implementations think of calling the parent first
- keeps polymorphic input on FIL::processDefaultValue(), which is painful to document (see the curent phpdoc)
2) by the caller of ::processDefaultValues(), before the call.
Pros:
- removes polymorphism on FIL::processDefaultValue() completely, clearer and easier to document
Cons:
- the code that calls ::processDefaultValues() is duplicated in BaseFieldDefinition::getDefaultValue() and FieldConfigBase::getDefaultValue()...
Both approaches remove polymorphism on FDI::getDefaultValue(), since it always returns an array. That's a good thing, polymorphism is nice DX on setters but terrible DX on getters :-)
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | interdiff.txt | 4.36 KB | yched |
| #8 | 2347711-processDefaultValue_normalize-8.patch | 7.03 KB | yched |
| #6 | interdiff.txt | 1.03 KB | yched |
| #6 | 2347711-processDefaultValue_normalize-6.patch | 2.93 KB | yched |
| #3 | 2347711-processDefaultValue_normalize-3.patch | 1.76 KB | undertext |
Comments
Comment #1
yched commentedPatch goes with approach 2). Some phpdocs still need to be adjusted.
Comment #3
undertext commentedreset() requires a reference. So fixed it.
Comment #5
yched commentedDoh, indeed. Thanks undertext :-)
(side note: when posting a new patch, it is highly recommended to post an interdiff as well, so that other people participating in the issue can see the changes you made without re-reading the whole patch - see https://www.drupal.org/documentation/git/interdiff)
I'm looking into the fail.
Comment #6
yched commentedTest fix.
Comment #7
yched commentedNote : depending on which one of #2318605: uuid, created, changed, language default value implementations need to be updated or this one gets in first, the other one will needs to be adjusted.
Comment #8
yched commentedAdjusting the docs of FDI::getDefaultValue(), BFD::setDefaultValue() and FILI::processDefaultValue(), according to where the polymorphism gets normalized.
Comment #9
yched commentedThis should be ready for reviews
Comment #11
amateescu commentedI think the patch looks great but we need to fix those two fails before RTBC :)
Comment #13
amateescu commentedThose were just random fails.
Comment #14
alexpottI agree that
.
Committed f33b510 and pushed to 8.0.x. Thanks!