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
Patch.Test coverage.Review.
User interface changes
None.
API changes
See CR.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#33 | 2929932-33-do-not-test.patch | 27.23 KB | Wim Leers |
| |||
#33 | 2929932-33.patch | 73.99 KB | Wim Leers |
| |||
#33 | interdiff.txt | 976 bytes | Wim Leers |
#31 | 2929932-31-do-not-test.patch | 26.31 KB | Wim Leers |
#31 | 2929932-31.patch | 73.03 KB | Wim Leers |
Comments
Comment #2
Wim LeersBased on #2926508-31: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API.
Comment #3
Wim LeersThis 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.)Comment #4
Wim LeersComment #5
Wim Leers#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
To fix #2870609: [PP-1] Core Date field is serialized as String instead of timestamp/long, we should also add
Comment #6
e0ipsoI 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).
Comment #7
Wim LeersInteresting point!
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 :(
Comment #8
e0ipsoThat'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.
Comment #9
Wim LeersI wrote this in #3:
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 :)
Comment #11
Wim LeersThere we go — integration tests!
Comment #12
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!
Comment #13
Wim Leers#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!
Comment #15
Wim LeersThis 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 runningNodeTest::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 … 😰Comment #16
Wim LeersOpened a new critical bug report for #15: #2955615: Field properties are not being denormalized.
Comment #17
Wim Leers#2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API is RTBC again. I'll be working on this again tomorrow.
Comment #18
Wim LeersSee #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.
Comment #19
Wim LeersThis actually belongs in 8.x-2.x, because it'll change the normalization of
timestamp
fields (e.g. thecreated
andchanged
fields onNode
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.Comment #20
Wim LeersSee #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!
Comment #21
Wim LeersRebased.
Comment #22
Wim Leers#2926508-115: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API also makes this change:
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.
Comment #23
Wim LeersFix CS violation.
Comment #24
Wim LeersThis 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.)
Comment #25
Wim LeersThat 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.
Comment #26
Wim LeersActually, 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:
🤞
IMHO this is RTBC.
Comment #27
Wim LeersMy prediction came true :)
Comment #28
gabesulliceI concur :P nice work!
Comment #29
Wim LeersNote 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.
Comment #30
gabesulliceComment #31
Wim LeersRebased #25 on top of #2955615-33: Field properties are not being denormalized.
Comment #33
Wim LeersA small fix is enough to make it pass tests again :)
Comment #34
Wim Leers#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.
Comment #35
Wim LeersCR created: https://www.drupal.org/node/2982678.
Comment #36
Wim LeersCame back green! Let's do this! 🚀;
Comment #37
Wim LeersComment #39
Wim LeersComment #40
Wim LeersRather than updating the existing entry, I should've uncommented the todo.
There was a big todo in this class, which I forgot to address.
Creating a follow-up…
Comment #41
Wim LeersCreated #2985321: Follow-up for #2929932: two @todos left for that issue that actually already have been addressed, and updated CR to point to it.