Timestamp field type doesn't provide sample values

Thanks for reporting and fixing this. Quick, first look here...

+++ b/core/modules/field/tests/src/Kernel/Timestamp/TimestampFormatterTest.php
@@ -10,18 +10,14 @@
-class TimestampFormatterTest extends KernelTestBase {
-
-  /**
-   * {@inheritdoc}
-   */
-  public static $modules = ['system', 'field', 'text', 'entity_test', 'user'];
+class TimestampFormatterTest extends FieldKernelTestBase {

I think this is an out of scope change, and I don't think this really belongs in TimestampFormatterTest. Maybe create TimestampItemTest and put it there? Can we also post a test-only patch? The method exists on the class, it is just a no-op because the base class is an empty function. So, this should be eligible for 8.2.x in a point release as a bugfix.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/TimestampItem.php
@@ -53,4 +54,16 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
+    $timestamp = REQUEST_TIME - mt_rand(0, 86400 * 365);

Little hesitant to use `REQUEST_TIME` directly here, but injecting the request stack to get it out of the request is seriously overkill for this situation. Tempted to leave this as-is, and then handle it as part of the followups to #2717207: Add a time service to provide consistent interaction with time()/microtime() and superglobals.

Will take a closer look at this once you think you have it settled out.

OK, looked at this in place.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/TimestampItem.php
   @@ -53,4 +54,16 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
   +    // Just pick a date in the past year. No guidance is provided by this Field
   +    // type.
   

   How about "Pick a random timestamp in the past year."

2. +++ b/core/modules/field/tests/src/Kernel/Timestamp/TimestampFormatterTest.php
   @@ -10,18 +10,14 @@
   +use Drupal\Tests\field\Kernel\FieldKernelTestBase;
    
    /**
     * Tests the timestamp formatters.
     *
     * @group field
     */
   -class TimestampFormatterTest extends KernelTestBase {
   -
   -  /**
   -   * {@inheritdoc}
   -   */
   -  public static $modules = ['system', 'field', 'text', 'entity_test', 'user'];
   +class TimestampFormatterTest extends FieldKernelTestBase {
    
   

   This is an out-of-scope change. Let's create TimestampItemTest as a FieldKernelTestBase instead.

3. +++ b/core/modules/field/tests/src/Kernel/Timestamp/TimestampFormatterTest.php
   @@ -131,6 +127,11 @@ public function testTimestampFormatter() {
   +
   +      // Test sample item generation.
   +      $entity = EntityTest::create();
   +      $entity->{$this->fieldName}->generateSampleItems();
   +      $this->entityValidateAndSave($entity);
        }
   

   This test doesn't fail when I revert the change to TimestampItem. I suspect it is because of PHP weirdness, and an empty value is getting cast to a 0 somewhere, which is a valid timestamp.

OK, this is pretty good. The fail in #12 for 8.4 looks unrelated, but this still needs a little work. The main problem is that TimestampItem has no real relation to DatetimeItem, or the datetime.module. We need to make sure we don't couple them in the test.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/TimestampItem.php
   @@ -53,4 +54,15 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
   +    // Just pick a date in the past year. No guidance is provided by this Field
   +    // type.
   

   How about "Pick a random timestamp in the past year."

2. +++ b/core/modules/datetime/tests/src/Kernel/DateTimestampItemTest.php
   @@ -0,0 +1,102 @@
   +namespace Drupal\Tests\datetime\Kernel;
   

   We need to place this outside the datetime module. The `Drupal\Tests\field\Kernel\Timestamp` namespace is probably better.

3. +++ b/core/modules/datetime/tests/src/Kernel/DateTimestampItemTest.php
   @@ -0,0 +1,102 @@
   + * @group datetime
   

   @group field would be better.

4. +++ b/core/modules/datetime/tests/src/Kernel/DateTimestampItemTest.php
   @@ -0,0 +1,102 @@
   +class DateTimestampItemTest extends FieldKernelTestBase {
   

   Should be TimestampItem test, to match the field class.

5. +++ b/core/modules/datetime/tests/src/Kernel/DateTimestampItemTest.php
   @@ -0,0 +1,102 @@
   +  public static $modules = ['datetime'];
   

   OK, this is the crux of the last few comments :) TimestampItem is provided by Core, not datetime.module, so we need to explicitly make sure this isn't installed.

6. +++ b/core/modules/datetime/tests/src/Kernel/DateTimestampItemTest.php
   @@ -0,0 +1,102 @@
   +  protected function setUp() {
   +    parent::setUp();
   

   Missing the @{inheritdoc}

7. +++ b/core/modules/datetime/tests/src/Kernel/DateTimestampItemTest.php
   @@ -0,0 +1,102 @@
   +    $this->fieldStorage->setSetting('datetime_type', DateTimeItem::DATETIME_TYPE_DATETIME);
   +    $this->fieldStorage->save();
   +
   

   Not sure why this is needed; that setting doesn't exist on timestamps.

8. +++ b/core/modules/datetime/tests/src/Kernel/DateTimestampItemTest.php
   @@ -0,0 +1,102 @@
   +    $this_year = \Drupal::time()->getRequestTime() - 86400 * 366;
   +    $this->assertGreaterThan($this_year, $entity->field_timestamp->value);
   

   Not quite sure what this is testing?

Sorry for the delay on final review of this. Just a few small things.

1. +++ b/core/modules/field/tests/src/Kernel/Timestamp/TimestampItemTest.php
   @@ -0,0 +1,92 @@
   +use Drupal\Core\Field\FieldItemListInterface;
   +use Drupal\Core\Field\FieldItemInterface;
   +use Drupal\entity_test\Entity\EntityTest;
   +use Drupal\field\Entity\FieldConfig;
   +use Drupal\Tests\field\Kernel\FieldKernelTestBase;
   +use Drupal\field\Entity\FieldStorageConfig;
   

   Nit; should be alphabetical.

2. +++ b/core/modules/field/tests/src/Kernel/Timestamp/TimestampItemTest.php
   @@ -0,0 +1,92 @@
   +    // Test the generateSampleValue() method.
   +    $entity = EntityTest::create();
   +    $entity->field_timestamp->generateSampleItems();
   +    $this->entityValidateAndSave($entity);
   +
   

   To be consistent with other tests, let's just say "Test sample item generation."

3. +++ b/core/modules/field/tests/src/Kernel/Timestamp/TimestampItemTest.php
   @@ -0,0 +1,92 @@
   +    // Ensure there is sample value a generated for the field.
   +    $field_value = $entity->field_timestamp->value;
   +    $this->assertTrue(!empty($field_value));
   +  }
   +
   

   We can get rid of this. None of the other item tests seem to do this.

Verified TimestampItemTest is being picked up. If we fix these, we are RTBC. We may want some followups to enhance the test coverage, but that is lower priority.

Sorry for the delay; missed it in my last NR sweep. This looks good to me.

The bug fix itself looks good for me. I also updated the issue credit.

+++ b/core/modules/field/tests/src/Kernel/Timestamp/TimestampItemTest.php
@@ -0,0 +1,88 @@
+    $this->assertEqual($entity->field_timestamp->value, $value);
+    $this->assertEqual($entity->field_timestamp[0]->value, $value);
...
+    $this->assertEqual($entity->field_timestamp->value, $new_value);
...
+    $this->assertEqual($entity->field_timestamp->value, $new_value);

KernelTestBase::assertEqual has been deprecated. Could you also post a failing test patch because some of the assertions have been removed since the last failing test?

Thank you for adding the additional test coverage for the TimestampItem.

Looks good to me.

@lauriii asked me about the abstract class implementing an empty generateSampleValue - I agreed this was odd and that we should remove this in 9.0.0 to ensure that all field types implement this properly. Opened #2872790: Deprecate \Drupal\Core\Field\FieldItemBase::generateSampleValue

Thanks @alexpott, that makes sense.

Committed 02cd2f4 and pushed to 8.4.x. Thanks!