Problem/Motivation
#2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX fixed the normalization of "Time fields" aka the TimestampItem
. But it did so by adding a normalizer for the @FieldType
level. It could have been implemented just as well at the @DataType
level, but this simply didn't cross anybody's mind AFAICT. At least it wasn't discussed on the issue. (And I was very active on it too, so my bad!)
IOW: not a normalizer with
use Drupal\Core\Field\Plugin\Field\FieldType\TimestampItem;
…
protected $supportedInterfaceOrClass = TimestampItem::class;
but with
\Drupal\Core\TypedData\Plugin\DataType\Timestamp
…
protected $supportedInterfaceOrClass = Timestamp::class;
The benefit is that the normalizers are then no longer format-specific: we'll need only one and it'll work for both the default normalization (serialization
module: json
+ xml
formats) and the HAL normalization (hal
module: hal_json
format). But also for contrib's jsonapi
normalization/format!
Note: this will also fix #2870609: [PP-1] Core Date field is serialized as String instead of timestamp/long.
Proposed resolution
- Add
serializer.normalizer.timestamp=\Drupal\serialization\Normalizer\TimestampNormalizer
(for@DataType=timestamp
, which is used by@FieldType=timestamp
) + unit tests. - Add
serializer.normalizer.datetimeiso8601=\Drupal\serialization\Normalizer\DateTimeIso8601Normalizer
(for@DataType=datetime_iso8601
, which is used by@FieldType=datetime
and@FieldType=daterange
) + unit tests. - Create a base normalizer
\Drupal\serialization\Normalizer\DateTimeNormalizer
used by the two mentioned above. Plus unit tests. - Deprecate
\Drupal\serialization\Normalizer\TimeStampItemNormalizerTrait
. - Explicit functional denormalization tests, proving that the denormalization logic is called prior to the field value getting set and stored.
- Explicit functional BC tests.
- Explicit deprecation test.
See #72 for a clear overview of the patch.
Remaining tasks
None.
User interface changes
None.
API changes
@FieldType=timestamp
: no changes!@FieldType=datetime
fields configured to store date + time:
- Before
-
'2017-03-01T20:02:00'
Note: timezone information is absent!
- After
-
'2017-03-01T20:02:00+11:00'
Note: the site's timezone is present! This is now a valid RFC3339 datetime string.
Note: backward compatibility is retained: a datetime string without timezone information is still accepted. This is discouraged though, and deprecated: it will be removed for Drupal 9.
@FieldType=datetime
fields configured to store date only:
- Before
-
'2017-03-01T20:02:00'
Note: time information is present despite this being a date-only field!
- After
-
'2017-03-01'
RFC3339 only covers combined date and time representations. For date-only representations, we need to use ISO 8601. There isn't a particular name for this date-only format, so we have to hardcode the format. See https://en.wikipedia.org/wiki/ISO_8601#Calendar_dates.
Data model changes
None.
Release notes snippet
datetime
fields that store both date and time now specify a timezone when normalized, datetime
fields that store only a date no longer expose a (meaningless) time when normalized. daterange
fields behave the same way as datetime
fields, because both field types use the same "data type" under the hood, and hence get the same normalization. Finally, there is now automatic timezone conversion when POST
ing or PATCH
ing datetime
or daterange
fields: the client can specify any timezone, and the necessary transformations to match the site's timezone are applied.
Comment | File | Size | Author |
---|---|---|---|
#197 | 2926508-197.patch | 63.12 KB | Wim Leers |
#197 | interdiff.txt | 2.23 KB | Wim Leers |
#195 | 2926508-195.patch | 62.52 KB | Wim Leers |
#195 | interdiff.txt | 2.09 KB | Wim Leers |
#191 | 2926508-191.patch | 62.56 KB | Wim Leers |
Comments
Comment #2
Wim LeersComment #3
Wim LeersCompleted the issue summary.
Comment #4
Wim Leers#3 was the ideal, based on the discussion in #2860350: Document why JSON API only supports @DataType-level normalizers. Now for reality…
I got
TimestampNormalizer::normalize()
working! And was able to removeTimestampItemNormalizer
altogether!Comment #5
Wim Leers… but I failed to get
TimestampNormalizer::denormalize()
working, becauseFieldItemNormalizer::denormalize()
preventsTimestampNormalizer::denormalize()
from even being called. So the unfortunate reality is that we need to keepTimestampItemNormalizer
, just to be able to haveTimestampNormalizer::denormalize()
get called during the denormalization process.Another unfortunate reality is the fact that the way Typed Data is designed, as a user of Typed Data, you're never supposed to add a
Timestamp
object ot aTimestampItem
object. Typed Data insists on doing that for you. SoTimestampNormalizer::denormalize()
must return an array, not aTimestamp
object. Which is pretty counter-intuitive, but we can't do anything about it.This is where the reality meets, that both the Symfony serialization component and the Typed Data API have design flaws. We'll be able to address those in the future, but not today.
Comment #8
Wim LeersI hadn't dealt with the
hal
normalization at all yet. This interdiff does, and should fix many failures.Comment #10
Wim LeersMost failures are now in the denormalization of HAL. That's for another day.
Comment #11
Wim LeersWhat #5 did for the
serialization
normalization's denormalization support, this does for thehal
normalizationComment #12
Wim LeersFixed an edge case.
Comment #15
Wim LeersThe last remaining failing test is failing due to a bug in the test itself. Created #2927566: Unit test EntityReferenceFieldItemNormalizerTest mocks incorrectly for that. Pulled in the minimal patch that fixes it: #2927566-2: Unit test EntityReferenceFieldItemNormalizerTest mocks incorrectly.
Comment #16
Wim LeersHere's a unit test for
TimestampNormalizer
.Comment #17
Wim LeersNow that we have that unit test, which was able to copy/paste quite a bit from
TimestampItemNormalizerTest
, we need to updateTimestampItemNormalizerTest
: it needs to test less (because much of the responsibility shifted to the @DataType normalizer), but it does need to test that the @DataType normalizer is called as expected.Comment #18
Wim Leers#17 is already a nice clean-up.
But … in working on the above, I noticed that the
\Drupal\Core\TypedData\Plugin\DataType\Timestamp
@DataType plugin has agetDateTime()
method which does something that is suspiciously similar to\Drupal\serialization\Normalizer\TimestampNormalizer::normalize()
.That normalization method does:
But that could simply become:
… which would work for all
\Drupal\Core\TypedData\Type\DateTimeInterface
objects.So, this patch:
DateTimeNormalizer
, which works for allDateTimeInterface
objects (includes tests)TimestampNormalizer
so thatclass TimestampNormalizer extends DateTimeNormalizer {
, i.e. "timestamp" is the more specific variant of "datetime", and hence decorates it (updates tests)With this approach, we'd also solve #2867206: API-First DX of "date" and "date range" fields: add 'datetime_iso8601' @DataType (de)normalizer using RFC3339 timestamps with timezones at the same time!
Comment #19
mpdonadioQuick first look.
Other places allow unix timestamps? Do we want this here?
If $datetime is ever in a timezone other than UTC, that will get reflected in the output in the offset string (still valid output, though). May be worth a comment.
Like the use of \DateTimeImmutable here ; safety against pass-by-reference bugs.
Comment #20
Wim Leers\Drupal\Core\TypedData\Plugin\DataType\Timestamp
. Happy to change that though!Comment #22
Wim LeersComment #23
Wim LeersThe current patch is making
core/modules/hal/src/Normalizer/FieldItemNormalizer.php
andcore/modules/serialization/src/Normalizer/ComplexDataNormalizer.php
a bit iffy, because the patch so far generated the normalization-only'format'
key inTimestampNormalizer
. If we move that back toTimestampItemNormalizer
, we can simplify a lot.(Also smaller patch!)
Comment #24
Wim LeersThis then allows us to close #2867206: API-First DX of "date" and "date range" fields: add 'datetime_iso8601' @DataType (de)normalizer using RFC3339 timestamps with timezones!
\Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDateonlyTest
and\Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDatetimeTest
are already testing thedatetime
field.It looks like all fields using the
DateTimeIso8601
@DataType are happy to allow RFC3339 timestamps, (for example2017-03-01T20:02:00-00:00
, the-00:00
at the end is allowed by RFC3339 but not by ISO8601), it's only the DB layer that's preventing this from happening. IOW:\Drupal\datetime\Plugin\Validation\Constraint\DateTimeFormatConstraintValidator::validate()
is not strict enough yet.(I was trying to verify that ISO8601 is stored, but RFC3339 is returned. It's the same 99.99% of the time, but that 0.01% is of course what we want to test…)
Looking forward to @mpdonadio's feedback :)
Comment #25
mpdonadioFull review later, but...
This is the issue that complicates #24, #2716891: DateTimeIso8601::setDateTime() vs. DateTimeItem::schema(): (at least) one of them is broken.
Not sure if this is strictly accurate? If you try to validate a string w/ an offset it should fail. DateTimeItemTest should cover this, as we added a ton of valid timestamp strings that aren't accepted by storage, and then updated it so that it used FieldKernelTestBase::entityValidateAndSave() would make sure the constraint got called (done in #2824717: Add a format constraint to DateTimeItem to provide REST error message).
Comment #26
Wim LeersBut appending
'2017-03-01T20:02:00-00:00'
is happily accepted, so … AFAICT it doesn't.Comment #28
mpdonadioCan you show a failing test for #26? Really baffled what we are missing in DateTimeItemTest, and don't see an obvious problem in DateTimeFormatConstraintValidator.
Comment #29
dawehnerFor stuff like that it would be nice to explain WHY we put UTC in there. Timezones are hard for everyone dealing with them, so let's make future us happier.
Should we get rid of that?
Can we throw a trigger_error somehow?
We try to move the setExpectedException as near as possible to the actual function which throws it.
Comment #30
mpdonadioThis looks really nice. I need to read this applied, but two things jumped out at me that may reshape this a bit
I am kinda surprised, that this didn't cause a fail (sigh, not really based on coverage I just looked at...). If this change is needed, I think it should be up to the caller to set the TZ of the object they just received because the caller needs it in that TZ.
I'm wondering if a user provides a format, if we really need to limit to our allowed formats. Seems a bit odd to allow the 'format' key and then come back and say that only a few are allowed, which we will try in the input against anyway. Maybe
Comment #31
Wim Leers{value: "<RFC 3339 timestamp>", format: "<RFC 3339 format>"}
normalization forTimestampItem
only as of #23. Which means that also onlyTimestampItem
s are allowed to send aformat
key in the serialized data sent for deserialization. (See #23's comment + interdiff for details.)Comment #32
Wim LeersThis blocks JSON API. For now, adding a work-around in JSON API: #2929932: Work around core's ill-designed @FieldType-level TimestampItemNormalizer normalization until #2926508 lands.
Comment #33
Wim LeersSee #2913941 and specifically #2913941-6: [PP-1] Fields using the DateTimeIso8601 @DataType are missing an explicit time zone: missing a normalizer, which is the JSON API equivalent to core's #2867206: API-First DX of "date" and "date range" fields: add 'datetime_iso8601' @DataType (de)normalizer using RFC3339 timestamps with timezones. If we'd solve this, then we'd also solve it for JSON API.
+
Fortunately, the fix is easy thanks to all the work above :)
This should make
\Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDatetimeTest
fail.Comment #35
Wim Leers#33 fails because:
and
and
and
and
I know what to do about the
field_daterange
/field_datetime
change (add the timezone information that is missing in the current expected value), but I don't know what to do about thefield_dateonly
change. Assigning to @mpdonadio for feedback.Comment #36
Wim LeersHere's the solution for
field_daterange
andfield_datetime
: it's just updating the expected normalization, to include the timezone used in tests (Australia/Sydney
).Unfortunately, the BC test coverage for
bc_timestamp_normalizer_unix
complicates things, but actually that's a good thing, because it caught a regression that would otherwise have gone unnoticed: if we add #33'sserializer.normalizer.datetime
normalizer, then we'd end up not respecting thebc_timestamp_normalizer_unix
flag: "timestamp" fields would no longer get integers (UNIX timestamps) in their normalization, but RFC3339 timestamps, thanks toserializer.normalizer.datetime
, becauseclass Timestamp extends IntegerData implements DateTimeInterface { … }
.The solution: not add
serializer.normalizer.datetime
until 9.0.0, and only addserializer.normalizer.datetimeiso8601
for now. Because until 9.0.0, thebc_timestamp_normalizer_unix
BC layer must be supported.Comment #37
Wim LeersObviously I forgot the patch.
Comment #38
Wim LeersDown to 1 failure, as expected, yay!
What remains now, is the
field_dateonly
case. AFAICT, https://tools.ietf.org/html/rfc3339 doesn't explicitly cover this. But we're definitely not the first to deal with this: 1, 2.According to https://github.com/raml-org/raml-spec/issues/202, RFC3339 is indeed a simplification/clarification of ISO8601, but only for date+time, not for date-only, time-only or datetime-without-timezone-offset values.
Given that, it seems like the
\Drupal\serialization\Normalizer\DateTimeIso8601Normalizer
that I added in the previous comment would need to exist always, because we need it to detect the "date only" setting", and then use ISO8601 normalization instead.Note: ideally we wouldn't have to do this — ideally we'd have a
Date
data type and aDateItem
field type. But that's not the reality just yet.Comment #41
mpdonadioReroll b/c #2927566: Unit test EntityReferenceFieldItemNormalizerTest mocks incorrectly. Just a rebase, and still need to at #35+.
Comment #42
mpdonadioReally wish you could edit and change status...
Comment #43
mpdonadioFriday night random notes. Some of this has been mentioned above, but want to consolidate so we don't miss anything.
Need to revert this and fix where it is called, and it may cause b/c problems.
Needs to move to datetime_range namespace.
Short of getting the earth to stop spinning (out of scope), or contrived shenanigans to convert back and forth to objects, this change is needed.
We should note this in CR.
Like this comment and code. Unfortunately, there really isn't a constant that we can use, but that is the most common form of date-only representations and also aligns with the HTML5 format.
Need to update this to just use what was provided, and not to limit to what we allow (see idea in an above comment).
Re the question in #39, I am fine with normalizing to 'Y-m-d' for date-only. This aligns with the change in #2739290: UTC+12 is broken on date only fields.
Comment #45
Wim LeersIf you'd like to push this issue forward yourself, I'd be happy to switch from writing patches to reviewing patches instead :)
Comment #46
mpdonadioAddressing some of my own feedback. Need a full test run so see where 43-1 pops up.
Comment #48
mpdonadioNeed a full run, but I think 43-1 was b/c TestBot is configured for UTC, but the default TZ for tests is 'Australia/Sydney'. This grabs the value from config, since it is available where we need it.
Comment #50
mpdonadioLooks like we have one real fail in Drupal\Tests\serialization\Kernel\EntitySerializationTest; the rest look like deprecations.
'Australia/Sydney' is set up in the phpunit bootstrap, which KTB will use, but it's not in config for this test. I'm going to just hardcode it here and polish up a few other things. We can address that in a f/up.
Comment #51
mpdonadioComment #52
mpdonadioSeriously confused here. Looking at the interdiff:
OK, this is just a sanity check that the $entity is what we think it is. The first assertion double checks the value. The second checks the computed value, which will be a date object with the timezone set to UTC (see DateTimeComputed::getValue(), line 52 where the TZ is explicit set to UTC. All of this looks OK.
This is a hack for demo purposes; I am not sure how were are passing here. This is our value from storage (implicitly UTC) with the Sydney offset appended on. I think the normalized value would either be '2017-03-01T20:02:00+00:00' (UTC) or '2017-03-02T07:02:00+11:00' (Australia/Sydney, assuming I did the math right in my head).
I think this is because DateTimeIso8601::getDateTime() just creates a DrupalDateTime() from the value and doesn't pass in a TZ argument. If the value string doesn't have a TZ of offset, this it will be picked up from the default, which is Australia/Sydney.
TypedDataTest has coverage, but all of the strings it uses have offsets.
We may have looped back to #2716891: DateTimeIso8601::setDateTime() vs. DateTimeItem::schema(): (at least) one of them is broken.
Comment #54
jhedstromPostponing on #2716891: DateTimeIso8601::setDateTime() vs. DateTimeItem::schema(): (at least) one of them is broken.
Comment #55
Wim LeersSorry for the silence on my side :( End of year holidays and all that.
#46: 👍 looks great!
#48: woah, great detective work!
#52: hm…
So if I understand this correctly, the problem here is that the end result is pretty much "UTC-stored datetime with … the offset stupidly appended"? i.e. the time should have been recalculated, but it isn't. Right?See research below.This led to me digging very deep. And now I'm not so sure about my "looks great!" for #46 :P
Are you sure that #46's reverting of #43.1 is correct? What
DrupalDateTime::createFromTimestamp($this->value, 'UTC');
really does, is forcing theUTC
timezone to be used for the formatted output. i.e. the following pretty complex call chain:\Drupal\Core\Datetime\DrupalDateTime::createFromTimestamp(TZ=X)
\Drupal\Core\Datetime\DrupalDateTime::prepareTimezone::createFromTimestamp(TZ=X)
\Drupal\Core\Datetime\DrupalDateTime::__construct(TZ=X)
\Drupal\Component\Datetime\DateTimePlus::__construct(TZ=X)
\Drupal\Core\Datetime\DrupalDateTime::prepareTimezone(TZ=X)
drupal_get_user_timezone()
). That function returns the user's timezone preference (if that's enabled, and the user chose one), otherwise it uses thesystem.date
config'stimezone.default
setting, and if that is not configured, it falls back tophp.ini
's default timezone.)\Drupal\Component\Datetime\DateTimePlus::prepareTimezone(TZ=X1)
X
, butX1
: the value returned bydrupal_get_user_timezone()
. Therefore, it effectively doesn't change anything anymore — it might as well not have been called.)(What is key here is that
DateTimePlus
is the non-Drupal specific component with simple default timezone behavior, layered on top of PHP's built-in\DateTime
, andDrupalDateTime
layers on top ofDateTimePlus
, adding Drupal-specific bits such as automatically preferring the user's timezone.)IOW: the code questioned in #43.1 (it's great to question it!) was only enforcing that any string representation generated from
\Drupal\Core\TypedData\Plugin\DataType\Timestamp::getDateTime()
would default toUTC
. You could still override it if you wanted to, by calling->setTimezone()
. (Which is what you said in #30.1: ).As of #48, we're condoning the fact that
date_default_timezone_get()
is used, which is identical to$this->config('system.date')->get('timezone.default')
because that's precisely what we pass todate_default_timezone_set()
.(More accurately,
\Drupal\Core\Session\AccountProxy::setAccount()
callsdate_default_timezone_set(drupal_get_user_timezone());
, and it's\drupal_get_user_timezone()
that either uses the user's timezone preference (if that's enabled, and the user chose one), otherwise it uses thesystem.date
config'stimezone.default
setting, and if that is not configured, it falls back tophp.ini
's default timezone.This is why the following change was necessary, in HEAD's test coverage:
This indicates a soft BC break: it means
@FieldType=Timestamp
field data is now getting RFC3339 string representations in the site's timezone rather than UTC. I called it "soft" because in theory clients should use a library that correctly handles timezones. But it's definitely an unnecessary BC break, that still can break some clients, clients that don't do proper timezone handling.Also, the normalizing of UNIX timestamps to RFC3339 strings with a UTC timezone offset was a deliberate decision of #2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX. Which is why the tests were already enforcing it, and the change cited above the previous paragraph is showing that we're changing that decision.
The above explains quite a bit: the reason we want to default to
UTC
for@DataType=Timestamp
data is that we're letting it be transformed to a string in\Drupal\serialization\Normalizer\DateTimeNormalizer::normalize()
. And that normalizer class doesn't care whether the PHP\DateTime
object it receives originates from@DataType=Timestamp
(string representation should be in UTC!) or from@DataType=Date
.So I propose:
@DataType=Timestamp
explicitly default to'UTC'
.BcTimestampNormalizerUnixTestTrait
.@DataType=DateTimeIso8601
, which means that rather than relying on the rather complex call chain (see above) to determine the timezone to use, we explicitly set one. Clarity through explicitness is one reason. No variations between different users requesting the same data is another: if we don't explicitly set the site's timezone, then we'll be returning different normalizations depending on which user requested them: RFC3339 timestamps in different timezones!@DataType=DateTimeIso8601
, which means it will default to the behavior described above, which will default to the user's preferred timezone, otherwise the site's default timezone, otherwise UTC. This is the default Drupal behavior for timezone handling. Or, if we would like to, we could be more explicitI much prefer the explicit one.
In doing that, I also extended the
testWtf()
test case that you added. And in doing that, I found the answer to #52.2! The root cause is that normalization of date time fields happens based onDateTimeItem
'svalue
property (uses@DataType=DateTimeIso8601
), notDateTimeItem
'sdate
property (which is computed using\Drupal\datetime\DateTimeComputed
. Why? Becausevalue
is a non-computed property, anddate
is a computed property. Computed properties aren't exposed by default (see\Drupal\Core\TypedData\DataDefinition::isInternal()
). In this case, it also doesn't make sense to expose the computed property, because this computed property only makes sense in a PHP context: it's a convenience property for PHP developers interacting with datetime fields. So, only thevalue
property is normalized, and that usesDateTimeIso8601
. And per the above explanation, that already implicitly respects the user's preferred timezone. This patch is merely defaulting that to the site's default timezone rather than the user's preferred timezone. But that is why #52.2's "why the hell does this magically work as expected" can be explained :)Therefore this is NOT blocked on #2716891: DateTimeIso8601::setDateTime() vs. DateTimeItem::schema(): (at least) one of them is broken AFAICT.
Comment #57
Wim Leers#55 caused failures in
DateTimeItemTest
andTypedDataTest
. We'll need to check those.What matters most at the moment, is that all REST tests pass :)
But first, feedback on #55 is needed, so moving back to NR.
Comment #58
jhedstromWill this need to be revisited for #2632040: [PP-1] Add ability to select a timezone for datetime field? If so, let's add an
@todo
so it isn't forgotten (or hopefully a test somewhere would catch the incompatibility...)Comment #59
Wim Leers#58: I'd be inclined to say
, but having A) read the IS, B) scanned the patch, the answer is :\Drupal\Core\TypedData\Plugin\DataType\DateTimeIso8601
at all\Drupal\Core\TypedData\Plugin\DataType\DateTimeIso8601::getDateTime()
at all\Drupal\Core\TypedData\Type\DateTimeInterface::getDateTime()
It's all in
DateTimeDefaultFormatter
, where it callssetTimeZone()
. That will still have exactly the same result.Comment #60
mpdonadioRe #55, wow. That is some fantastic insight. I actually had to print that comment out to fully take it in. Still thinking about it.
Here are some Friday night thoughts related to that and maybe #58 to ponder...
DateTime is weird, because the same value (a point in time) can be expressed in different ways. The same timestamp for me, @jhedstrom, and @WimLeers has a different string output when we are at home. Its essentially a localization issue.
So, we are thinking about setting the explicit timezone to UTC (the baseline) "deep" in core in the data type to get consistent output, or deferring that decision until the output is needed, "closer" to the surface. The time we are working with never changes, just how we want to potentially use it. Almost a lazy vs eager evaluation thing. Working with that time (ie, computation) shouldn't matter, just how we express it to a consumer (output in some form).
We have to balance a lot of things here: Field API, REST, and general UX expectations. I think the main think that I am pondering, the soft BC break that is alluded to, is whether we are fixing a REST problem too deep in core. IOW, should we really have to mess with things at the datatype to get the normalizer to work the way we need to?
Part of me says we should keep stuff in UTC, for consistency and sanity, until someone says they need it otherwise. We should compute with it in an TZ agnostic way, but if it gets used somewhere, we at least have a reliable output. The other part of me says that shouldn't matter.
Perhaps, for a better commit history, we need to split out the datatype UTC thing into a separate issue and add test coverage around that, and be explicit that we are doing it for consistency and expectations, not output. Then we finish up this issue, and tackle things like #2632040: [PP-1] Add ability to select a timezone for datetime field knowing a more stable starting point.
?
Comment #61
Wim LeersYou're probably right.
What about this alternative approach? Moves the decision which timezone to use out of the
@DataType
plugin again, into the normalizer. But then at least does that consistently.(In doing so, I ran into very nasty PHPUnit Prophecy limitations 😣)
Comment #63
mpdonadioThis looks nice.
Do you think there is any danger here with objects being passed by reference and $datetime will potentially have a different TZ after the function call? Maybe we need to clone it first?
o_O
Comment #65
Wim LeersTime to get this going again now that the Drupal 8.5 frenzy is over. The good news is that #61 still applies cleanly!
The only failures in #61 were deprecation errors. Assuming there are no new failures, this should make the patch green.
Comment #67
Wim LeersRebased #65 on 8.6 (#65 was on top of 8.5, oops). Slightly different context in one place.
Comment #68
Wim LeersFix CS violations.
Comment #69
Wim LeersRe-read the entire issue. (And like #60, reading my own comment #55 from 2.5 months ago was most helpful, otherwise it'd be nearly impossible to get back into this issue — whew!)
So, in #55, I did a deep dive plus a significant patch reroll based on insights from that deep dive. @mpdonadio made some excellent remarks in #60, which I addressed in #61. Then, @mpdonadio made another set of remarks in #63. Those were not addressed yet. Let's do them now!
#63:
I don't see how there could be any danger, given that
\Drupal\Core\TypedData\Plugin\DataType\DateTimeIso8601::getDateTime()
returns a fresh\Drupal\Core\Datetime\DrupalDateTime
object on every call.DrupalDateTime extends DateTimePlus
, andDateTimePlus
wraps PHP's native\DateTime
. See\Drupal\Component\Datetime\DateTimePlus::__call()
.Comment #70
Wim LeersSelf-review:
I think this can be removed now. Done.
This fixes the normalization of
datetime
fields — the timezone was missing.Before:
'2017-03-01T20:02:00'
.After:
'2017-03-01T20:02:00+11:00'
.Indentation is off here, fixed.
Added. In doing so, I discovered that the validation constraints for
datetime_range
fields are still lacking — probably because there aren't any yet!8.6.0, here and everywhere else.
Typo, plus inconsistent with
\Drupal\hal\Normalizer\TimestampItemNormalizer
. Fixed.DateTimeNormalizerTest
andTimestampNormalizerTest
(for the corresponding classes).But we don't have
DateTimeIso8601NormalizerTest
yet, for@DataType=datetime_iso8601
's "date only" mode! Added. (We already have\Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDateonlyTest
but that's an integration test.)… and in doing so, I realized that
DateTimeIso8601Normalizer
is around to stay, specifically for that "date only" mode. Which means neither the class should be deprecated, nor the service. (The service was deprecated in #65.) So fixed that too.Comment #71
Wim Leers8.6.0, here and everywhere else.
Comment #72
Wim LeersSo let's make this patch more understandable/reviewable.
The key additions!
This is the heart of everything. Because
DateTimeInterface
is the shared interface between@DataType=timestamp
and@DataType=datetime_iso8601
.Because this is the common foundation, this is why it makes sense to also address #2913941: [PP-1] Fields using the DateTimeIso8601 @DataType are missing an explicit time zone: missing a normalizer as part of this issue.
There are two reasons for this subclass to exist for
@DataType=timestamp
: A) to enforce theUTC
timezone to be used, B) to allow additional formats for denormalization, for BC reasons.date-only date fields with
@DataType=datetime_iso8601
values need special treatment. Otherwise, we wouldn't need to do anything for@DataType=datetime_iso8601
: they could just use the genericDateTimeNormalizer
referenced in point 1!Detailed unit test coverage.
Things worth clarifying in test coverage changes
This fixes the normalization of
datetime
fields — the timezone was missing.Before:
'2017-03-01T20:02:00'
.After:
'2017-03-01T20:02:00+11:00'
.This is a sibling for
\Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDatetimeTest
, and tests@FieldType=daterange
.Just a new comment to prevent us from getting confused about this again in the future!
Comment #73
Wim LeersComment #74
Wim LeersAlso rerolled #2929932-13: Work around core's ill-designed @FieldType-level TimestampItemNormalizer normalization until #2926508 lands based on this patch.
Comment #75
Wim LeersUnassigning, I'm done with this for now. Hopefully this can be RTBC'd!
Comment #76
mpdonadioMy hands are a little too dirty on this to RTBC, but I am assigning myself for a review. If anyone wants to do a full review, don't let that stop RTBC.
Comment #77
mpdonadioFew initial thoughts.
Further up on `static $dateString`, I wonder if we should change the comment to say something about how it is in storage format in UTC. I did a double take on this after having not read this patch in a while.
I like the reminder here.
We use noon UTC (12:00:00) on date only fields. Maybe we need to match here and comment?
Comment #78
Wim Leers#76: Great! I honestly don't think it's a problem for you to RTBC this though — it's been a long time since you looked at it, you're one of the least "RTBC trigger-happy" reviewers around, and I've made plenty of changes. Plus, you're one of the very few people who can review this! Of course, it'd be wonderful to also get a review from @jhedstrom :)
#77:
The fact that this doesn't trigger a validation message then points to another problem: we lack a validation constraint that ensures that date-only
DateTimeItem
fields only contain UTC noon values!In doing so, I discovered
\Drupal\datetime\Plugin\Field\FieldType\DateTimeItemInterface::DATE_STORAGE_FORMAT
, which we can use instead ofY-m-d
! So, slightly improved the comment there too :)Comment #80
Wim LeersI forgot to update the test coverage in #78 for point 3.
Comment #81
Wim LeersFixed CS violations.
Comment #82
borisson_I have some nits to pick.
Nitpick: 80 cols, the const should be on the next line.
Nitpick: In most code I've seen this is in one line.
])->save();
Nitpick: 80 cols
Nitpick: 80 cols
Should this
@todo
be here? We could open a new issue and link to it from here or we should probably just remove this.This doesn't seem to match the documentation?
I'm not sure, I couldn't find the docs about this, but it looks like we usually do:
Because this method starts with
test*
this is counted as a test, let's change this toprovideNormalizedDates
.Comment #83
Wim Leers#82:
Comment #84
borisson_All the nitpicks I had were resolved in #83. This looks very solid now.
Comment #85
Wim LeersThank you! I much appreciate your RTBC. But I do think this really needs @mpdonadio's sign-off. Let's give him say 2 weeks. If he still has not reviewed it by then, then we can re-RTBC this.
Comment #86
Wim LeersPinged @mpdonadio gently on Twitter (https://twitter.com/wimleers/status/986598469921013760) since two weeks have passed, and he hasn't been on IRC in the past 2 weeks. Hoping he's okay!
Comment #87
jhedstromI've given #83 another review and think it is RTBC as well.
Comment #88
Wim LeersThanks, @jhedstrom!
@jhedstrom is also a maintainer of the
datetime
module, just like @mpdonadio. @mpdonadio has helped shape this patch, so having @jhedstrom confirm the RTBC of this patch is even better!Moving to RTBC per #84 and #87.
Comment #90
Wim LeersHm …
With this successful RTBC testing history:
Retesting. I wonder what's going on here.
Comment #92
tacituseu CreditAttribution: tacituseu commentedThis was a fun one, turns out it's a combination of timezone changes and intricacies of
DateTime::createFromFormat()
, and will pass or fail depending on the time of the day the tests ran.http://php.net/manual/en/datetime.createfromformat.php
Test snippet (to reproduce set site/user timezone to 'Australia/Sydney' and play with your system time):
Results:
Comment #93
Wim LeersAwesome research, thank you @tacituseu! 🙏
I unfortunately still don't understand … the tests did run at various times of the day, and always passed. I don't see anything materially different in later test runs.
Comment #94
tacituseu CreditAttribution: tacituseu commentedSpun up a test site, steps:
1. 8.6.x core with devel module and issue's patch
2. enable devel and serialization modules
3. change site's timezone under configuration/regional settings and in user's profile to 'Australia/Sydney' (gives best chances)
4. use script from #92 in devels 'Execute PHP Code'
Attached result.
Comment #95
tacituseu CreditAttribution: tacituseu commentedIt will show the symptoms for almost 10 hours since the time I posted it.
Edit: to expand on the why: the time part of
DateTime
object is inherited from system clock, the timezone change from Sydney to UTC will subtract 11 hours from it, so it will push it into previous day if the time part of the result ofcreateFromFormat()
is less than 11:00 o'clock, then it is set to 12:00 of the previous day hence the 24h difference in timestamps.Comment #96
mpdonadioConfirmed.
phpunit modules/serialization/tests/src/Unit/Normalizer/DateTimeIso8601NormalizerTest.php
PHPUnit 6.5.8 by Sebastian Bergmann and contributors.
Testing Drupal\Tests\serialization\Unit\Normalizer\DateTimeIso8601NormalizerTest
.....F
Time: 400 ms, Memory: 8.00MB
There was 1 failure:
1) Drupal\Tests\serialization\Unit\Normalizer\DateTimeIso8601NormalizerTest::testDenormalizeValidFormats with data set "denormalized dates have the site's timezone" ('2016-11-06', DateTimeImmutable Object (...))
Failed asserting that -86400 is identical to 0.
/Users/matt/Documents/PhpStorm/drupal-8.6.x/core/modules/serialization/tests/src/Unit/Normalizer/DateTimeIso8601NormalizerTest.php:164
FAILURES!
Tests: 6, Assertions: 9, Failures: 1.
(2926508-WIP) ~/Documents/PhpStorm/drupal-8.6.x/core$ date
Tue May 15 20:34:08 EDT 2018
Comment #97
mpdonadioThis should fail reliably.
Note that all of the assertions in DateTimeIso8601NormalizerTest::testDenormalizeValidFormats() fail.
Comment #98
mpdonadioAnd per the comment on DateTimeIso8601Normalizer::denormalize() we should be noon UTC and not system, correct?
Comment #101
mpdonadioOk, last fail was b/c #2910883: Move all entity type REST tests to the providing modules. This passes locally and PhpStorm doesn't show a deprecation anymore.
Comment #102
mpdonadioI kinda hate using literals here, but if we use the constants on DateTimeFieldItem, then we are coupling the normalizers to storage, which is wrong. Part of the final review should be that we document all of the UTC/noon/TZ stuff (first pass looks like we did a great job with this) and consider a f/up to keep this mess all in one place.
Comment #103
tacituseu CreditAttribution: tacituseu commentedUnfortunately the problem isn't within the test but the normalizer itself, it will fail on testbot when https://time.is/Sydney is between 00:00 and 09:59 (or 10:59 depending on DST).
One way to solve it would be to change
\DateTime::createFromFormat()
calls inDateTimeNormalizer::denormalize()
to depend on::getNormalizationTimezone()
and then overload it inDateTimeIso8601Normalizer
to return\DateTimeZone('UTC')
.Comment #105
mpdonadioReconfirmed. Have a solution in my head (don't think #103 will be bulletproof). Going to work on this while I still have a window of local fails.
Comment #106
mpdonadioHow about this? Works locally for me, and I am still in the fail window.
Comment #107
tacituseu CreditAttribution: tacituseu commentedA bit 'string-wrangly' but will work ;), I'll retest at various times and manually too to be sure.
Comment #108
tacituseu CreditAttribution: tacituseu commentedWorks fine, just a nitpick:
+ return \DateTime::createFromFormat('Y-m-d\TH:i:s O', $ymd . 'T12:00:00 UTC');
'O'
in format stands for offset in hours while'T'
for timezone abbreviation, so I think it should be the latter (though it seems to work fine either way).And there's one CS violation.
Comment #109
mpdonadioThat's what I get for going by memory. Rethans' book implies that e, P, O, T all get parsed the same. But, updated to 'e' to parallel the IANA name ('T' is the common local abbreviation, which is subject to change and can(?) be ambiguous in the tzdb).
Still want to do a thorough read of this if I can.
Comment #110
Wim LeersI can't thank you enough, @tacituseu and @mpdonadio! 👏 😳🙏
@mpdonadio in #98: woah … that totally makes sense. At this point I'm seriously confused why the patch used to be green for weeks!?
#98 indeed only failed due to a newly added deprecation error, which @mpdonadio noticed and fixed in #101. Yet #101 still has 1 fail. But it's a different fail from #98. @tacituseu explains it in #103. This reminded me to re-read @tacituseu's comment in #92, in which he points to the crazy logic/behavior of PHP's
\DateTime::createFromFormat()
. My mind was blown.I did not expect @mpdonadio's patch solution in #106 — I expected something that uses the
!
character. But I think @mpdonadio's proposal is actually easier to understand than grokking the intricacies of how PHP's date library functions treat the!
character (it's only superficially documented).So +1 for the patch in #106/#109!
Next: a full review.
Comment #111
Wim LeersDidn't mean to mark NW in the previous comment. Sorry.
Per https://www.drupal.org/node/2936397, we should make these
@internal
. Done.That's the only remark I have. Fixed that übernit and re-RTBC'd :)
Comment #112
mpdonadioTBH, I spent about 5min playing with ! and | and didn't get a satisfactory solution, and then decided string mangling + a big comment was better in the long run. Also like the comment where it is and not deeper in, which it is a little out of context from the impact.
Comment #113
Wim Leers#112++
Comment #114
Wim LeersI'm working on the JSON API module; it should be able to reuse these
@DataType
-level (de)normalizers.An explicit goal of the API-First Initiative is to have
@DataType
-level normalizers be reusable across different normalizations/HTTP API modules (see #2926507: Discourage @FieldType-level normalizers, encourage @DataType-level normalizers, to strengthen the API-First ecosystem). Because all the normalization-specific structures happen at the@FieldType
-level and above.The patch that is RTBC here gets extremely close, but it got one tiny detail wrong: the presence of that
['value' => …]
property name in the (de)normalization logic.By simply moving all that
['value' => …]
aka['MAIN_PROPERTY_NAME' => …]
business out of the@DataType
-level normalizers into the@FieldType
-level normalizers, we can get rid of all that complexity in the@DataType
-level normalizers. And … then it also automatically works for JSON API! 🎉 🎊🍻Keeping at RTBC because this is a trivial change. It also makes the logic in the
@DataType
-level normalizers significantly simpler to follow: no more mysterious array-wrapping! So not only does this enable contrib/custom module developers to not need to implement normalizers twice (once for core REST and once for JSON API), it also makes for simpler code!Comment #115
Wim LeersForgot one hunk: #114 updated
serialization
'sTimestampItemNormalizer
, we also need to updatehal
's.Comment #116
Wim Leers+
This is now the ONLY place that has to deal with
['value' => …]
! 🎉🎉🎉(Well, in that class for both
serialization
+hal
.)It makes sense it's only this normalizer that needs to do this/be aware of this, because it's
\Drupal\serialization\Normalizer\TimestampItemNormalizer::normalize()
and\Drupal\hal\Normalizer\FieldItemNormalizer::normalize()
which choose to explicitly expose every property name:Comment #118
Wim LeersGood news, the JSON API issue that is forward-porting this issue to JSON API until this issue lands, is green: #2929932-25: Work around core's ill-designed @FieldType-level TimestampItemNormalizer normalization until #2926508 lands. What that issue does, is forward port
DateTimeNormalizer
DateTimeIso8601Normalizer
TimestampNormalizer
Once this issue lands and JSON API requires Drupal 8.6, the JSON API module will be able to remove those forward-ported normalizers.
I love that the API-First Ecosystem is finally coming together like this!
Comment #119
Wim LeersThis is still true.
Comment #120
Wim Leersd.o was being weird.
Comment #122
tacituseu CreditAttribution: tacituseu commentedUnrelated (
yarn_install
network error).Comment #124
tacituseu CreditAttribution: tacituseu commentedUnrelated (#2975644: Random Failure in Drupal\Tests\Core\Command\QuickStartTest).
Comment #126
effulgentsia CreditAttribution: effulgentsia at Acquia commentedFrom the issue summary:
Looks to me like those would be BC breaks for existing Web API consumers. In the past, for stuff like that, we added
'bc_*'
flags for sites that want to retain the behavior that their existing apps are written against. For example,'bc_primitives_as_strings'
and'bc_timestamp_normalizer_unix'
. Is there a reason why we're not adding a new BC flag for this issue's changes?@tedbow worked on some of the prior issues that added BC flags, so I asked him to take a look at this one and give input. Insight from others is welcome too.
Comment #127
tedbowIt looks like the would not take effect if
bc_timestamp_normalizer_unix
was set to false so set to false. So we are piggybacking off the previous BC flag.So in the first case does BC coverage because if you set
bc_timestamp_normalizer_unix
to false you won't get this.The only thing you can't do is opt out this new behavior but then opt in to the changes in #2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX.
I think this ok because if want timestamps formated in UNIX format you probably want it everywhere.
This normalizer doesn't have BC flag
so you can't opt out of
Which is this case I think is ok because it not about the format really. Here we actually giving you information back that is incorrect in the before T20:02:00 is not valid information. The user was told just a date would be stored.
There is follow up referenced in the normalizer #2958416: Consider making "date only" a separate @DataType which removes this normalizer because it would make date only fields their own field type. But I am not sure about the prospects of that issue.
Is still this the intention?
I see
\Drupal\serialization\Normalizer\TimeStampItemNormalizerTrait
has@deprecated
but
\Drupal\serialization\Normalizer\TimestampItemNormalizer
and\Drupal\hal\Normalizer\TimestampItemNormalizer
do not.Settings to needs work for 3) or maybe it just needs issue summary update?
Comment #128
tedbowLooking back at #72
where @Wim Leers
I think I understand the patch better. Thanks!
We only need to deprecate
TimeStampItemNormalizerTrait
which is no longer used by the 2TimestampItemNormalizer
classes but could be used by other code. This logic has been moved down a level to\Drupal\serialization\Normalizer\DateTimeNormalizer
I am updating the proposed solution in the summary to reflect this.
Setting this back to RTBC since I have addressed my point in #127.3
Interested to see what @effulgentsia thanks of this.
Comment #129
tedbowUnsigning myself would be good to confirm that may changes to the summary in #128 are correct and those normalizers should not be deprecated.
The change notice also doesn't mention deprecation https://www.drupal.org/node/2955581
Comment #130
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIs the API change #3 in the issue summary correct? I'm not able to reproduce the Before result on the branch tip of either 8.5.x or 8.7.x. In which case, is #2 the only actual API change?
Comment #131
effulgentsia CreditAttribution: effulgentsia at Acquia commentedFor API change #2, i.e., this one:
I think we do need a BC flag, because, who knows, an existing client might be already assuming that the timezone is missing and following advice like this. And if they're appending "Z" unconditionally, then doing so with the timezone now present will result in an invalid date error.
In other words, I think we need a
bc_datetime_without_timezone
setting inserialization.settings.yml
.Comment #132
joelstein CreditAttribution: joelstein at On Fire Media commentedThanks for all of the excellent work, everyone! This latest patch is pretty awesome.
I did find one thing, though, which I believe has been touched on throughout this issue, but I believe it's still a problem.
Since datetime strings are stored in the database without the timezone offset (e.g. "2018-09-16T12:00:00"), when they are normalized through DateTimeIso8601::getDateTime(), no timezone information is sent to DrupalDateTime, and thus, the dates are considered to be local (when in fact they are stored in the database as UTC).
Thus, after applying this patch, if I visit a Rest URL for an entity with a date field, I see the wrong timezone offset:
This problem will go away when one (or both) of the following issues are resolved:
In the meantime, we "could" do something simple in DateTimeIso8601::getDateTime() to detect if the string is missing the timezone offset and then use UTC:
Comment #133
mpdonadio#132, need to catch up on this patch, but that also means we have a hole in our test coverage, so we need to add some test cases, too.
Comment #134
joelstein CreditAttribution: joelstein at On Fire Media commentedAnother issue I discovered is that DateTimeIso8601Normalizer::denormalize assumes a 'date' (date only) type, but it needs to also have support for the 'datetime' so that we don't get errors like this for datetime fields:
Comment #135
Wim Leers@joelstein++
Note that Joel originally found this in the JSON API 2.x branch, where this patch was already committed (see #118). He reported this in #2999438: [upstream] Datetime field shown with wrong timezone offset. 🙏 🤘
Indeed. But note that this is also a bug in HEAD.
The problem is that the test coverage in this patch
and in HEAD: HEAD'sActually, no, because in HEAD timezone information is simply missing; it's simply returning whatever is stored in the DB, which is always UTC. Which is one of the bugs this issue is fixing.\Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDatetimeTest
is also not testing this, is also getting it wrong.This change was wrong all along. I didn't notice, nor did any of the
datetime.module
maintainers… which IMHO proves how tricky and complicated we've managed to make things :(Fortunately we have @joelstein looking over our shoulders! ❤️
What puzzles me, is that
DateTimeDefaultFormatter
does work correctly. So what's the difference between that code and the core REST/JSON API normalizers? In both cases, we're transforming TypedData in some way to generate some kind of representation (HTML or JSON).Turns out that
DateTimeDefaultFormatter
has\Drupal\datetime\Plugin\Field\FieldFormatter\DateTimeFormatterBase::viewElements()
which doesThis is iterating over all deltas in a
datetime
field and accessing thedate
computed property.. This means we need to look at\Drupal\datetime\DateTimeComputed::getValue()
and that indeed specifies the storage timezone:Remember,
@FieldType=datetime
has two properties defined:So the fun fact is that the field formatter for
@FieldType=datetime
does NOT look at thevalue
property (which contains the raw stored data); it looks ONLY at thedate
property (which is computed and has the correct timezone handling).This is in sharp contrast with the core REST and contrib JSON API modules' normalizers, which do the exact opposite: they do NOT look at the computed
date
property and ONLY looks at thevalue
property (raw stored data).But the true root cause of the problem discovered and described by @joelstein was already pointed out by Joel, both here in #132 and in #2999438-5: [upstream] Datetime field shown with wrong timezone offset:
\Drupal\Core\TypedData\Plugin\DataType\DateTimeIso8601::getDateTime()
returns an incorrectDrupalDateTime
object.. This bug was introduced in #2002102: Move TypedData primitive types to interfaces, more than 5 years ago: it simply did not take into account timezone handling. Which means we fall back to the default timezone handling. Which means it interprets the given datetime as being in the user's timezone, or if that does not exist the site's timezone, or if that doesn't exist, UTC. This is wrong: this data always needs to be interpreted using UTC.So the solution is pretty simple:
EntityTestDatetimeTest
test coverage to have correct expectations (same forEntityTestDateRangeTest
)\Drupal\Core\TypedData\Plugin\DataType\DateTimeIso8601::getDateTime()
to hardcode theUTC
timezone to ensure theDrupalDateTime
object is actually representative of the stored data (note that this is not incompatible with #2716891: DateTimeIso8601::setDateTime() vs. DateTimeItem::schema(): (at least) one of them is broken: once a timezone offset gets stored, the hardcoded timezone would end up being ignored!)Comment #136
Wim LeersThis updates the test coverage. This should fail.
Comment #137
Wim LeersThis should fix it.
Comment #139
mpdonadioWow. I just read #135 about five times.
O_O
You would think we would have problems all over, but I think the analysis about value vs date is spot on.
We definitely have missing coverage and/or bad assumptions in TypedDataTest and maybe DateTimeItemTest.
I think we may want to split this hunk out, and add some additional coverage, for a cleaner patch on this issue. We can bang it out quickly.
Comment #140
Wim LeersRight, because it's also a bug in Drupal 8.6 (and 8.5). Created #3002164: DateTimeIso8601::getDateTime() does not specify the storage timezone when constructing DrupalDateTime object.
Comment #141
Wim LeersPinged @mpdonadio: https://twitter.com/wimleers/status/1065252796079968256
Comment #142
mpdonadioReading whole patch again. And sorry for the noise that will come; decided to do running commentary on it so we can all see the thought process.
Comment #143
mpdonadioFirst read. Next read will be with the patch applied so I can see all of the tests in situ.
This is here now to make the test pass, but will be removed once other patch lands.
OK, rather than work with the field value and assume system TZ, just hardcode what we expect. We out fancied ourselves here.
Yup.
2017-03-02T07:02:00 is eleven hours after 2017-03-01T20:02:00. Good.
Nice comment. PHP is so awesome.
When we remove the code from the other patch, should also update the deprecations.
No offset specified, so it picked up the +11:00.
Under normal circumstances, math like this may be bad, but with a set date in the past, this is OK.
Comment #144
mpdonadioSuper nit, but string used as the key should be single single quotes.
Attached interdiff between last RTBC patch and this one. That just shows how subtle this problem was.
I think this is good to go, pending
- the other patch getting in, and removing the dup code
- updating the deprecated stuff
- fixing the little string bit ^^
Comment #145
Wim LeersWOOOT! Thanks so much! Agreed on fixing the nit and updating the deprecation notice of course. Can’t wait for that other patch to land!
Comment #146
Wim LeersAs of #135, this was sidetracked by the bug that @joelstein found in #132. We split that off into a separate issue: #3002164: DateTimeIso8601::getDateTime() does not specify the storage timezone when constructing DrupalDateTime object — which landed late last night!
That means this is finally unblocked again 🎉
The last green patch from #137 still applies cleanly, with the exception that the changes made to
DateTimeIso8601
have already landed in #3002164: DateTimeIso8601::getDateTime() does not specify the storage timezone when constructing DrupalDateTime object.DateTime system maintainer @mpdonadio already reviewed this in detail in #143 + #144. He had two nits he wanted fixed, and they're fixed here :)
Comment #147
Wim LeersChange record overhauled: https://www.drupal.org/node/2955581.
Also note that this is a hard blocker for getting JSON:API into core: #2843147: Add JSON:API to core as a stable module. That is why JSON:API already adopted this code more than six months ago (see #118) — and it is thanks to a JSON:API user noticing the timezone issues that we even discovered #3002164: DateTimeIso8601::getDateTime() does not specify the storage timezone when constructing DrupalDateTime object at all! 🤘
This patch represents a big leap forward in making our
@FieldType=timestamp
+@FieldType=datetime
+@FieldType=daterange
handling consistent, by making@DataType=timestamp
+@DataType=datetime_iso8601
consistent. And as a small bonus: with this committed, we'll be able to remove a lot of complexity in Drupal 9 :)Comment #149
Wim LeersAnother bug has been reported by a JSON:API user, thanks to JSON:API having shipped with forward compatibility with this core patch in its 2.x branch.
This time, it's not a long-preexisting bug in Drupal core like #3002164: DateTimeIso8601::getDateTime() does not specify the storage timezone when constructing DrupalDateTime object was. This time, it's a pretty obscure edge case, reported in #3017945: [upstream] Field types with a required property *and* an optional @DataType=datetime_iso8601 property trigger fatal error in DateTimeNormalizer when optional property is empty:
In both of these cases,
$datetime
is a@DataType=datetime_iso8601
instance.In both cases, we invoke the
getDateTime()
method (defined byDateTimeInterface::getDateTime()
) on it to cast it to aDrupalDateTime
object, and then we immediately perform formatting.Unfortunately, there's a bug in there.
That interface method dictates that if there is no value, that
NULL
should be returned instead of aDrupalDateTime
object. And invoking a method onNULL
of course results in a fatal error.I hear you thinking: .
Except it is, because this bug is impossible to reproduce this with an optional
datetime
field (either base or configurable): because those fields would then be considered empty and not be normalized at all. This can only be reproduced with an optionaldatetime_iso8601
property on a non-empty field.. In the case of\Drupal\webform\Plugin\Field\FieldType\WebformEntityReferenceItem
, thetarget_id
property is not empty (because it's required), theopen
andclosed
properties on that same field item are optional.The only way to write functional test coverage for this is to create a custom test-only
@FieldType
similar to@FieldType=webform
. Not even@FieldType=daterange
allows this to happen, because its two@DataType=datetime_iso8601
properties are required.Fortunately, writing unit test coverage is totally possible :)
Comment #151
Wim LeersThis patch is and has been ready for a very long time. If it had already landed without #149, that wouldn't have been a big deal: it'd be a simple fix. Fortunately, JSON:API already adopting this pre-commit means we're getting valuable feedback already, and are finding unhandled edge cases :)
#149 was trivial.
Another JSON:API user reported another less trivial case last week (in #3021194: [upstream] PATCHing DateTime field results in fatal error), that I've only managed to fully understand today. Hang on.
Problem 1 (which masks the root cause):
@DataType
-level denormalizers are never called by the REST/Serialization moduleIn hindsight, this is actually a copout — technically this is right when referring to the field storage layer, but that does not mean storage-level implementation details are okay to expose in our data normalizations to the world via our REST API!
It's an understandable one though: it's because #2957385: FieldItemNormalizer never calls @DataType-level normalizer service' ::denormalize() method hasn't happened yet.
This means that as far as the REST module is concerned, any client is supposed to send in its
PATCH
andPOST
requests the exact expected representation of the properties for every field type.For
@FieldType=text
, that's{value:"<p>Hello World!</p>", format: "basic_html"}
.For
@FieldType=datetime
, that's{value: "2018-12-20T12:16:00"}
… which is not a valid ISO8601 date (the timezone suffix of e.g.+0100
is missing) despite$properties['value'] = DataDefinition::create('datetime_iso8601')
. Why? Because for some reason, these are stored without the timezone and hence the timezone is ommitted in storage (see\Drupal\datetime\Plugin\Field\FieldType\DateTimeItemInterface::DATETIME_STORAGE_FORMAT
).This means we're not just exposing storage-level implementation details (which we're fixing in #2716891: DateTimeIso8601::setDateTime() vs. DateTimeItem::schema(): (at least) one of them is broken!), it even means that the
GET
normalization is different from the normalization one must use whenPATCH
ing orPOST
ing!The root cause for this lack of mapping between the intended normalization and internal storage details is quite simple: #2957385: FieldItemNormalizer never calls @DataType-level normalizer service' ::denormalize() method. #5 already alluded to this, but made the wrong analysis.
The JSON:API module already fixed this in #2955615: Field properties are not being denormalized. Which is why this was reported in #3021194: [upstream] PATCHing DateTime field results in fatal error against JSON:API.
I just posted #2957385-5: FieldItemNormalizer never calls @DataType-level normalizer service' ::denormalize() method to fix this in REST. So far, we don't have any
@DataType
-level denormalizers. This issue is the first to add those.Problem 2 (the root cause):
DateTimeNormalizer
denormalizes into\DateTime
,DateTimeIso8601Normalizer::denormalize()
should map these to strings as expected rather than pass them throughThis issue is making
DateTimeNormalizer
the standardized foundation.class TimestampNormalizer extends DateTimeNormalizer
needs to do barely anything, and the same goes forDateTimeIso8601Normalizer extends DateTimeNormalizer
.But
DateTimeNormalizer::denormalize()
returns a\DateTime
object.TimestampNormalizer::denormalize()
maps this to an integer, as is expected byTimestampItem
'svalue
property (which is a@DataType=timestamp
).Similarly,
DateTimeIso8601Normalizer::denormalize()
should have been mapping this to a string, as is expected byDateTimeItem
'svalue
property (which is a@DataType=datetime_iso8601
. This is what we forgot.Because of problem 1, this was easily missed. I should've noticed it in the unit tests though.
So, updated the unit tests. There's more change than you might expect because
DateTimeIso8601Normalizer
needed to change quite a bit: it was currently only handling the "date-only" case (DateTimeItem::DATETIME_TYPE_DATE
). But it needs to do the cast-\DateTime
-to-string thing for all@DataType=datetime_iso8601
occurrences, so also for the "date+time" case (DateTimeItem::DATETIME_TYPE_DATETIME
).Using one and the same
@DataType
for semantically different types of data does not make sense, that's why #2958416: Consider making "date only" a separate @DataType was opened many months ago. Once that lands, that complexity can go away.(I first thought of moving into
->setSetting()
calls on the properties ofDateTimeItem
, but that means it becomes overridable/configurable, when in reality it is not.)Comment #152
Wim Leers#2957385-5: FieldItemNormalizer never calls @DataType-level normalizer service' ::denormalize() method was almost green, #2957385-7: FieldItemNormalizer never calls @DataType-level normalizer service' ::denormalize() method will be green. Stay tuned for more here.
Comment #154
Wim LeersFirst: fix the one CS violation that #151 introduced and remove a test coverage comment that is no longer accurate.
Comment #155
Wim LeersSecond:
$context['datetime_allowed_formats']
(introduced in #151) provides a superset of what$context['datetime_format']
was enabling before #151. So we can remove$context['datetime_format']
.Comment #156
Wim Leers#2957385: FieldItemNormalizer never calls @DataType-level normalizer service' ::denormalize() method is green and has full test coverage. Which means we can now build on top of it! 🎉
But first let's prove we need it.
That allows us to fix problem #1 here:
Let's remove this incorrect justification comment (see the "copout" bit in #151) and instead send a value that doesn't match the storage implementation details but a value that matches the normalization: one that includes the timezone and is therefore an actually valid RFC3339 timestamp.
I expect this to fail, like so:
(This error is happening in the generic
EntityResourceTestBase::testPost()
test coverage.)Because there is no
@DataType
-level denormalization happening yet. This error message proves that: the storage layer is generating a validation error because the value we're now sending (including the+00:00
suffix) is exactly the one we're trying to send. This proves that the denormalization logic that this issue/patch is adding is not yet being called.#2957385: FieldItemNormalizer never calls @DataType-level normalizer service' ::denormalize() method will fix that.
Comment #158
Wim LeersSo let's apply the latest from #2957385: #2957385-20: FieldItemNormalizer never calls @DataType-level normalizer service' ::denormalize() method. That should fix the problem explained in the previous comment!
But alas, we still run into a failure:
But we're getting quite a bit further! Line 927 instead of line 906 in
EntityResourceTestBase
, which is the line that calls the optionalassertNormalizationEdgeCases()
method.EntityTestDatetimeTest::assertNormalizationEdgeCases()
is checking how Drupal's REST module handles various edge cases: when the input data is not a datetime string, but an array, when the datetime string format is incorrect, when the format is correct the values are invalid, and so on.The error in the test failure now points to the next flaw, which points to another small oversight in the denormalization logic being added in this issue (much like "problem 2" in #151):
DateTimeNormalizer->denormalize()
assumes that it always receives a datetime string, and passes that to PHP's native\DateTime::createFromFormat()
. The denormalization logic needs to check its input more carefully.Comment #159
Wim LeersNow we're failing a bit further in
EntityTestDatetimeTest::assertNormalizationEdgeCases()
:Line 140 versus line 128 previously.
So what's wrong here?
The expected error message is coming from
\Drupal\datetime\Plugin\Validation\Constraint\DateTimeFormatConstraintValidator
, i.e. the storage-level validation constraint. That constraint is still important, to prevent storing invalid data.But now that we can have
@DataType
-level (de)normalization (which as explained in #151), we have invalid denormalization errors before we have invalid field storage value errors. And that's a good thing! Because it means we're no longer writing the incoming data directly to storage: this proves property-level denormalization works.So we update the expected error messages to match that thrown by
\Drupal\serialization\Normalizer\DateTimeNormalizer::denormalize()
instead of that thrown by\Drupal\datetime\Plugin\Validation\Constraint\DateTimeFormatConstraintValidator
.Comment #160
Wim LeersWith the fixes mentioned at the end of #159,
EntityTestDatetimeTest
becomes all green again!But I don't expect
EntityTestDateOnlyTest
will pass tests yet, I expect this:This is thanks to
EntityTestDateonlyTest::assertNormalizationEdgeCases()
explicitly testing a nonsensical date:2017-13-55
.It's surprising that this results in a 201 response instead of a 422 response, isn't it? Before we fix this, let's bring this test coverage to
EntityTestDatetimeTest::assertNormalizationEdgeCases()
andEntityTestDateRangeTest::assertNormalizationEdgeCases()
as well.Comment #162
Wim LeersWe want explicit test coverage for the edge case that is failing in #160 to be tested everywhere. So let's bring it to
EntityTestDatetimeTest::assertNormalizationEdgeCases()
andEntityTestDateRangeTest::assertNormalizationEdgeCases()
as well. IOW: this patch should have more failures than the previous one.Comment #165
Wim LeersSo, why is this happening? Well, thank PHP:
prints:
😱 🙃🙃🙃🙃
See for yourself at https://3v4l.org/VVuaf.
So clearly
\DateTime
has a pretty fascinating way of handling invalid dates! Let's update the denormalization logic to treat any errors or warnings as failures.Comment #166
Wim LeersIf all is well, the above patch will be green! 🎉 ✅
But requiring a valid datetime string instead of
\Drupal\datetime\Plugin\Field\FieldType\DateTimeItemInterface::DATETIME_STORAGE_FORMAT === 'Y-m-d\TH:i:s'
technically is a BC break, since #2824717: Add a format constraint to DateTimeItem to provide REST error message added test coverage for it, which meansPATCH
ing andPOST
ing date+time information this has been working this bizarre way ever since Drupal 8.4. REST clients talking to Drupal 8.4/8.5/8.6 may have made it work this way.So … despite all the work above to make things actually sensible, we'll now need to go back and actually keep BC too. We'll be able to stop supporting this in Drupal 9, but not in 8.7. To guarantee this, let's add an explicit functional test that sends the same values HEAD does.
Comment #167
Wim LeersAnd this then should make it green again! ✅
Includes an explicit deprecation test. 💪
Comment #170
Wim LeersIssue summary updated.
Comment #171
Wim LeersComment #173
Wim LeersSo close!
No failed assertions, it only failed because the functional test is not catching the deprecation error.
Comment #174
Wim LeersPinged @mpdonadio again today: https://twitter.com/wimleers/status/1087434765597401088 (and also pinged @jhedstrom). I previously pinged @mpdonadio 17 days ago: https://twitter.com/wimleers/status/1081304430073602048.
Comment #175
mpdonadioSo much technical debt from not storing the offset and dealing with date+time and date-only in the same field. Combining that with PHP oddities means sadness.
Read comments and interdiff between 137 and 173 (also attached).
+1 from me.
Comment #176
jhedstromThis all looks great and I think ready to go. Just that one tiny question below.
I didn't know it was possible to do this with traits :) That said, I'm curious why it's needed since the trait already defines the method as
protected
?Ha! This is great :)
Comment #177
Wim Leers#175: indeed :(
#176.1: because it only needs that one method from the trait, and PHP requires to specify its visibility. The
use
statement is indeed specifying the same visibility as the one in the trait.#175 & #176: WOW, a double +1 from both component maintainers! Thank you both so much! 🙏 🎉 Moving back to RTBC based on that 😀
#2957385: FieldItemNormalizer never calls @DataType-level normalizer service' ::denormalize() method has been rerolled since #173. Rebased on top of #2957385-36: FieldItemNormalizer never calls @DataType-level normalizer service' ::denormalize() method, which has been RTBC for ~9 days. That still blocks this, so adding
[PP-1]
to the issue title. Boldly keeping this marked instead of to signal that both of these RTBC issues are blockers to #2843147: Add JSON:API to core as a stable module.Comment #178
Wim Leers#2957385: FieldItemNormalizer never calls @DataType-level normalizer service' ::denormalize() method landed! That means this is again unblocked. #177's do-not-test patch applies cleanly. Just rerolling it and testing it explicitly.
Comment #179
mpdonadioStill +1 from me.
Comment #180
Wim LeersThanks :)
Comment #181
larowlanThis looks great, test coverage is thorough - great work
Couple of minor observations
nit: we don't need the else here, because early returns 💪
should we inject the config factory so the dependency is explicit?
do we need
@
here to suppress warnings?could be wrong, but I don't see a deprecation test for this, should be straight forward - NW for that
Comment #182
Wim LeersFALSE
. See https://secure.php.net/manual/en/datetime.createfromformat.php.That's why the following line is the one to get the errors — that's how PHP designed it:
Comment #183
Wim LeersComment #185
Wim Leers#182 had a random fail in
Drupal\Tests\system\FunctionalJavascript\OffCanvasTest
. Retesting.Also: fixing title 😅
Comment #186
Wim LeersPassed tests, back to RTBC, since it's all trivial changes :)
Comment #187
knyshuk.vova CreditAttribution: knyshuk.vova at Internetdevels, Drupal Ukraine Community commentedThe patch looks good and applies successfully. +1 for RTBC.
Comment #188
mradcliffeI tested the patch with consideration for my contrib normalizer, which does use datetime_iso8601 data types directly, but also expects the full date time. For any date (Y-m-d) data types, I'm already using a string and setting the Symfony Date Constraint, which has been great and that should help out #2958416: Consider making "date only" a separate @DataType.
Results:
- I didn't find any regressions to normalized or denormalized data in my module unit tests.
- I didn't find any regressions when I manually inspected/output the data.
Another +1 RTBC :-)
Comment #189
Wim Leers@mradcliffe WONDERFUL! That is super super helpful feedback, thank you so much! 🙏
Comment #190
plachThis looks great! I only have a few nits and one question:
Why static?
What about adding an assertion that we're dealing with a
DateTimeInterface
here?$field_definition
may be null.Why are we unsetting these if
$context
is no longer used afterwards?I'm confused: isn't denormalization supposed to return an instance of the specified class? In this case it would be an instance of
DateTimeInterface
, if I'm understanding the code correctly.Comment #191
Wim Leers\Drupal\Tests\rest\Functional\ResourceTestBase
+\Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase
started doing this years ago :)\Drupal\serialization\Normalizer\FieldItemNormalizer
has a similar if-test, but throws an exception. We should do the same here. Done!$context
is effectively a global in the Symfony Serialization component: anything that gets set at some point continues to exist for the remainder of the normalization process. So we explicitlyunset()
to avoid side effects (to avoid contrib/custom normalizers from relying on this value having been set previously). It's not a hard requirement, this patch can live without it: it's just to avoid having something linger in that$context
"global".then you can see this is returning a
\DateTimeInterface
instance.(Also, yes, there are many confusing aspects about the Symfony serialization component. Happy to explain more things.)
Comment #192
plachI meant
Drupal\Core\TypedData\Type\DateTimeInterface
, sorry for the confusion. Aren't we dealing with the typed data item here?Comment #193
plachHow can we affect the caller code if
$context
is not passed by reference?Comment #194
plachSorry for the spamminess, we're missing
DateTimeNormalizer::normalize()
:)Comment #195
Wim Leers#192: Ah! Yes, that is confusing. But that pattern is not being introduced here. Look at
\Drupal\serialization\Normalizer\FieldItemNormalizer::denormalize()
: rather than it receiving@TypedData
objects, setting those on aFieldItem
object and returning aFieldItem
, it:FieldItem::setValue()
, because that method doesn't accept typed data items. (This is also visible when usingNode::create(['field_name' => …])
: that must always be a primitive or an array of primitives, never an object.)FieldItem
, it receives an existingFieldItem
instance via$context['target_instance']
All of that is super confusing. But due to how Entity/Field/Typed Data API work, normalizers don't have a whole lot of choice.
\Drupal\serialization\Normalizer\DateTimeNormalizer::denormalize()
's overrides (\Drupal\serialization\Normalizer\DateTimeIso8601Normalizer::denormalize()
and\Drupal\serialization\Normalizer\TimestampNormalizer::denormalize()
call the parent implementation to get a\DateTimeInterface
instance and then transform it to the necessary primitive: an integer for@DataType=timestamp
, and a particularly formatted string for@DataType=datetime_iso8601
.#193: ✅— clearly I was wrong on that detail. From working on core REST for years, I've learned to be extremely careful when using
$context
, in this case too careful. (Confirmed my previous explanation was wrong by stepping through the code with a debugger.)#194: ✅
Comment #196
plachThanks for the thorough explanations and updates, this is ready to go as far as I'm concerned!
Comment #197
Wim LeersAdded explicit test for the exception added in #191.
Comment #198
plachRTBC +1, thanks!
Comment #199
mradcliffeI was curious about what @plach mentioned too so I spent the last hour or so looking into it and came up with the same explanation as @Wim Leers.
I also looked back at what I was doing with Typed Data. I'm using offsetSet(), etc... before I get to primitive normalization. So I wouldn't run into any return value issues. My comment still applies that the patch wouldn't affect me. I think since I had to do this way back when in Drupal 8's development cycle, I saw that denormalization wasn't supported by any of the core classes and wrote my own to handle it. If someone wrote their own denormalize method on a Map that went through properties calling all the respective normalizers, then I think this would be a big problem for them.
To expand on what @Wim Leers mentioned in #195,
Our current PHP Unit tests for date time denormalization expect returning non-typed data. We probably don't see it because it's just not used by anything but entity denormalization.
More importantly neither DateTimeNormalizer nor DateTimeIso8601Normalizer support denormalization via serializer so it would be impossible to use those without entities (or invoking the class directly with the correct arguments). When I added a snippet to support it I immediately ran into the InvalidArgumentException because I don't use field items.
I think it's confusing that the return value is there for either so maybe a comment is necessary to help explain as my current self had to go back to figure out why my past self decided to not implement primitive normalization in contrib. back in 2013.
Summarizing:
At the moment no, we don't need to return Typed Data, because it's @internal and the return value isn't used anyway. But it is pretty confusing and I think we need a comment to explain about this.
I also think it would be nice to support Typed Data for DateTimeIso8601Normalizer and TimestampNormalizer, and that those shouldn't depend on field contexts in $context. That would be nice to have them in the @api rather than @internal. It's workaround-able though.
Comment #200
Wim LeersTalked to @mradcliffe. He agreed with improving this documentation in a separate issue, since it's a pre-existing problem. So, created that: #3033361: Document entity+field denormalizer architecture.
Comment #201
mradcliffeAgreed. Back to RTBC + 1.
Comment #202
plachSaving credits
Comment #203
plachCommitted c8dc57e and pushed to 8.7.x. Thanks!
I think it would be good to mention this in the release notes, so please let's add a snippet to the IS.
Comment #205
Wim Leers🥳
Will do!
Comment #206
Wim LeersRelease note snippet added. (This issue was created before that existed in the issue template!)
Also marked #2867206: API-First DX of "date" and "date range" fields: add 'datetime_iso8601' @DataType (de)normalizer using RFC3339 timestamps with timezones as fixed thanks to this :) And it unblocks #2843147: Add JSON:API to core as a stable module, #2847041: Add a format and start/end validation constraints to DateRangeItem to provide helpful REST/JSON:API error message, #2958416: Consider making "date only" a separate @DataType, and #2926507: Discourage @FieldType-level normalizers, encourage @DataType-level normalizers, to strengthen the API-First ecosystem.
Comment #207
plachAwesome work, thanks everyone!
Comment #208
Wim Leers#2843147-131: Add JSON:API to core as a stable module was updated and now the core patch no longer includes a port of this patch! 🥳
Comment #209
Wim LeersComment #210
joelstein CreditAttribution: joelstein at On Fire Media commentedCongrats everyone! Thank you so much for your hard work on this issue.
Comment #211
mondrakeUndesired side effect? #3034015: Class to test deprecation error for deprecated trait triggers deprecation error itself
Comment #212
Wim Leers#210: Thanks :)
#211: Responded there.
Comment #214
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedRe: changes to
@FieldType=datetime
in the issue summary:This isn't strictly true, while before and after this patch, datetimes without timezone information are accepted, the actual timezone handling changed here. Before the patch, these would be accepted and treated as if they were in UTC, after the patch they are accepted and treated as if they were in the sites local timezone.
Just tracking down an interesting bug on a decoupled project that ran into this. For anyone who ran into this and finds the ticket, the fix in my case was a batch updates on entities created after deploying this issue to modify dates by the sites timezone offset and additional append '+0000' to UTC dates sent from the front-end.