Problem/Motivation

See #2860350: Document why JSON API only supports @DataType-level normalizers and particularly #2860350-33: Document why JSON API only supports @DataType-level normalizers and #2860350-36: Document why JSON API only supports @DataType-level normalizers.

Quoting #2860350: Document why JSON API only supports @DataType-level normalizers:

JSON API does not inherit Drupal 8.4's improved normalization of timestamp fields (#2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX)

Proposed resolution

Until #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API lands and JSON API can require the Drupal 8 minor in which it was introduced, we need to add our own work-around that implements the exact same behavior.

Remaining tasks

  1. Patch.
  2. Test coverage.
  3. Review.

User interface changes

None.

API changes

See CR.

Data model changes

None.

CommentFileSizeAuthor
#33 2929932-33-do-not-test.patch27.23 KBWim Leers
#33 2929932-33.patch73.99 KBWim Leers
#33 interdiff.txt976 bytesWim Leers
#31 2929932-31-do-not-test.patch26.31 KBWim Leers
#31 2929932-31.patch73.03 KBWim Leers
#25 2929932-25-do-not-test.patch26.3 KBWim Leers
#25 2929932-25.patch74.59 KBWim Leers
#24 2929932-24-do-not-test.patch26.3 KBWim Leers
#24 2929932-24.patch73.94 KBWim Leers
#23 2929932-23.patch26.3 KBWim Leers
#23 interdiff.txt692 bytesWim Leers
#22 2929932-22.patch26.35 KBWim Leers
#22 interdiff.txt1.05 KBWim Leers
#21 2929932-21.patch26.03 KBWim Leers
#20 2929932-20.patch26.02 KBWim Leers
#20 interdiff.txt4.69 KBWim Leers
#15 2929932-15.patch25.1 KBWim Leers
#15 interdiff.txt17 KBWim Leers
#13 2929932-13.patch8.15 KBWim Leers
#13 interdiff.txt8.09 KBWim Leers
#9 2929932-2.patch5.46 KBWim Leers
#2 2929932-2.patch5.46 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Wim Leers’s picture

This should have caused test failures, but JSON API doesn't yet have test coverage akin to https://wimleers.com/blog/api-first-drupal-really and therefore doesn't notice these BC breaks…

(JSON API does have \Drupal\Tests\jsonapi\Functional\JsonApiFunctionalTest, which only tests functionality and not normalizations. That is great btw! But we also need tests for JSON API's normalizations, so that we're notified of BC breaks.)

Wim Leers’s picture

Issue tags: +Needs tests
Wim Leers’s picture

#2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API is fixing not only @DataType=timestamp (which fixes @FieldType=timestamp), but also @DataType=datetime_iso8601, which fixes both @FieldType=datetime and @FieldType=daterange.

This patch is currently only adding

+++ b/jsonapi.services.yml
@@ -143,3 +143,11 @@ services:
+  serializer.normalizer.timestamp.jsonapi:
+    class: \Drupal\jsonapi\ForwardCompatibility\Normalizer\TimestampNormalizer

To fix #2870609: [PP-1] Core Date field is serialized as String instead of timestamp/long, we should also add

+  serializer.normalizer.datetime.jsonapi:
+    class: \Drupal\jsonapi\ForwardCompatibility\Normalizer\DateTimeNormalizer

e0ipso’s picture

I worry about making this work for everyone regardless of their core version. It seems to me that we already have a workaround for this in JSON API Extras.

Do you think it's advisable to #donothing until #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API lands? That way we'll avoid much work for almost no gain (since we effectively have a workaround already).

Wim Leers’s picture

Interesting point!

  • If we do want to move JSON API into core, then we can't say also install JSON API Extras to get the expected behavior.
  • On the other hand, your'e right that we can totally wait on #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API. There's one important downside though: more JSON API consumers will contain code using timestamps-as-ints. By doing this ASAP, we'll have more JSON API consumers that can transition painlessly when #2926508 happens.
  • Finally, quoting the IS: and JSON API can require the Drupal 8 minor in which it was introduced, — even if #2926508 landed today and Drupal 8.5.0 shipped tomorrow, then it could still be many months until people update to Drupal 8.5 in practice, which further stresses the previous bullet.

