Add explicit test coverage for JSON:API filtering on a datetime field

Isn't this working as expected? You are querying by a timestamp and DateTimeItemInterface::DATETIME_STORAGE_FORMAT is Y-m-d\TH:i:s. Are you expecting the timestamp to be converted to a string format on query?

Looking at: https://www.drupal.org/docs/8/modules/jsonapi/filtering, it is conflicting.

Filtering on Date (Date only, no time)

filter[datefilter][condition][path]=field_test_date
filter[datefilter][condition][operator]=%3D
filter[datefilter][condition][value]=2019-06-27

Filtering on Date (with Date AND Time)

filter[datefilter][condition][path]=field_test_date
filter[datefilter][condition][operator]=%3D
filter[datefilter][condition][value]=1562004576

It shows that date and time should require a timestamp. But these docs are using =

Looks like the JSON API documentation needs to be updated. You cannot pass a timestamp anymore. You must pass a time string.

I updated the documentation node. I figure we can close this, then.

I agree this would be a good addition to JSON:API's test coverage.

1. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalDateFieldTest.php
   @@ -0,0 +1,161 @@
   +    // TODO: Check exactly which node is present.
   ...
   +    // TODO: Check exactly which node is present.
   ...
   +    // TODO: Check exactly which node is present.
   ...
   +    // TODO: Check exactly which node is present.
   

   These still need to be implemented.

2. I'm wondering if this test coverage should live in core/modules/datetime?

1. Took some inspiration from core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalTest.php and added assertions for checking the nodes.

2.

I'm wondering if this test coverage should live in core/modules/datetime?

Not certain about this one as the issue is still under jsonapi.module but would love to do the required changes if needed

Resolving deprecation notices

Great work, this seems like a good addition. I would need a reroll to 9.3.x i think. Could you do that? I'll review it again after.

No it doesn't need a reroll. Well done.

Just some suggestions for improvements to the comments.

1. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalDateFieldTest.php
   @@ -0,0 +1,183 @@
   + * General functional test class.
   

   The comment should reflect that this is a test of the date field not a 'General' test.

2. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalDateFieldTest.php
   @@ -0,0 +1,183 @@
   +    $timestamp_node_1 = '5000000';
   +    $timestamp_node_2 = '6000000';
   +    $timestamp_node_3 = '7000000';
   

   The naming here is a bit confusing. $timestamp_node_2 is used for node 1 and node 2 and node 3. I think it would be easier to follow to remove the '_node_' part of the name.

3. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalDateFieldTest.php
   @@ -0,0 +1,183 @@
   +    // >.
   ...
   +    // >=.
   ...
   +    // <.
   ...
   +    // =<.
   

   These are rather cryptic. It would be easier to understand if this was in plain English.

Addressed changes specified in #19 Not 100% sure about #19.3, gave it a try though. Thanks!

Thanks for the changes (and for the feedback @quietone). Looking good.

Nice work here!

1. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalDateFieldTest.php
   @@ -0,0 +1,183 @@
   + * JSON:API integration test for the "Date" field.
   

   I was gonna say "isn't it 'datetime'?" — but no:

    * @FieldType(
    *   id = "datetime",
    *   label = @Translation("Date"),
   …
    *   list_class = "\Drupal\datetime\Plugin\Field\FieldType\DateTimeFieldItemList",
    *   constraints = {"DateTimeFormat" = {}}
    * )
    */
   class DateTimeItem
   

   🙈

2. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalDateFieldTest.php
   @@ -0,0 +1,183 @@
   +    $field_config = FieldConfig::create([
   +      'field_name' => 'field_datetime',
   +      'label' => 'Date and time',
   +      'entity_type' => 'node',
   +      'bundle' => 'article',
   +      'required' => FALSE,
   +      'settings' => [],
   +      'description' => '',
   +    ]);
   +    $field_config->save();
   

   Übernit: 🤔 Why assign it to a variable if you're not going to use it?

3. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalDateFieldTest.php
   @@ -0,0 +1,183 @@
   +   * Test the GET method.
   

   Übernit: s/Test/Tests/

4. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalDateFieldTest.php
   @@ -0,0 +1,183 @@
   +          'operator' => '>',
   +          'value' => date(DateTimeItemInterface::DATETIME_STORAGE_FORMAT, $timestamp_greater_than_value),
   

   So this is the key thing that this is implicitly documenting through tests: you apparently have to use the storage-level datetime representation to get this to work.

   That makes sense from a Field API POV.

   It's not ideal from a JSON:API POV. But … we can still work to improve this in the future obviously :)

   Wonderful to have this explicitly documented & tested!

Keeping at RTBC.

Unrelated test failure.

Committed and pushed 623cc951b5 to 9.3.x and 7fb32c14ed to 9.2.x. Thanks!

Backported to 9.2.x as this is a test only change.