DatetimeDatelistWidget and DateTimeDefaultWidget have this code:
public function __construct($plugin_id, array $plugin_definition, FieldInstance $instance, array $settings, $weight) {
// Identify the function used to set the default value.
$instance['default_value_function'] = $this->defaultValueFunction();
...
}
public function defaultValueFunction() {
return 'datetime_default_value';
}
It looks like the intent is to let the *widget* force the name of the function that is used to generate default values for the field.
But:
- the $instance['default_value_function'] property is only ever used in field_get_default_value(), itself only ever called in field.module's hook_entity_create() (assigns default field values on newly created entities, including "entity add" forms).
- on "entity add' forms, entity_create() has already run by the time widgets get created,
So I don't get what's the effect of this assignment of $instance['default_value_function'] ?
(#1950632: Create a FieldDefinitionInterface and use it for formatters and widgets is bumping on this code)
Comment | File | Size | Author |
---|---|---|---|
#38 | date_default_values-1989468-38.patch | 49.93 KB | plopesc |
#35 | date_default_values-1989468-35.patch | 49.96 KB | plopesc |
#34 | interdiff.txt | 727 bytes | yched |
#34 | date_default_values-1989468-34.patch | 18.97 KB | yched |
#33 | interdiff.txt | 1.01 KB | plopesc |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedLet's see what breaks.
Comment #2
Wim LeersNothing… but I think you'd need to remove the reference to it in
DatetimeFieldTest::testDefaultValue()
as well to get accurate test results?And then there are zero remaining references to
datetime_default_value()
, so let's see if I can remove that too.Comment #4
yched CreditAttribution: yched commentedSo, IIRC from conversations with @KarenS from CCK era, those functions are supposed to support specific handling of "default values" for date fields:
- The "regular" UI for default values is to display an input widget on the "instance edit" form, to let users enter a litteral value, using the widget currently assigned to the field.
- This does not fit the needs of date fields, since the most common use case for a "default value" for date fields is not an litteral, fixed date, but "the current date" (i.e the date at which the node create form is displayed - I think Date in contrib supported more advanced options like "now + 1 week", "the next friday" - anything supported by strtotime(), but I guess this has been scaled down when moving to core)
Regular date widgets only let you enter a specific date, and cannot handle this kind of input ("now"). So date fields are supposed to opt out of this "generic handling of default values", and provide their own. I guess that's what the code here does.
Now, even in current HEAD, this seems broken:
- The "instance edit form" shows both the regular "default value" widget *and* a date-field-specific select for "default value: now / no default"
- When selecting "now", the field doesn't get pre-populated on an actual entity create form.
At any rate, this is some deliberate (but broken) code here, we probably cannot remove it like that :-/
Is this blocking #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets ? I mean, it's currently broken without failing tests, so I guess we can code around it without attempting to fix it ?
Comment #5
Wim LeersFWIW, I also don't see why this blocks #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets.
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedWhether FieldDefinitionInterface will end up needing a setDefaultValueFunction() method. It doesn't currently have any setter methods. We can add it there temporarily though, and remove it if/when this lands.
Comment #7
yched CreditAttribution: yched commentedWell, FieldDefinitionInterface currently doesn't have any setter to begin with, right ?
Which is fine IMO, it is intended to be a read-only structure (e.g there is no writing properties on a FieldDefinition that comes from a base field; configurable fields are writable, because they are ConfigurableFieldInterface, not because they are FieldDefinitionInterface).
Rather than trying to override the default_value_function property at runtime, I think date fields use case would be better addressed by having the "get default value" runtime operation (currently field_get_default_value()) involve an explicit step where the field type has its say.
- in the current HEAD formulation, that would be a new hook_field_*()
- with #1969728: Implement Field API "field types" as TypedData Plugins, this might be as simple as field_get_default_value() becoming a method on Drupal\Core\Entity\Field\Type\Field - a specific field type can then just override it. I still haven't taken the time to check how #1777956: Provide a way to define default values for entity fields approaches this for base fields, though.
Comment #8
yched CreditAttribution: yched commentedOr, possible workaround before #1969728: Implement Field API "field types" as TypedData Plugins and a Drupal\Core\Entity\Field\Type\Field::getDefaultValue() method:
have DatetimeDatelistWidget & DateTimeDefaultWidget::__construct() do something like:
?
Comment #9
yched CreditAttribution: yched commentedThe following issues should let us clean up what date module is trying to do here.
#2056405: Let field types provide their support for "default values" and associated input UI
#2050801: Unify handling of default values between base and configurable fields
Comment #10
plopescHello!
Attaching new patch creating a new
DateTimeField
class that manages the default values. I created some constants in theDateTimeItem
class for setting values, maybe it's not the best approach.Any suggestion is welcomed ;)
Thanks in advance!
Comment #11
plopescRedefining the patch architecture...
Comment #12
plopescUploading a new patch where default value has been moved from
instanceSettingsForm
todefaultValuesForm
.Regards.
Comment #13
yched CreditAttribution: yched commentedAwesome !
yay!
The default values should still be an array, like:
array(0 => array('value' => 'now'))
(or just an empty array when DEFAULT_VALUE_BLANK is selected)
(the config schema requires it stays an array)
Extraneous white lines before and after that code block
Cool, but not related ?
missing space at the beginning at the sentence
Why don't you reuse the field defined in setUp() like the test did so far ?
If so, then no need to overwrite $this->field, just put it in a $field variable ?
Comment #14
plopescThanks for your review, patch re-rolled with a comment to your suggestions.
4.- These constants are used in
DateTimeItemField
to return a default value in date or datetime format. Not necessary, but I think it adds consistency :)Regards.
Comment #16
CaptainWonky CreditAttribution: CaptainWonky commentedHello
Just a quick note to say that
\Drupal\field\Plugin\Type\FieldType\ConfigField
has been renamed to\Drupal\field\Plugin\Type\FieldType\ConfigFieldItemList
It was done here - #2051923-62: Rename \Drupal\Core\Entity\Field\Field, related subclasses and interfaces to *FieldItemList
Comment #17
plopescRe-rolling...
Regards
Comment #18
plopescAlso rename
DateTimeField
toDateTimeFieldItemList
.Regards.
Comment #19
yched CreditAttribution: yched commentedWe should not need a constant for "no default value", the current practice is to just return an empty array in defaultValuesFormSubmit(), so that 'default_value' remains empty in the saved CMI file.
The key for 'no default value' could simply be '' ?
Then,
Looks like getDefaultValue() this could be simplified as well:
Comment #20
plopescRemoved 'blank' as possible default value and stored as empty.
New test for "No default value" added.
Comment #21
plopescPatch grepped. ;-)
This comment should be added to #2097691: Move FIELD_CARDINALITY_UNLIMITED constant to FieldDefinitionInterface
Sorry
Comment #22
plopescRerolled after #2050835: Move Widget, Formatter, and FieldType plugin types to the Core\Field system is in.
Comment #23
plopescRerolled after #2115145: Move field type plugins to Plugin/Field/FieldType.
Comment #24
sunLet's use #empty_option for the "No default value" option, or perhaps even just #empty_value instead so as to get the standard "- None -" option label.
Comment #25
plopescSuggestion addressed.
Thank you.
Comment #27
plopescDamn it. It was too easy...
Test fixed. Also renamed
DatetimeFieldTest
classDatetimeFieldTest
to maintain naming consistency with other datetime related test classes likeDateTimeItemTest
orDateTimePlusTest
.Regards.
Comment #28
plopescRe-roll.
Comment #29
yched CreditAttribution: yched commentedThanks for sticking to this despite reviews being a bit late :-)
I understand why you call parent::getDefaultValue() here and not the implementation in the class (since that one generates the "actual current date" for runtime use). Still calling the parent here looks a bit weird.
We should IMO directly use the raw content of the FieldInstance definition: $this->getFieldDefinition()->default_value, this is what we edit here.
Also,
- we should wrap all this in "if (empty($this->getFieldDefinition()->default_value_function)) {", like ConfigFieldItemList::defaultValuesForm() does.
- we need to override defaultValuesFormValidate() to an empty method, or else the one from ConfigFieldItemList runs, and it's not fit for our custom form.
We need to account for the fact that there might be a 'default_value_function' configured for the field, in which case it has already run in parent::getDefaultValue(), and $default_values has actual values with actual dates, and no 'default_date' entry.
Adding an if (isset($default_values[0]['default_date']) should do the job.
The descriptions are a bit misleading, those are possible values for the "datetime_type" setting, so this is field-by-field rather than about the "field type".
Maybe:
"Value for the 'datetime_type' setting: store only a date."
"Value for the 'datetime_type' setting: store a date and time."
?
Didn't look at the tests yet.
Comment #30
plopescThanks for your review.
New patch addressing your points.
Regards.
Comment #31
yched CreditAttribution: yched commentedThanks!
No need for the first statement in the if now, the isset() check encompasses that already.
Still need to review the tests
Comment #32
yched CreditAttribution: yched commentedFinally got to try this, works just fine, and ++ for the extensive tests :-)
Thanks!
Comment #33
plopescAddressing commnet #31, I didn't work on it waiting until your tests comments.
Finally we got it, although it has been a looong road :)
Thanks!
Comment #34
yched CreditAttribution: yched commented#1973534: Provide config schema to field types and storage in datetime module added a config schema for the date field, which means we now need to change it accordingly.
(the schema added over there included the long gone user_register_form setting, patch removed it as well)
Comment #35
plopescRe-roll after #2143263: Remove "Field" prefix from FieldDefinitionInterface methods
Comment #36
yched CreditAttribution: yched commented35: date_default_values-1989468-35.patch queued for re-testing.
Comment #37
yched CreditAttribution: yched commentedGreen. RTBC bump, then :-)
Comment #38
plopescRe-rolling again
Regards.
Comment #39
xjm38: date_default_values-1989468-38.patch queued for re-testing.
Comment #42
plopesc38: date_default_values-1989468-38.patch queued for re-testing.
Comment #43
yched CreditAttribution: yched commentedStill RTBC if green
Comment #44
yched CreditAttribution: yched commentedGetting this in would help clarify the situation with #2164151: Regression: Cannot create field with unlimited values ...
Comment #45
yched CreditAttribution: yched commentedActually, this fixes a fatal error when you create a date field with "unlimited values".
Thus, raising to "critical".
#2164151: Regression: Cannot create field with unlimited values is open to determine whether a similar bug can be observed on other field types (been reported but not consistently reproduced).
Comment #46
yched CreditAttribution: yched commented38: date_default_values-1989468-38.patch queued for re-testing.
Comment #47
catchCommitted/pushed to 8.x, thanks!