So … I totally see your point, and I'd love to do less work, but I fear this may be a necessity if we want to minimize BC pains for sites using JSON API :(

e0ipso’s picture

Priority: Critical » Major

There's one important downside though: more JSON API consumers will contain code using timestamps-as-ints.

That's the reality of it. We either provide a BC layer now, or later. In any case we'll need to support timestamps-as-ints even with the current number of consumers. Honestly, I'm good either way.

Wim Leers’s picture

I wrote this in #3:

This should have caused test failures, but JSON API doesn't yet have test coverage akin to https://wimleers.com/blog/api-first-drupal-really and therefore doesn't notice these BC breaks…

But #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only changed all that! Re-uploading the patch from #2 to see how it does now :)

Status: Needs review » Needs work

The last submitted patch, 9: 2929932-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

Issue tags: -Needs tests

There we go — integration tests!

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

#2 was based on #2926508-31: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API. I'm first digging into #2926508 again, whose latest comment is by now number 64… Stay tuned!

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
8.09 KB
8.15 KB

#2/#9 was based on the patch in comment 31 of #2926508.

Rerolled to be based on the latest & greatest from that issue, which incorporated @mpdonadio's feedback!

Status: Needs review » Needs work

The last submitted patch, 13: 2929932-13.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
17 KB
25.1 KB

This will make the testGetIndividual() test coverage pass.

… but …

The PATCH test coverage will still fail though, due to a pre-existing bug deep in JSON API: JSON API apparently does not denormalize the @DataType-level data it receives! 😱 You'd think that #2950486: Test coverage: custom modules' format-agnostic @FieldType and @DataType-level normalizers behavior in JSON API proves this is actually does work, but you'd be mistaken: that only handles normalization, not denormalization. You can easily observe this yourself by running NodeTest::testPatchIndividual() and putting a breakpoint in \Drupal\jsonapi\Normalizer\EntityNormalizer::prepareInput().

\Drupal\jsonapi\Normalizer\FieldItemNormalizer::normalize() calls core's @serializer service's ::normalize() method on a field item's properties. And \Drupal\jsonapi\Normalizer\FieldItemNormalizer::denormalize() simply throws an exception.

This means that in the current (and all prior) versions of JSON API, any @DataType-level normalization only works for reading, not for writing … 😰

Wim Leers’s picture

Title: Work around core's ill-designed @FieldType-level TimestampItemNormalizer normalization until #2926508 lands » [PP-1] Work around core's ill-designed @FieldType-level TimestampItemNormalizer normalization until #2926508 lands
Assigned: Wim Leers » Unassigned
Status: Needs review » Postponed
Related issues: +#2955615: Field properties are not being denormalized

Opened a new critical bug report for #15: #2955615: Field properties are not being denormalized.

Wim Leers’s picture

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

See #2955615-6: Field properties are not being denormalized. That's a hard blocker for this issue. I'm working on this cluster of issues, so assigning this to myself.

Wim Leers’s picture

Version: 8.x-1.x-dev » 8.x-2.x-dev
Issue tags: +BC break, +Needs change record

This actually belongs in 8.x-2.x, because it'll change the normalization of timestamp fields (e.g. the created and changed fields on Node entities). When core fixed this in #2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX, there was an update path. But because JSON API never complied with that, and hence never respected that BC flag, it's not reasonable to suddenly start supporting it, a year after core shipped with it.

Wim Leers’s picture

See #2926508-114: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API through #2926508-116: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API — the patch in #2926508 has been updated to be agnostic, so that it also works for JSON API.

#13 and #15 were based on a patch from ~2 months ago. This should still fail though, because #2955615: Field properties are not being denormalized hasn't been fixed yet. Once there is a patch there, simply rebasing this patch on top of that should make tests pass!

