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)

Files: 
CommentFileSizeAuthor
#38 date_default_values-1989468-38.patch49.93 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 60,094 pass(es).
[ View ]
#35 date_default_values-1989468-35.patch49.96 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 59,408 pass(es).
[ View ]
#34 interdiff.txt727 bytesyched
#34 date_default_values-1989468-34.patch18.97 KByched
PASSED: [[SimpleTest]]: [MySQL] 59,294 pass(es).
[ View ]
#33 interdiff.txt1.01 KBplopesc
#33 date_default_values-1989468-33.patch49.26 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 59,201 pass(es).
[ View ]
#30 interdiff.txt3.57 KBplopesc
#30 date_default_values-1989468-30.patch49.28 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 59,081 pass(es).
[ View ]
#28 date_default_values-1989468-28.patch48.94 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 59,084 pass(es).
[ View ]
#25 interdiff.txt972 bytesplopesc
#25 date_default_values-1989468-25.patch17.56 KBplopesc
FAILED: [[SimpleTest]]: [MySQL] 57,998 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#23 date_default_values-1989468-23.patch17.55 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 59,714 pass(es).
[ View ]
#22 date_default_values-1989468-22.patch17.67 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 59,415 pass(es).
[ View ]
#21 move_cardinality-2097691-8.patch35.21 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 58,680 pass(es).
[ View ]
#21 interdiff.txt26.96 KBplopesc
#20 date_default_values-1989468-20.patch17.88 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 58,342 pass(es).
[ View ]
#20 interdiff.txt5.24 KBplopesc
#18 date_default_values-1989468-18.patch16.71 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 59,344 pass(es).
[ View ]
#18 interdiff.txt1.82 KBplopesc
#17 date_default_values-1989468-17.patch16.67 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 59,321 pass(es).
[ View ]
#17 interdiff.txt5.31 KBplopesc
#14 date_default_values-1989468-14.patch16.16 KBplopesc
FAILED: [[SimpleTest]]: [MySQL] 58,987 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#14 interdiff.txt2.69 KBplopesc
#12 date_default_values-1989468-12.patch16.35 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 58,981 pass(es).
[ View ]
#10 date_default_values-1989468-10.patch12.34 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 59,014 pass(es).
[ View ]
#2 date_default_values_function-1989468-2.patch5.29 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 56,437 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#2 interdiff.txt2.66 KBWim Leers
#1 date_default_values_function.patch2.49 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 55,331 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new2.49 KB
PASSED: [[SimpleTest]]: [MySQL] 55,331 pass(es).
[ View ]

Let's see what breaks.

StatusFileSize
new2.66 KB
new5.29 KB
FAILED: [[SimpleTest]]: [MySQL] 56,437 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

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 ?

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.

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.

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:

<?php
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';
  }
}
?>

?

Status:Needs work» Needs review
StatusFileSize
new12.34 KB
PASSED: [[SimpleTest]]: [MySQL] 59,014 pass(es).
[ View ]

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!

Status:Needs review» Needs work

Redefining the patch architecture...

Status:Needs work» Needs review
StatusFileSize
new16.35 KB
PASSED: [[SimpleTest]]: [MySQL] 58,981 pass(es).
[ View ]

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

Regards.

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 ?

StatusFileSize
new2.69 KB
new16.16 KB
FAILED: [[SimpleTest]]: [MySQL] 58,987 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

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.

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

Status:Needs work» Needs review
StatusFileSize
new5.31 KB
new16.67 KB
PASSED: [[SimpleTest]]: [MySQL] 59,321 pass(es).
[ View ]

Re-rolling...

Regards

StatusFileSize
new1.82 KB
new16.71 KB
PASSED: [[SimpleTest]]: [MySQL] 59,344 pass(es).
[ View ]

Also rename DateTimeField to DateTimeFieldItemList.

Regards.

  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:

    <?php
    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,
        )
      );
    }
    ?>

StatusFileSize
new5.24 KB
new17.88 KB
PASSED: [[SimpleTest]]: [MySQL] 58,342 pass(es).
[ View ]

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

New test for "No default value" added.

StatusFileSize
new26.96 KB
new35.21 KB
PASSED: [[SimpleTest]]: [MySQL] 58,680 pass(es).
[ View ]

Patch grepped. ;-)

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

Sorry

StatusFileSize
new17.67 KB
PASSED: [[SimpleTest]]: [MySQL] 59,415 pass(es).
[ View ]

StatusFileSize
new17.55 KB
PASSED: [[SimpleTest]]: [MySQL] 59,714 pass(es).
[ View ]

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

Issue summary:View changes
StatusFileSize
new17.56 KB
FAILED: [[SimpleTest]]: [MySQL] 57,998 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new972 bytes

Suggestion addressed.

Thank you.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new29.56 KB
PASSED: [[SimpleTest]]: [MySQL] 57,809 pass(es).
[ View ]
new20.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.

StatusFileSize
new48.94 KB
PASSED: [[SimpleTest]]: [MySQL] 59,084 pass(es).
[ View ]

Re-roll.

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.

Status:Needs work» Needs review
StatusFileSize
new49.28 KB
PASSED: [[SimpleTest]]: [MySQL] 59,081 pass(es).
[ View ]
new3.57 KB

Thanks for your review.

New patch addressing your points.

Regards.

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

Status:Needs review» Reviewed & tested by the community

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

StatusFileSize
new49.26 KB
PASSED: [[SimpleTest]]: [MySQL] 59,201 pass(es).
[ View ]
new1.01 KB

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!

StatusFileSize
new18.97 KB
PASSED: [[SimpleTest]]: [MySQL] 59,294 pass(es).
[ View ]
new727 bytes

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

StatusFileSize
new49.96 KB
PASSED: [[SimpleTest]]: [MySQL] 59,408 pass(es).
[ View ]

Green. RTBC bump, then :-)

StatusFileSize
new49.93 KB
PASSED: [[SimpleTest]]: [MySQL] 60,094 pass(es).
[ View ]

Re-rolling again

Regards.

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.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

Still RTBC if green

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

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

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.