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

Comments

yched’s picture

Status: Active » Needs review
StatusFileSize
new1.69 KB

Patch goes with approach 2). Some phpdocs still need to be adjusted.

Status: Needs review » Needs work

The last submitted patch, 1: 2347711-processDefaultValue_normalize-1.patch, failed testing.

undertext’s picture

Status: Needs work » Needs review
StatusFileSize
new1.76 KB

reset() requires a reference. So fixed it.

Status: Needs review » Needs work

The last submitted patch, 3: 2347711-processDefaultValue_normalize-3.patch, failed testing.

yched’s picture

reset() requires a reference

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

yched’s picture

Status: Needs work » Needs review
StatusFileSize
new2.93 KB
new1.03 KB

Test fix.

yched’s picture

Note : 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.

yched’s picture

StatusFileSize
new7.03 KB
new4.36 KB

Adjusting the docs of FDI::getDefaultValue(), BFD::setDefaultValue() and FILI::processDefaultValue(), according to where the polymorphism gets normalized.

yched’s picture

This should be ready for reviews

Status: Needs review » Needs work

The last submitted patch, 8: 2347711-processDefaultValue_normalize-8.patch, failed testing.

amateescu’s picture

I think the patch looks great but we need to fix those two fails before RTBC :)

amateescu’s picture

Status: Needs work » Reviewed & tested by the community

Those were just random fails.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I agree that

polymorphism is nice DX on setters but terrible DX on getters

.

Committed f33b510 and pushed to 8.0.x. Thanks!

  • alexpott committed f33b510 on 8.0.x
    Issue #2347711 by yched, undertext: Fixed FieldItemlListInterface::...

Status: Fixed » Closed (fixed)

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