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)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

Status: Active » Needs review
FileSize
2.49 KB

Let's see what breaks.

Wim Leers’s picture

Nothing… 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.

Status: Needs review » Needs work

The last submitted patch, date_default_values_function-1989468-2.patch, failed testing.

yched’s picture

So, 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 ?

Wim Leers’s picture

effulgentsia’s picture

Whether 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.

yched’s picture

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

yched’s picture

Or, 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:

public function __construct($plugin_id, array $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $weight) {
  if ($field_definition instanceof FieldInstanceInterface) {
    $instance['default_value_function'] = 'datetime_default_value';
  }
}

?

yched’s picture

plopesc’s picture

Status: Needs work » Needs review
FileSize
12.34 KB

Hello!

Attaching new patch creating a new DateTimeField class that manages the default values. I created some constants in the DateTimeItem class for setting values, maybe it's not the best approach.

Any suggestion is welcomed ;)

Thanks in advance!

plopesc’s picture

Status: Needs review » Needs work

Redefining the patch architecture...

plopesc’s picture

Status: Needs work » Needs review
FileSize
16.35 KB

Uploading a new patch where default value has been moved from instanceSettingsForm to defaultValuesForm.

Regards.

yched’s picture

Awesome !

  1. +++ b/core/modules/datetime/datetime.module
    @@ -167,48 +167,6 @@ function datetime_datelist_widget_validate(&$element, &$form_state) {
    -function datetime_default_value($entity, $field, $instance, $langcode) {
    

    yay!

  2. +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/field/field_type/DateTimeField.php
    @@ -0,0 +1,86 @@
    +    return $form_state['values']['default_value_input'];
    

    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)

  3. +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/field/field_type/DateTimeField.php
    @@ -0,0 +1,86 @@
    +
    +        // A default value should be in the format and timezone used for date
    +        // storage.
    +        $date = new DrupalDateTime('now', DATETIME_STORAGE_TIMEZONE);
    +        $storage_format = $this->getFieldDefinition()->getFieldSetting('datetime_type') == DateTimeItem::DATETIME_TYPE_DATE ? DATETIME_DATE_STORAGE_FORMAT: DATETIME_DATETIME_STORAGE_FORMAT;
    +        $value = $date->format($storage_format);
    +
    

    Extraneous white lines before and after that code block

  4. +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/field/field_type/DateTimeItem.php
    @@ -24,16 +24,24 @@
    +   * Defines the field type as date.
    +   */
    +  const DATETIME_TYPE_DATE = 'date';
    +
    +  /**
    +   * Defines the field type as datetime.
    +   */
    +  const DATETIME_TYPE_DATETIME = 'datetime';
    +
    

    Cool, but not related ?

  5. +++ b/core/modules/datetime/lib/Drupal/datetime/Tests/DatetimeFieldTest.php
    @@ -292,40 +291,44 @@ function testDatelistWidget() {
    +    //Create a test content type.
    

    missing space at the beginning at the sentence

  6. +++ b/core/modules/datetime/lib/Drupal/datetime/Tests/DatetimeFieldTest.php
    @@ -292,40 +291,44 @@ function testDatelistWidget() {
    +    $this->field = entity_create('field_entity', array(
    

    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 ?

plopesc’s picture

Thanks 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.

Status: Needs review » Needs work

The last submitted patch, date_default_values-1989468-14.patch, failed testing.

CaptainWonky’s picture

Hello

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

plopesc’s picture

Status: Needs work » Needs review
FileSize
5.31 KB
16.67 KB

Re-rolling...

Regards

plopesc’s picture

Also rename DateTimeField to DateTimeFieldItemList.

Regards.

yched’s picture

  1. +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/field/field_type/DateTimeFieldItemList.php
    @@ -0,0 +1,84 @@
    +  /**
    +   * Defines the default value as blank.
    +   */
    +  const DEFAULT_VALUE_BLANK = 'blank';
    

    We 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.

  2. +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/field/field_type/DateTimeFieldItemList.php
    @@ -0,0 +1,84 @@
    +        '#options' => array(static::DEFAULT_VALUE_BLANK => t('No default value'), static::DEFAULT_VALUE_NOW => t('The current date')),
    

    The key for 'no default value' could simply be '' ?

  3. +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/field/field_type/DateTimeFieldItemList.php
    @@ -0,0 +1,84 @@
    +  public function defaultValuesFormSubmit(array $element, array &$form, array &$form_state) {
    +    return array($form_state['values']['default_value_input']);
    +  }
    

    Then,

    if ($form_state['values']['default_value_input']) {
      return array($form_state['values']['default_value_input']);
    }
    return array();
    
  4. +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/field/field_type/DateTimeFieldItemList.php
    @@ -0,0 +1,84 @@
    +    if ($default_value) {
    +      $value = '';
    +      $date = '';
    +      if ($default_value[0]['default_date'] == static::DEFAULT_VALUE_NOW) {
    

    Looks like getDefaultValue() this could be simplified as well:

    if ($default_value && $default_value[0]['default_date'] == static::DEFAULT_VALUE_NOW) {
      // (comment)
      $date = new DrupalDateTime('now', DATETIME_STORAGE_TIMEZONE);
      $storage_format = $this->getFieldDefinition()->getFieldSetting('datetime_type') == DateTimeItem::DATETIME_TYPE_DATE ? DATETIME_DATE_STORAGE_FORMAT: DATETIME_DATETIME_STORAGE_FORMAT;
      // (comment)
      $default_value =  array(
        array(
           'value' => $date->format($storage_format);
           'date' => $date,
        )
      );
    }
    
plopesc’s picture

Removed 'blank' as possible default value and stored as empty.

New test for "No default value" added.

plopesc’s picture

Patch grepped. ;-)

This comment should be added to #2097691: Move FIELD_CARDINALITY_UNLIMITED constant to FieldDefinitionInterface

Sorry

plopesc’s picture

plopesc’s picture

sun’s picture

+++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/Field/FieldType/DateTimeFieldItemList.php
@@ -0,0 +1,78 @@
+        '#options' => array('' => t('No default value'), static::DEFAULT_VALUE_NOW => t('The current date')),

Let'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.

plopesc’s picture

Issue summary: View changes
FileSize
17.56 KB
972 bytes

Suggestion addressed.

Thank you.

Status: Needs review » Needs work

The last submitted patch, 25: date_default_values-1989468-25.patch, failed testing.

plopesc’s picture

Status: Needs work » Needs review
FileSize
29.56 KB
20.02 KB

Damn it. It was too easy...

Test fixed. Also renamed DatetimeFieldTest class DatetimeFieldTest to maintain naming consistency with other datetime related test classes like DateTimeItemTest or DateTimePlusTest.

Regards.

plopesc’s picture

FileSize
48.94 KB

Re-roll.

yched’s picture

Status: Needs review » Needs work

Thanks for sticking to this despite reviews being a bit late :-)

  1. +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/Field/FieldType/DateTimeFieldItemList.php
    @@ -0,0 +1,79 @@
    +  public function defaultValuesForm(array &$form, array &$form_state) {
    +    $default_value = parent::getDefaultValue();
    

    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.

  2. +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/Field/FieldType/DateTimeFieldItemList.php
    @@ -0,0 +1,79 @@
    +  public function getDefaultValue() {
    +    $default_value = parent::getDefaultValue();
    +
    +    if ($default_value && $default_value[0]['default_date'] == static::DEFAULT_VALUE_NOW) {
    

    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.

  3. +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/Field/FieldType/DateTimeItem.php
    @@ -22,16 +22,24 @@
       /**
    +   * Defines the field type as date.
    +   */
    +  const DATETIME_TYPE_DATE = 'date';
    +
    +  /**
    +   * Defines the field type as datetime.
    +   */
    +  const DATETIME_TYPE_DATETIME = 'datetime';
    

    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.

plopesc’s picture

Status: Needs work » Needs review
FileSize
49.28 KB
3.57 KB

Thanks for your review.

New patch addressing your points.

Regards.

yched’s picture

Thanks!

+++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/Field/FieldType/DateTimeFieldItemList.php
@@ -0,0 +1,86 @@
+    if ($default_value && isset($default_value[0]['default_date']) && $default_value[0]['default_date'] == static::DEFAULT_VALUE_NOW) {

No need for the first statement in the if now, the isset() check encompasses that already.

Still need to review the tests

yched’s picture

Status: Needs review » Reviewed & tested by the community

Finally got to try this, works just fine, and ++ for the extensive tests :-)
Thanks!

plopesc’s picture

Addressing 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!

yched’s picture

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

plopesc’s picture

yched’s picture

yched’s picture

Green. RTBC bump, then :-)

plopesc’s picture

Re-rolling again

Regards.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 38: date_default_values-1989468-38.patch, failed testing.

The last submitted patch, 38: date_default_values-1989468-38.patch, failed testing.

plopesc’s picture

Status: Needs work » Needs review
yched’s picture

Status: Needs review » Reviewed & tested by the community

Still RTBC if green

yched’s picture

Getting this in would help clarify the situation with #2164151: Regression: Cannot create field with unlimited values ...

yched’s picture

Priority: Normal » Critical

Actually, 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).

yched’s picture

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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