Wim Leers’s picture

Wim Leers’s picture

#2926508-115: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API also makes this change:

+++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/Timestamp.php
@@ -26,7 +26,7 @@ class Timestamp extends IntegerData implements DateTimeInterface {
-    if ($this->value) {
+    if (isset($this->value)) {

But this patch cannot port that. But it can implement a work-around at the normalizer level until #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API lands. So let's do that.

Wim Leers’s picture

Fix CS violation.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Postponed » Needs review
FileSize
73.94 KB
26.3 KB

This is #23, rebased on top of #2955615-18: Field properties are not being denormalized. If all goes well, this should be green. For the reasons outlined in #15.

(The do-not-test patch is identical to #23's patch. Once #2955615 lands, the do-not-test patch should apply cleanly and should pass tests.)

Wim Leers’s picture

That failed unexpectedly. Turns out it's because #2942561: Assert denormalizing the JSON API response results in the expected object got committed between the time I rolled #2955615-18: Field properties are not being denormalized and #24! Yay for better test coverage uncovering problems before they are committed :)

Fixed that over at #2955615-22: Field properties are not being denormalized.

So this is again #23, rebased on top of #2955615-22: Field properties are not being denormalized.

Wim Leers’s picture

Actually, the 64 failures in #24 are identical to the ones in #2955615-20: Field properties are not being denormalized. So … if #2955615-21: Field properties are not being denormalized is green, this is green. And that just came back green!

So, I'm going to predict this:

Yay! That passed as expected 🎉

🤞


IMHO this is RTBC.

Wim Leers’s picture

My prediction came true :)

gabesullice’s picture

Status: Needs review » Reviewed & tested by the community

IMHO this is RTBC.

I concur :P nice work!

Wim Leers’s picture

Note this is RTBC against 8.x-2.x, so let's hold off committing this a bit longer; until we decide that the 8.x-1.x branch receives only minimal maintenance. That'll keep things simpler.

gabesullice’s picture

Issue tags: +Needs reroll
Wim Leers’s picture

The last submitted patch, 31: 2929932-31.patch, failed testing. View results

Wim Leers’s picture

A small fix is enough to make it pass tests again :)

Wim Leers’s picture

Title: [PP-1] Work around core's ill-designed @FieldType-level TimestampItemNormalizer normalization until #2926508 lands » Work around core's ill-designed @FieldType-level TimestampItemNormalizer normalization until #2926508 lands

#2955615: Field properties are not being denormalized landed! Testing #33's do-not-test patch now, since it should apply cleanly to HEAD at last.

Wim Leers’s picture

Wim Leers’s picture

Came back green! Let's do this! 🚀;

Wim Leers’s picture

Issue summary: View changes

  • Wim Leers committed c034235 on 8.x-2.x
    Issue #2929932 by Wim Leers, e0ipso, gabesullice: Work around core's ill...
Wim Leers’s picture

Status: Reviewed & tested by the community » Fixed
Wim Leers’s picture

  1. +++ b/tests/src/Functional/FeedTest.php
    @@ -127,18 +124,14 @@ class FeedTest extends ResourceTestBase {
    -          'modified' => 123456789,
    +          'modified' => '1973-11-29T21:33:09+00:00',
               // @todo uncomment this in https://www.drupal.org/project/jsonapi/issues/2929932
               /* 'modified' => $this->formatExpectedTimestampItemValues(123456789), */
    

    Rather than updating the existing entry, I should've uncommented the todo.

  2. +++ b/tests/src/Functional/NodeTest.php
    --- a/tests/src/Functional/ResourceTestBase.php
    +++ b/tests/src/Functional/ResourceTestBase.php
    

    There was a big todo in this class, which I forgot to address.

Creating a follow-up…

Wim Leers’s picture

  • gabesullice committed 6f7f58c on 8.x-2.x
    Issue #2985321 by Wim Leers: Follow-up for #2929932: two @todos left for...

Status: Fixed » Closed (fixed)

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