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

  1. Add serializer.normalizer.timestamp=\Drupal\serialization\Normalizer\TimestampNormalizer (for @DataType=timestamp, which is used by @FieldType=timestamp) + unit tests.
  2. Add serializer.normalizer.datetimeiso8601=\Drupal\serialization\Normalizer\DateTimeIso8601Normalizer (for @DataType=datetime_iso8601, which is used by @FieldType=datetime and @FieldType=daterange) + unit tests.
  3. Create a base normalizer \Drupal\serialization\Normalizer\DateTimeNormalizer used by the two mentioned above. Plus unit tests.
  4. Deprecate \Drupal\serialization\Normalizer\TimeStampItemNormalizerTrait.
  5. Explicit functional denormalization tests, proving that the denormalization logic is called prior to the field value getting set and stored.
  6. Explicit functional BC tests.
  7. Explicit deprecation test.

See #72 for a clear overview of the patch.

Remaining tasks

None.

User interface changes

None.

API changes

  1. @FieldType=timestamp: no changes!
  2. @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.

  3. @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 POSTing or PATCHing datetime or daterange fields: the client can specify any timezone, and the necessary transformations to match the site's timezone are applied.

CommentFileSizeAuthor
#197 2926508-197.patch63.12 KBWim Leers
#197 interdiff.txt2.23 KBWim Leers
#195 2926508-195.patch62.52 KBWim Leers
#195 interdiff.txt2.09 KBWim Leers
#191 2926508-191.patch62.56 KBWim Leers
#191 interdiff.txt1.64 KBWim Leers
#183 2926508-183.patch62.3 KBWim Leers
#183 interdiff.txt3.01 KBWim Leers
#182 2926508-182.patch60.03 KBWim Leers
#182 interdiff.txt10.57 KBWim Leers
#178 2926508-178.patch59.28 KBWim Leers
#177 2926508-177--includes-2957385-36.patch80.98 KBWim Leers
#177 2926508-177-do-not-test.patch59.28 KBWim Leers
#175 interdiff-137-173.txt26.58 KBmpdonadio
#173 2926508-173--includes-2957385-20.patch80.55 KBWim Leers
#173 2926508-173-do-not-test.patch59.36 KBWim Leers
#173 interdiff.txt989 bytesWim Leers
#167 2926508-167--includes-2957385-20.patch80.24 KBWim Leers
#167 2926508-167-do-not-test.patch59.05 KBWim Leers
#167 interdiff.txt8.1 KBWim Leers
#166 2926508-166--includes-2957385-20.patch78.44 KBWim Leers
#166 2926508-166-do-not-test.patch57.25 KBWim Leers
#166 interdiff.txt1.45 KBWim Leers
#165 2926508-165--includes-2957385-20.patch77.86 KBWim Leers
#165 2926508-165-do-not-test.patch56.66 KBWim Leers
#165 interdiff.txt2.05 KBWim Leers
#162 2926508-162--includes-2957385-20.patch77.08 KBWim Leers
#162 2926508-162-do-not-test.patch55.88 KBWim Leers
#162 interdiff.txt3.05 KBWim Leers
#160 2926508-160--includes-2957385-20.patch75.88 KBWim Leers
#160 2926508-160-do-not-test.patch54.69 KBWim Leers
#160 interdiff.txt3.1 KBWim Leers
#159 2926508-159--includes-2957385-20.patch73.36 KBWim Leers
#159 2926508-159-do-not-test.patch52.16 KBWim Leers
#159 interdiff.txt2.46 KBWim Leers
#158 2926508-158--includes-2957385-20.patch72.96 KBWim Leers
#158 interdiff--2957385-20.txt21.29 KBWim Leers
#156 2926508-156-FAIL.patch51.77 KBWim Leers
#156 interdiff.txt1.88 KBWim Leers
#155 2926508-155.patch51.82 KBWim Leers
#155 interdiff.txt4.52 KBWim Leers
#154 2926508-154.patch52.01 KBWim Leers
#154 interdiff.txt1.36 KBWim Leers
#151 2926508-151.patch52.31 KBWim Leers
#151 interdiff-total.txt11.18 KBWim Leers
#151 interdiff-fixes.txt6.64 KBWim Leers
#151 2926508-151-tests_only_FAIL.patch52.48 KBWim Leers
#151 interdiff-tests.txt4.59 KBWim Leers
#149 2926508-149.patch50.92 KBWim Leers
#149 2926508-149-tests_only_FAIL.patch50.67 KBWim Leers
#149 interdiff-total.txt4.74 KBWim Leers
#149 interdiff-tests.txt2.63 KBWim Leers
#146 2926508-146.patch49.32 KBWim Leers
#146 interdiff.txt2.24 KBWim Leers
#144 interdiff-115-137.txt2.81 KBmpdonadio
#137 2926508-137.patch50.2 KBWim Leers
#137 interdiff.txt965 bytesWim Leers
#136 2926508-136.patch49.32 KBWim Leers
#136 interdiff.txt2.17 KBWim Leers
#132 2926508-132.patch763 bytesjoelstein
#115 2926508-115.patch48.68 KBWim Leers
#115 interdiff.txt783 bytesWim Leers
#114 2926508-114.patch48.66 KBWim Leers
#114 interdiff.txt10.03 KBWim Leers
#111 2926508-111.patch50.81 KBWim Leers
#111 interdiff.txt1.4 KBWim Leers
#109 interdiff-106-109.txt1.37 KBmpdonadio
#109 2926508-109.patch50.86 KBmpdonadio
#106 interdiff-101-106.txt2.72 KBmpdonadio
#106 2926508-106.patch50.86 KBmpdonadio
#101 interdiff-98-101.txt875 bytesmpdonadio
#101 2926508-101.patch50.28 KBmpdonadio
#98 interdiff-83-98.txt1.48 KBmpdonadio
#98 interdiff-97-98.txt1.58 KBmpdonadio
#98 2926508-98.patch50.41 KBmpdonadio
#97 interdiff-83-97.txt974 bytesmpdonadio
#97 2926508-97.patch50.38 KBmpdonadio
#94 timeshift.png42.51 KBtacituseu
#90 Screen Shot 2018-04-28 at 17.13.02.png61.08 KBWim Leers
#83 2926508-83.patch49.19 KBWim Leers
#83 interdiff.txt5.92 KBWim Leers
#81 2926508-81.patch49.21 KBWim Leers
#81 interdiff.txt2.47 KBWim Leers
#80 2926508-80.patch49.46 KBWim Leers
#80 interdiff.txt985 bytesWim Leers
#78 2926508-78.patch49.46 KBWim Leers
#78 interdiff-71-78.txt3.24 KBWim Leers
#71 2926508-71.patch48.27 KBWim Leers
#71 interdiff.txt2.44 KBWim Leers
#70 2926508-70.patch49.41 KBWim Leers
#70 interdiff.txt18.56 KBWim Leers
#68 2926508-68.patch40.18 KBWim Leers
#68 interdiff.txt5.5 KBWim Leers
#67 2926508-67.patch40.36 KBWim Leers
#65 2926508-65.patch40.3 KBWim Leers
#65 interdiff.txt1.93 KBWim Leers
#61 interdiff-55-61.txt8.63 KBWim Leers
#61 2926508-61.patch39.13 KBWim Leers
#55 2926508-55.patch37.47 KBWim Leers
#55 interdiff-51-55.txt5.6 KBWim Leers
#51 interdiff-48-51.txt2.12 KBmpdonadio
#51 2926508-51.patch35.28 KBmpdonadio
#48 interdiff-46-48.txt827 bytesmpdonadio
#48 2926508-48.patch34.74 KBmpdonadio
#46 interdiff-41-46.txt5.93 KBmpdonadio
#46 2926508-46.patch33.93 KBmpdonadio
#41 2926508-41.patch32.86 KBmpdonadio
#38 2926508-38.patch35.26 KBWim Leers
#38 interdiff.txt1.51 KBWim Leers
#37 2926508-36.patch34.54 KBWim Leers
#37 interdiff.txt5.13 KBWim Leers
#33 2926508-33.patch32.44 KBWim Leers
#33 interdiff.txt1.11 KBWim Leers
#31 2926508-31.patch32.09 KBWim Leers
#31 interdiff.txt4.69 KBWim Leers
#24 2926508-24.patch31.63 KBWim Leers
#24 interdiff.txt4.79 KBWim Leers
#23 2926508-23.patch27.88 KBWim Leers
#23 interdiff.txt7.81 KBWim Leers
#22 2926508-22.patch29.7 KBWim Leers
#22 interdiff.txt706 bytesWim Leers
#18 2926508-18.patch29.65 KBWim Leers
#18 interdiff.txt13.1 KBWim Leers
#17 2926508-17.patch23.36 KBWim Leers
#17 interdiff.txt6.92 KBWim Leers
#16 2926508-16.patch18.19 KBWim Leers
#16 interdiff.txt4.33 KBWim Leers
#15 2926508-15.patch13.89 KBWim Leers
#15 interdiff.txt1.55 KBWim Leers
#12 2926508-12.patch12.41 KBWim Leers
#12 interdiff.txt1.83 KBWim Leers
#11 2926508-11.patch12.35 KBWim Leers
#11 interdiff.txt2.55 KBWim Leers
#8 2926508-8.patch12.47 KBWim Leers
#8 interdiff.txt2.68 KBWim Leers
#5 2926508-5.patch9.87 KBWim Leers
#5 interdiff.txt7.23 KBWim Leers
#4 2926508-4.patch5.67 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Issue summary: View changes

Completed the issue summary.

Wim Leers’s picture

Status: Active » Needs review
FileSize
5.67 KB

#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 remove TimestampItemNormalizer altogether!

Wim Leers’s picture

FileSize
7.23 KB
9.87 KB

… but I failed to get TimestampNormalizer::denormalize() working, because FieldItemNormalizer::denormalize() prevents TimestampNormalizer::denormalize() from even being called. So the unfortunate reality is that we need to keep TimestampItemNormalizer, just to be able to have TimestampNormalizer::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 a TimestampItem object. Typed Data insists on doing that for you. So TimestampNormalizer::denormalize() must return an array, not a Timestamp 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.

The last submitted patch, 4: 2926508-4.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 5: 2926508-5.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.68 KB
12.47 KB

I hadn't dealt with the hal normalization at all yet. This interdiff does, and should fix many failures.

Status: Needs review » Needs work

The last submitted patch, 8: 2926508-8.patch, failed testing. View results

Wim Leers’s picture

Most failures are now in the denormalization of HAL. That's for another day.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.55 KB
12.35 KB

What #5 did for the serialization normalization's denormalization support, this does for the hal normalization

Wim Leers’s picture

Fixed an edge case.

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

Status: Needs review » Needs work

The last submitted patch, 12: 2926508-12.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
1.55 KB
13.89 KB

The 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.

Wim Leers’s picture

Here's a unit test for TimestampNormalizer.

Wim Leers’s picture

Now that we have that unit test, which was able to copy/paste quite a bit from TimestampItemNormalizerTest, we need to update TimestampItemNormalizerTest: 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.

Wim Leers’s picture

Title: Add TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON API » Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON API
Related issues: +#2867206: API-First DX of "date" and "date range" fields: add 'datetime_iso8601' @DataType (de)normalizer using RFC3339 timestamps with timezones
FileSize
13.1 KB
29.65 KB

#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 a getDateTime() method which does something that is suspiciously similar to \Drupal\serialization\Normalizer\TimestampNormalizer::normalize().

That normalization method does:

  public function normalize($timestamp, $format = NULL, array $context = []) {
    $date = new \DateTime();
    $date->setTimestamp($timestamp->getValue());
    $date->setTimezone(new \DateTimeZone('UTC'));
    return [
      'value' => $date->format(\DateTime::RFC3339),
…

But that could simply become:

  public function normalize($timestamp, $format = NULL, array $context = []) {
    return [
      'value' => $timestamp->getDateTime()->format(\DateTime::RFC3339),
…

… which would work for all \Drupal\Core\TypedData\Type\DateTimeInterface objects.


So, this patch:

  1. adds DateTimeNormalizer, which works for all DateTimeInterface objects (includes tests)
  2. updates TimestampNormalizer so that class 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!

mpdonadio’s picture

Quick first look.

  1. +++ b/core/modules/serialization/src/Normalizer/DateTimeNormalizer.php
    @@ -0,0 +1,75 @@
    +  protected $allowedFormats = [
    +    'RFC 3339' => \DateTime::RFC3339,
    +    'ISO 8601' => \DateTime::ISO8601,
    +  ];
    

    Other places allow unix timestamps? Do we want this here?

  2. +++ b/core/modules/serialization/src/Normalizer/DateTimeNormalizer.php
    @@ -0,0 +1,75 @@
    +  public function normalize($datetime, $format = NULL, array $context = []) {
    +    return $datetime->getDateTime()->format(\DateTime::RFC3339);
    +  }
    +
    

    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.

  3. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/DateTimeNormalizerTest.php
    @@ -0,0 +1,127 @@
    +    $data['RFC3339'] = ['2016-11-06T09:02:00+00:00', new \DateTimeImmutable('2016-11-06T09:02:00+00:00')];
    +    $data['RFC3339 +0100'] = ['2016-11-06T09:02:00+01:00', new \DateTimeImmutable('2016-11-06T09:02:00+01:00')];
    +    $data['RFC3339 -0600'] = ['2016-11-06T09:02:00-06:00', new \DateTimeImmutable('2016-11-06T09:02:00-06:00')];
    

    Like the use of \DateTimeImmutable here ; safety against pass-by-reference bugs.

Wim Leers’s picture

  1. Because these are "datetime" fields, not timestamp fields. My rationale was that you wouldn't want UNIX timestamps on data types other than \Drupal\Core\TypedData\Plugin\DataType\Timestamp. Happy to change that though!
  2. Well that's part of RFC 3339, right? The whole point is that it encodes the timezone offset. IMHO no extra comment is necessary there.
  3. :)

Status: Needs review » Needs work

The last submitted patch, 18: 2926508-18.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
706 bytes
29.7 KB
Wim Leers’s picture

FileSize
7.81 KB
27.88 KB

The current patch is making core/modules/hal/src/Normalizer/FieldItemNormalizer.php and core/modules/serialization/src/Normalizer/ComplexDataNormalizer.php a bit iffy, because the patch so far generated the normalization-only 'format' key in TimestampNormalizer. If we move that back to TimestampItemNormalizer, we can simplify a lot.

(Also smaller patch!)

Wim Leers’s picture

FileSize
4.79 KB
31.63 KB

This 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 the datetime field.

It looks like all fields using the DateTimeIso8601 @DataType are happy to allow RFC3339 timestamps, (for example 2017-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 :)

mpdonadio’s picture

Full review later, but...

This is the issue that complicates #24, #2716891: DateTimeIso8601::setDateTime() vs. DateTimeItem::schema(): (at least) one of them is broken.

IOW: \Drupal\datetime\Plugin\Validation\Constraint\DateTimeFormatConstraintValidator::validate() is not strict enough yet.

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).

Wim Leers’s picture

If you try to validate a string w/ an offset it should fail

But appending '2017-03-01T20:02:00-00:00' is happily accepted, so … AFAICT it doesn't.

Status: Needs review » Needs work

The last submitted patch, 24: 2926508-24.patch, failed testing. View results

mpdonadio’s picture

Can you show a failing test for #26? Really baffled what we are missing in DateTimeItemTest, and don't see an obvious problem in DateTimeFormatConstraintValidator.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/Timestamp.php
    @@ -26,8 +26,8 @@ class Timestamp extends IntegerData implements DateTimeInterface {
    +    if (isset($this->value)) {
    +      return DrupalDateTime::createFromTimestamp($this->value, 'UTC');
         }
    

    For 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.

  2. +++ b/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDateRangeTest.php
    @@ -0,0 +1,125 @@
    +//    var_dump($x[static::$fieldName]);
    

    Should we get rid of that?

  3. +++ b/core/modules/serialization/src/Normalizer/TimeStampItemNormalizerTrait.php
    @@ -6,6 +6,8 @@
     /**
      * A trait for TimestampItem normalization functionality.
    + *
    + * @deprecated in 8.5.0, use \Drupal\serialization\Normalizer\TimestampNormalizer instead.
      */
     trait TimeStampItemNormalizerTrait {
    

    Can we throw a trigger_error somehow?

  4. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/DateTimeNormalizerTest.php
    @@ -0,0 +1,131 @@
    +    $this->setExpectedException(UnexpectedValueException::class, 'The specified date "2016/11/06 09:02am GMT" is not in an accepted format: "Y-m-d\TH:i:sP" (RFC 3339), "Y-m-d\TH:i:sO" (ISO 8601).');
    ...
    +
    +    $this->normalizer->denormalize($normalized, DateTimeInterface::class, NULL, []);
    

    We try to move the setExpectedException as near as possible to the actual function which throws it.

mpdonadio’s picture

This looks really nice. I need to read this applied, but two things jumped out at me that may reshape this a bit

  1. +++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/Timestamp.php
    @@ -26,8 +26,8 @@ class Timestamp extends IntegerData implements DateTimeInterface {
    -    if ($this->value) {
    -      return DrupalDateTime::createFromTimestamp($this->value);
    +    if (isset($this->value)) {
    +      return DrupalDateTime::createFromTimestamp($this->value, 'UTC');
    

    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.

  2. +++ b/core/modules/serialization/src/Normalizer/DateTimeNormalizer.php
    @@ -0,0 +1,75 @@
    +    // First check for a provided format.
    +    if (!empty($data['format']) && in_array($data['format'], $this->allowedFormats)) {
    +      return \DateTime::createFromFormat($data['format'], $data['value']);
    +    }
    +    // Otherwise, loop through formats.
    

    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

    public function denormalize($data, $class, $format = NULL, array $context = []) {
      // First check for a provided format, and try to create a date object using it.
      if (!empty($data['format'])) {
        $date =  \DateTime::createFromFormat($data['format'], $data['value']);
        if ($date !== FALSE) {
    	  return $date;
        }
        else {
          throw new UnexpectedValueException(sprintf('The specified date "%s" could not not be converted from the format: "%s".', $data['value'], $data['format']));
        }
      }
    
      // Loop through the allowed formats and create a \DateTime from the
      // input data if it matches the defined pattern. Since the formats are
      // unambiguous (i.e., they reference an absolute time with a defined time
      // zone), only one will ever match.
      foreach ($this->allowedFormats as $format) {
        $date = \DateTime::createFromFormat($format, $data['value']);
        if ($date !== FALSE) {
    	  return $date;
        }
      }
    
      $format_strings = [];
    
      foreach ($this->allowedFormats as $label => $format) {
        $format_strings[] = "\"$format\" ($label)";
      }
    
      $formats = implode(', ', $format_strings);
      throw new UnexpectedValueException(sprintf('The specified date "%s" is not in an accepted format: %s.', $data['value'], $formats));
    }
    
Wim Leers’s picture

Status: Needs work » Needs review
FileSize
4.69 KB
32.09 KB
  • #29.1 + #30.1: I had to do this to make tests pass, but I see now that that was wrong. Let's figure out a fix. I haven't fixed it yet in this reroll.
  • #29.2 + #29.3: done.
  • #29.4: I don't understand what you mean here.
  • #30.2: Damn! I totally lost track of that one :( I kept the {value: "<RFC 3339 timestamp>", format: "<RFC 3339 format>"} normalization for TimestampItem only as of #23. Which means that also only TimestampItems are allowed to send a format key in the serialized data sent for deserialization. (See #23's comment + interdiff for details.)
Wim Leers’s picture

Wim Leers’s picture

See #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.

However, the datetime in the Json feed is lacking a timezone identifier, causing problems with relying application like Ember that assume datetimes are in a local timezone.

The field is output as:

"field_test": "2017-10-16T05:00:00"

I would expect something like this:

"field_test": "2017-10-16T05:00:00Z"

Note the ISO 8601 standard, which says that if a timezone identifier is lacking, the time should assumed to be local rather than UTC.

+

Oh, I think I understand. JSON API prints the raw field value, and in this case Drupal stores the date in UTC with no timezone identifier. I think this is a core bug: #2914779: Date field values are stored without timezone identifier

Fortunately, the fix is easy thanks to all the work above :)

This should make \Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDatetimeTest fail.

Status: Needs review » Needs work

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

Wim Leers’s picture

Assigned: Unassigned » mpdonadio

#33 fails because:

HEAD
Array &0 (
    'created' => Array &1 (
        0 => Array &2 (
            'format' => 'Y-m-d\TH:i:sP'
            'value' => '2017-12-12T16:55:10+00:00'
        )
    )
    'field_dateonly' => Array &3 (
        0 => Array &4 (
            'value' => '2017-03-01'
        )
    )

and

Array &0 (
    'created' => Array &1 (
        0 => Array &2 (
            'format' => 'Y-m-d\TH:i:sP'
            'value' => '2017-12-12T16:55:10+00:00'
        )
    )
    'field_datetime' => Array &3 (
        0 => Array &4 (
            'value' => '2017-03-01T20:02:00+11:00'
        )
    )
…

and

Array &0 (
    'created' => Array &1 (
        0 => Array &2 (
            'format' => 'Y-m-d\TH:i:sP'
            'value' => '2017-12-12T16:55:16+00:00'
        )
    )
    'field_daterange' => Array &3 (
        0 => Array &4 (
            'end_value' => '2017-03-01T20:02:00'
            'value' => '2017-03-01T20:02:00'
        )
    )
…
patch
Array &0 (
    'created' => Array &1 (
        0 => Array &2 (
            'format' => 'Y-m-d\TH:i:sP'
            'value' => '2017-12-12T16:55:10+00:00'
        )
    )
    'field_dateonly' => Array &3 (
        0 => Array &4 (
            'value' => '2017-03-01T00:00:00+11:00'
        )
    )
…

and

Array &0 (
    'created' => Array &1 (
        0 => Array &2 (
            'format' => 'Y-m-d\TH:i:sP'
            'value' => '2017-12-12T16:55:10+00:00'
        )
    )
    'field_datetime' => Array &3 (
        0 => Array &4 (
            'value' => '2017-03-01T20:02:00'
        )
    )
…

and

Array &0 (
    'created' => Array &1 (
        0 => Array &2 (
            'format' => 'Y-m-d\TH:i:sP'
            'value' => '2017-12-12T16:55:16+00:00'
        )
    )
    'field_daterange' => Array &3 (
        0 => Array &4 (
            'end_value' => '2017-03-01T20:02:00+11:00'
            'value' => '2017-03-01T20:02:00+11:00'
        )
    )
…

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 the field_dateonly change. Assigning to @mpdonadio for feedback.

Wim Leers’s picture

Status: Needs work » Needs review

Here's the solution for field_daterange and field_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's serializer.normalizer.datetime normalizer, then we'd end up not respecting the bc_timestamp_normalizer_unix flag: "timestamp" fields would no longer get integers (UNIX timestamps) in their normalization, but RFC3339 timestamps, thanks to serializer.normalizer.datetime, because class Timestamp extends IntegerData implements DateTimeInterface { … }.

The solution: not add serializer.normalizer.datetime until 9.0.0, and only add serializer.normalizer.datetimeiso8601 for now. Because until 9.0.0, the bc_timestamp_normalizer_unix BC layer must be supported.

Wim Leers’s picture

FileSize
5.13 KB
34.54 KB

Obviously I forgot the patch.

Wim Leers’s picture

FileSize
1.51 KB
35.26 KB

Down 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 a DateItem field type. But that's not the reality just yet.

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

Status: Needs review » Needs work

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

mpdonadio’s picture

mpdonadio’s picture

Status: Needs work » Needs review

Really wish you could edit and change status...

mpdonadio’s picture

Friday night random notes. Some of this has been mentioned above, but want to consolidate so we don't miss anything.

  1. +++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/Timestamp.php
    @@ -26,8 +26,8 @@ class Timestamp extends IntegerData implements DateTimeInterface {
    -    if ($this->value) {
    -      return DrupalDateTime::createFromTimestamp($this->value);
    +    if (isset($this->value)) {
    +      return DrupalDateTime::createFromTimestamp($this->value, 'UTC');
    

    Need to revert this and fix where it is called, and it may cause b/c problems.

  2. +++ b/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDateRangeTest.php
    @@ -0,0 +1,123 @@
    +class EntityTestDateRangeTest extends EntityTestResourceTestBase {
    +
    

    Needs to move to datetime_range namespace.

  3. +++ b/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDatetimeTest.php
    @@ -90,7 +90,7 @@ protected function getExpectedNormalizedEntity() {
    -          'value' => $this->entity->get(static::$fieldName)->value,
    +          'value' => '2017-03-01T20:02:00+11:00',
    

    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.

  4. +++ b/core/modules/hal/src/Normalizer/TimestampItemNormalizer.php
    @@ -24,8 +27,18 @@ class TimestampItemNormalizer extends FieldItemNormalizer {
    +      // 'format' is not a property on Timestamp objects. This is present to
    +      // assist consumers of this data.
    +      'format' => \DateTime::RFC3339,
    

    We should note this in CR.

  5. +++ b/core/modules/serialization/src/Normalizer/DateTimeIso8601Normalizer.php
    @@ -0,0 +1,35 @@
    +      // RFC3339 only covers combined date and time representations. For
    +      // date-only representations, we need to use ISO 8601.
    +      // @see https://en.wikipedia.org/wiki/ISO_8601#Calendar_dates
    +      return $datetime->getDateTime()->format('Y-m-d');
    

    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.

  6. +++ b/core/modules/serialization/src/Normalizer/DateTimeNormalizer.php
    @@ -0,0 +1,86 @@
    +    if (!empty($context['datetime_format']) && in_array($context['datetime_format'], $this->allowedFormats)) {
    +      return \DateTime::createFromFormat($context['datetime_format'], $data['value']);
    +    }
    +    // Otherwise, loop through formats.
    

    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.

Status: Needs review » Needs work

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

Wim Leers’s picture

If you'd like to push this issue forward yourself, I'd be happy to switch from writing patches to reviewing patches instead :)

mpdonadio’s picture

Status: Needs work » Needs review
Issue tags: +Needs change record
FileSize
33.93 KB
5.93 KB

Addressing some of my own feedback. Need a full test run so see where 43-1 pops up.

Status: Needs review » Needs work

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

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
34.74 KB
827 bytes

Need 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.

Status: Needs review » Needs work

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

mpdonadio’s picture

Looks like we have one real fail in Drupal\Tests\serialization\Kernel\EntitySerializationTest; the rest look like deprecations.

+++ b/core/modules/rest/tests/src/Functional/BcTimestampNormalizerUnixTestTrait.php
@@ -31,7 +31,7 @@ protected function formatExpectedTimestampItemValues($timestamp) {
-    $date->setTimezone(new \DateTimeZone('UTC'));
+    $date->setTimezone(new \DateTimeZone($this->config('system.date')->get('timezone.default')));

'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.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
35.28 KB
2.12 KB
mpdonadio’s picture

Seriously confused here. Looking at the interdiff:

  1. +++ b/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDatetimeTest.php
    @@ -68,6 +68,14 @@ public function setUp() {
    +  public function testWTF() {
    +    $value = $this->entity->field_datetime->value;
    +    $date = $this->entity->field_datetime->date;
    +
    +    $this->assertEquals(static::$dateString, $value);
    +    $this->assertEquals(static::$dateString . '+00:00', $date->format('c'));
    +  }
    

    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.

  2. +++ b/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDatetimeTest.php
    @@ -90,7 +98,7 @@ protected function getExpectedNormalizedEntity() {
    -          'value' => '2017-03-01T20:02:00+11:00',
    +          'value' => static::$dateString . '+11:00',
    

    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.

Status: Needs review » Needs work

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

jhedstrom’s picture

Wim Leers’s picture

Status: Postponed » Needs review
FileSize
5.6 KB
37.47 KB

Sorry for the silence on my side :( End of year holidays and all that.


#46: 👍 looks great!
#48: woah, great detective work!
#52: hm…

  1. 👌
  2. 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 the UTC timezone to be used for the formatted output. i.e. the following pretty complex call chain:

  1. \Drupal\Core\Datetime\DrupalDateTime::createFromTimestamp(TZ=X)
  2. \Drupal\Core\Datetime\DrupalDateTime::prepareTimezone::createFromTimestamp(TZ=X)
  3. \Drupal\Core\Datetime\DrupalDateTime::__construct(TZ=X)
  4. \Drupal\Component\Datetime\DateTimePlus::__construct(TZ=X)
  5. \Drupal\Core\Datetime\DrupalDateTime::prepareTimezone(TZ=X)
  6. (The above defaults to the user's preferred timezone (drupal_get_user_timezone()). That function returns the user's timezone preference (if that's enabled, and the user chose one), otherwise it uses the system.date config's timezone.default setting, and if that is not configured, it falls back to php.ini's default timezone.)
  7. \Drupal\Component\Datetime\DateTimePlus::prepareTimezone(TZ=X1)
  8. (The above does not receive X, but X1: the value returned by drupal_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, and DrupalDateTime layers on top of DateTimePlus, 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 to UTC. You could still override it if you wanted to, by calling ->setTimezone(). (Which is what you said in #30.1: 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.)).

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 to date_default_timezone_set().
(More accurately, \Drupal\Core\Session\AccountProxy::setAccount() calls date_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 the system.date config's timezone.default setting, and if that is not configured, it falls back to php.ini's default timezone.

This is why the following change was necessary, in HEAD's test coverage:

+++ b/core/modules/rest/tests/src/Functional/BcTimestampNormalizerUnixTestTrait.php
@@ -29,9 +29,11 @@ protected function formatExpectedTimestampItemValues($timestamp) {
     // Otherwise, format the date string to the same that
     // \Drupal\serialization\Normalizer\TimestampItemNormalizer will produce.
+    // 'Australia/Sydney' is the default time zone for tests. It is hardcoded
+    // here for test cases where the config isn't available.
     $date = new \DateTime();
     $date->setTimestamp($timestamp);
-    $date->setTimezone(new \DateTimeZone('UTC'));
+    $date->setTimezone(new \DateTimeZone('Australia/Sydney'));

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:

  1. To not break BC (hence reverting #46/#48's changes in this area/bringing back those bits from #18): let @DataType=Timestamp explicitly default to 'UTC'.
  2. To therefore also revert the changes in BcTimestampNormalizerUnixTestTrait.
  3. To either:
    1. explicitly specify behavior — prevents user-specific timezones To do something similar for @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!
    2. implicitly rely on default behavior — allows user-specific timezones To not do something similar for @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 explicit

    I 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 on DateTimeItem's value property (uses @DataType=DateTimeIso8601), not DateTimeItem's date property (which is computed using \Drupal\datetime\DateTimeComputed. Why? Because value is a non-computed property, and date 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 the value property is normalized, and that uses DateTimeIso8601. 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.

Status: Needs review » Needs work

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

Wim Leers’s picture

Status: Needs work » Needs review

#55 caused failures in DateTimeItemTest and TypedDataTest. 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.

jhedstrom’s picture

+++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/DateTimeIso8601.php
@@ -22,11 +22,17 @@ class DateTimeIso8601 extends StringData implements DateTimeInterface {
+      // Ensure that all string representations generated from this value use
+      // the site's default timezone. Otherwise, they may end up being
+      // normalized in the user's preferred timezone.
+      // @see \Drupal\Core\Datetime\DrupalDateTime::prepareTimezone()
+      // @see drupal_get_user_timezone()
+      $default_timezone = \Drupal::config('system.date')->get('timezone.default');
       if (is_array($this->value)) {
-        $datetime = DrupalDateTime::createFromArray($this->value);
+        $datetime = DrupalDateTime::createFromArray($this->value, new \DateTimeZone($default_timezone));

Will 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...)

Wim Leers’s picture

#58: I'd be inclined to say yes, but having A) read the IS, B) scanned the patch, the answer is no:

  • that patch doesn't touch \Drupal\Core\TypedData\Plugin\DataType\DateTimeIso8601 at all
  • that patch doesn't call \Drupal\Core\TypedData\Plugin\DataType\DateTimeIso8601::getDateTime() at all
  • in fact, it doesn't even use \Drupal\Core\TypedData\Type\DateTimeInterface::getDateTime()

It's all in DateTimeDefaultFormatter, where it calls setTimeZone(). That will still have exactly the same result.

mpdonadio’s picture

Re #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.

?

Wim Leers’s picture

IOW, should we really have to mess with things at the datatype to get the normalizer to work the way we need to?

You'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 😣)

Status: Needs review » Needs work

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

mpdonadio’s picture

Assigned: mpdonadio » Unassigned

This looks nice.

  1. +++ b/core/modules/serialization/src/Normalizer/DateTimeNormalizer.php
    @@ -47,7 +47,27 @@ class DateTimeNormalizer extends NormalizerBase implements DenormalizerInterface
    +      ->setTimezone($this->getNormalizationTimezone())
    +      ->format(\DateTime::RFC3339);
    +  }
    

    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?

  2. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/TimestampNormalizerTest.php
    @@ -129,3 +131,14 @@ public function testDenormalizeException() {
    +// Note: Prophecy does not support magic methods. By subclassing and specifying
    +// an explicit method, Prophecy works.
    +// @see https://github.com/phpspec/prophecy/issues/338
    +// @see https://github.com/phpspec/prophecy/issues/34
    +// @see https://github.com/phpspec/prophecy/issues/80
    +class TimestampNormalizerTestDrupalDateTime extends DrupalDateTime {
    +  public function setTimezone(\DateTimeZone $timezone) {
    +    parent::setTimezone($timezone);
    +  }
    +}
    

    o_O

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Needs work » Needs review
FileSize
1.93 KB
40.3 KB

Time 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.

Status: Needs review » Needs work

The last submitted patch, 65: 2926508-65.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
40.36 KB

Rebased #65 on 8.6 (#65 was on top of 8.5, oops). Slightly different context in one place.

Wim Leers’s picture

Fix CS violations.

Wim Leers’s picture

Re-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:

  1. Love your diligence/attention to detail! ❤️ 👏

    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.

  2. Heh … to clarify: this is because DrupalDateTime extends DateTimePlus, and DateTimePlus wraps PHP's native \DateTime. See \Drupal\Component\Datetime\DateTimePlus::__call().
Wim Leers’s picture

Self-review:

  1. +++ b/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDatetimeTest.php
    @@ -68,6 +68,17 @@ public function setUp() {
    +  public function testWTF() {
    

    I think this can be removed now. Done.

  2. +++ b/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDatetimeTest.php
    @@ -90,7 +101,7 @@ protected function getExpectedNormalizedEntity() {
    -          'value' => $this->entity->get(static::$fieldName)->value,
    +          'value' => static::$dateString . '+11:00',
    

    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'.

  3. +++ b/core/modules/datetime_range/tests/src/Functional/EntityResource/EntityTest/EntityTestDateRangeTest.php
    @@ -0,0 +1,123 @@
    +    return parent::getNormalizedPostEntity() + [
    +        static::$fieldName => [
    

    Indentation is off here, fixed.

  4. +++ b/core/modules/datetime_range/tests/src/Functional/EntityResource/EntityTest/EntityTestDateRangeTest.php
    @@ -0,0 +1,123 @@
    +    // @todo
    

    Added. In doing so, I discovered that the validation constraints for datetime_range fields are still lacking — probably because there aren't any yet!

  5. +++ b/core/modules/serialization/src/Normalizer/TimeStampItemNormalizerTrait.php
    @@ -4,8 +4,12 @@
    + * @deprecated in 8.5.0, use \Drupal\serialization\Normalizer\TimestampNormalizer instead.
    

    8.6.0, here and everywhere else.

  6. +++ b/core/modules/serialization/src/Normalizer/TimestampItemNormalizer.php
    @@ -3,15 +3,15 @@
    + * Overrides FieldItemNormalizer to use\Drupal\serialization\Normalizer\TimestampNormalizer
    

    Typo, plus inconsistent with \Drupal\hal\Normalizer\TimestampItemNormalizer. Fixed.

  7. I noticed we have DateTimeNormalizerTest and TimestampNormalizerTest(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.

Wim Leers’s picture

+++ b/core/modules/serialization/src/Normalizer/TimeStampItemNormalizerTrait.php
@@ -4,8 +4,12 @@
+ * @deprecated in 8.5.0, use \Drupal\serialization\Normalizer\TimestampNormalizer instead.

8.6.0, here and everywhere else.

Wim Leers’s picture

So let's make this patch more understandable/reviewable.

The key additions!

  1. +++ b/core/modules/serialization/src/Normalizer/DateTimeNormalizer.php
    @@ -0,0 +1,103 @@
    +class DateTimeNormalizer extends NormalizerBase implements DenormalizerInterface {
    ...
    +  protected $supportedInterfaceOrClass = DateTimeInterface::class;
    

    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.

  2. +++ b/core/modules/serialization/src/Normalizer/TimestampNormalizer.php
    @@ -0,0 +1,55 @@
    +class TimestampNormalizer extends DateTimeNormalizer {
    ...
    +  protected $supportedInterfaceOrClass = Timestamp::class;
    
    +++ b/core/modules/hal/src/Normalizer/TimestampItemNormalizer.php
    @@ -4,15 +4,17 @@
    + * Overrides FieldItemNormalizer to
    + * - during normalization, add the 'format' key to assist consumers
    + * - during denormalization, use \Drupal\serialization\Normalizer\TimestampNormalizer
    
    @@ -24,8 +26,18 @@ class TimestampItemNormalizer extends FieldItemNormalizer {
    +    return parent::normalizedFieldValues($field_item, $format, $context) + [
    +      // 'format' is not a property on Timestamp objects. This is present to
    +      // assist consumers of this data.
    +      'format' => \DateTime::RFC3339,
    +    ];
    +  }
    
    +++ b/core/modules/serialization/src/Normalizer/TimestampItemNormalizer.php
    @@ -22,21 +22,22 @@ class TimestampItemNormalizer extends FieldItemNormalizer {
    +  public function normalize($object, $format = NULL, array $context = []) {
    +    return parent::normalize($object, $format, $context) + [
    +      // 'format' is not a property on Timestamp objects. This is present to
    +      // assist consumers of this data.
    +      'format' => \DateTime::RFC3339,
    +    ];
       }
    

    There are two reasons for this subclass to exist for @DataType=timestamp: A) to enforce the UTC timezone to be used, B) to allow additional formats for denormalization, for BC reasons.

  3. +++ b/core/modules/serialization/src/Normalizer/DateTimeIso8601Normalizer.php
    @@ -0,0 +1,35 @@
    +class DateTimeIso8601Normalizer extends DateTimeNormalizer {
    ...
    +    if ($field_item instanceof DateTimeItem && $field_item->getFieldDefinition()->getFieldStorageDefinition()->getSetting('datetime_type') === DateTimeItem::DATETIME_TYPE_DATE) {
    

    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 generic DateTimeNormalizer referenced in point 1!

  4. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/DateTimeNormalizerTest.php
    @@ -0,0 +1,188 @@
    +class DateTimeNormalizerTest extends UnitTestCase {
    
    +++ b/core/modules/serialization/tests/src/Unit/Normalizer/TimestampNormalizerTest.php
    @@ -0,0 +1,148 @@
    +class TimestampNormalizerTest extends UnitTestCase {
    
    +++ b/core/modules/serialization/tests/src/Unit/Normalizer/TimestampNormalizerTest.php
    @@ -0,0 +1,148 @@
    +class DateTimeIso8601NormalizerTest extends UnitTestCase {
    

    Detailed unit test coverage.

Things worth clarifying in test coverage changes

  1. +++ b/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDatetimeTest.php
    @@ -90,7 +101,7 @@ protected function getExpectedNormalizedEntity() {
    -          'value' => $this->entity->get(static::$fieldName)->value,
    +          'value' => static::$dateString . '+11:00',
    

    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'.

  2. +++ b/core/modules/datetime_range/tests/src/Functional/EntityResource/EntityTest/EntityTestDateRangeTest.php
    @@ -0,0 +1,123 @@
    +class EntityTestDateRangeTest extends EntityTestResourceTestBase {
    ...
    +      'type' => 'daterange',
    

    This is a sibling for \Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDatetimeTest, and tests @FieldType=daterange.

  3. +++ b/core/modules/rest/tests/src/Functional/BcTimestampNormalizerUnixTestTrait.php
    @@ -31,6 +31,8 @@ protected function formatExpectedTimestampItemValues($timestamp) {
    +    // Per \Drupal\Core\TypedData\Plugin\DataType\Timestamp::getDateTime(), they
    +    // default to string representations in the UTC timezone.
         $date->setTimezone(new \DateTimeZone('UTC'));
    

    Just a new comment to prevent us from getting confused about this again in the future!

Wim Leers’s picture

Issue summary: View changes
Issue tags: -Needs change record
  1. IS updated (to point reviewers to #72).
  2. CR created: https://www.drupal.org/node/2955581
Wim Leers’s picture

Wim Leers’s picture

Assigned: Wim Leers » Unassigned

Unassigning, I'm done with this for now. Hopefully this can be RTBC'd!

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

My 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.

mpdonadio’s picture

Few initial thoughts.

  1. +++ b/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDatetimeTest.php
    @@ -90,7 +90,7 @@ protected function getExpectedNormalizedEntity() {
    -          'value' => $this->entity->get(static::$fieldName)->value,
    +          'value' => static::$dateString . '+11:00',
    

    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.

  2. +++ b/core/modules/hal/src/Normalizer/TimestampItemNormalizer.php
    @@ -24,8 +26,21 @@ class TimestampItemNormalizer extends FieldItemNormalizer {
    +      // 'format' is not a property on Timestamp objects. This is present to
    +      // assist consumers of this data.
    +      'format' => \DateTime::RFC3339,
    

    I like the reminder here.

  3. +++ b/core/modules/serialization/src/Normalizer/DateTimeIso8601Normalizer.php
    @@ -0,0 +1,56 @@
    +    $date->setTime(0, 0, 0);
    

    We use noon UTC (12:00:00) on date only fields. Maybe we need to match here and comment?

Wim Leers’s picture

#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:

  1. ✔️ Done.
  2. 🙂
  3. Oh, we definitely should! Done.

    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 of Y-m-d! So, slightly improved the comment there too :)

Status: Needs review » Needs work

The last submitted patch, 78: 2926508-78.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
985 bytes
49.46 KB

I forgot to update the test coverage in #78 for point 3.

Wim Leers’s picture

Fixed CS violations.

borisson_’s picture

I have some nits to pick.

  1. +++ b/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDatetimeTest.php
    @@ -90,7 +90,12 @@ protected function getExpectedNormalizedEntity() {
    +          // format: \Drupal\datetime\Plugin\Field\FieldType\DateTimeItemInterface::DATETIME_STORAGE_FORMAT,
    

    Nitpick: 80 cols, the const should be on the next line.

  2. +++ b/core/modules/datetime_range/tests/src/Functional/EntityResource/EntityTest/EntityTestDateRangeTest.php
    @@ -0,0 +1,156 @@
    +    ])
    +      ->save();
    ...
    +    ])
    +      ->save();
    

    Nitpick: In most code I've seen this is in one line.

    ])->save();

  3. +++ b/core/modules/datetime_range/tests/src/Functional/EntityResource/EntityTest/EntityTestDateRangeTest.php
    @@ -0,0 +1,156 @@
    +      // @todo Test more edge cases in https://www.drupal.org/project/drupal/issues/2847041.
    

    Nitpick: 80 cols

  4. +++ b/core/modules/hal/src/Normalizer/TimestampItemNormalizer.php
    @@ -4,15 +4,17 @@
    + * - during denormalization, use \Drupal\serialization\Normalizer\TimestampNormalizer
    

    Nitpick: 80 cols

  5. +++ b/core/modules/serialization/src/Normalizer/DateTimeIso8601Normalizer.php
    @@ -0,0 +1,62 @@
    + * @todo Consider making "date only" a separate data type in Drupal 9.
    

    Should this @todo be here? We could open a new issue and link to it from here or we should probably just remove this.

  6. +++ b/core/modules/serialization/src/Normalizer/DateTimeIso8601Normalizer.php
    @@ -0,0 +1,62 @@
    +    // For consistency, we set the time to 12:00:00 UTC for date-only fields.
    ...
    +    $date->setTime(0, 0, 0);
    

    This doesn't seem to match the documentation?

  7. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/DateTimeIso8601NormalizerTest.php
    @@ -0,0 +1,208 @@
    +/**
    + * Unit test coverage for the "datetime_iso8601" @DataType.
    + *
    + * @group serialization
    + * @coversDefaultClass \Drupal\serialization\Normalizer\DateTimeIso8601Normalizer
    + * @see \Drupal\Core\TypedData\Plugin\DataType\DateTimeIso8601
    + *
    + * Only tests the "date only" mode of DateTimeIso8601, because everything else
    + * is handled by \Drupal\serialization\Normalizer\DateTimeNormalizer, for which
    + * we have \Drupal\Tests\serialization\Unit\Normalizer\DateTimeNormalizerTest.
    + *
    + * @see \Drupal\datetime\Plugin\Field\FieldType\DateTimeItem::DATETIME_TYPE_DATE
    + */
    

    I'm not sure, I couldn't find the docs about this, but it looks like we usually do:

    /**
     * subject
     * 
     * Extra documentation
     *
     * @coversDefaultClass 
     * @group
     * @see
     */
    
  8. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/DateTimeIso8601NormalizerTest.php
    @@ -0,0 +1,208 @@
    +  public function testNormalizeProvider() {
    

    Because this method starts with test* this is counted as a test, let's change this to provideNormalizedDates.

Wim Leers’s picture

#82:

  1. Well, I can't break this one up, so… keeping it as-is.
  2. ✔️
  3. ✔️
  4. ✔️
  5. ✔️ — Good point, created #2958416: Consider making "date only" a separate @DataType
  6. ✔️ — Wow, EXCELLENT find! Fixed.
  7. ✔️ — You're elevating nitpicking boilerplate stuff to a new level :P
  8. ✔️ — I noticed the same while working on point 6, great find!
borisson_’s picture

Status: Needs review » Reviewed & tested by the community

All the nitpicks I had were resolved in #83. This looks very solid now.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review

Thank 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.

Wim Leers’s picture

Pinged @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!

jhedstrom’s picture

I've given #83 another review and think it is RTBC as well.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, @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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 83: 2926508-83.patch, failed testing. View results

Wim Leers’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
FileSize
61.08 KB

Hm …

1) Drupal\Tests\serialization\Unit\Normalizer\DateTimeIso8601NormalizerTest::testDenormalizeValidFormats with data set "denormalized dates have the site's timezone" ('2016-11-06', DateTimeImmutable Object (...))

With this successful RTBC testing history:

Retesting. I wonder what's going on here.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 83: 2926508-83.patch, failed testing. View results

tacituseu’s picture

This 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

If format does not contain the character ! then portions of the generated time which are not specified in format will be set to the current system time.

If format contains the character !, then portions of the generated time not provided in format, as well as values to the left-hand side of the !, will be set to corresponding values from the Unix epoch.

The Unix epoch is 1970-01-01 00:00:00 UTC.

If timezone is omitted and time contains no timezone, the current timezone will be used.

Test snippet (to reproduce set site/user timezone to 'Australia/Sydney' and play with your system time):

use Drupal\Core\TypedData\Plugin\DataType\DateTimeIso8601;
use Drupal\serialization\Normalizer\DateTimeIso8601Normalizer;

$value = '2016-11-06'; $expected = new \DateTimeImmutable('2016-11-06T12:00:00+00:00');

// 1. confirmation
// from DateTimeIso8601NormalizerTest::testDenormalizeValidFormats($value, $expected)
$normalizer = new DateTimeIso8601Normalizer();
$normalized = ['value' => $value];

$denormalized = $normalizer->denormalize($normalized, DateTimeIso8601::class, NULL, []);
dpm($denormalized);
dpm($expected);
dpm($denormalized->getTimestamp());
dpm($expected->getTimestamp());
dpm($denormalized->getTimestamp()-$expected->getTimestamp());

// 2. figure out exactly why $normalizer->denormalize() does that
$allowedFormats = ['date-only' => 'Y-m-d'];
// from Drupal\serialization\Normalizer\DateTimeNormalizer::denormalize()
$data = $normalized;
$date = \DateTime::createFromFormat($allowedFormats['date-only'], $data['value']);
dpm($date);

// from Drupal\serialization\Normalizer\DateTimeIso8601Normalizer::denormalize()
$date->setTimezone(new \DateTimeZone('UTC'));
dpm($date);

$date->setTime(12, 0, 0);
dpm($date);

Results:

// 1.
DateTime @1478347200 {#498 ?
  date: 2016-11-05 12:00:00.0 UTC (+00:00)
}
DateTimeImmutable @1478433600 {#270 ?
  date: 2016-11-06 12:00:00.0 +00:00
}
1478347200
1478433600
-86400

// 2.
DateTime @1478362563 {#524 ?
  date: 2016-11-06 03:16:03.0 Australia/Sydney (+11:00)
}
DateTime @1478362563 {#524 ?
  date: 2016-11-05 16:16:03.0 UTC (+00:00)
}
DateTime @1478347200 {#524 ?
  date: 2016-11-05 12:00:00.0 UTC (+00:00)
}
Wim Leers’s picture

Awesome 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.

tacituseu’s picture

FileSize
42.51 KB

Spun 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.

tacituseu’s picture

It 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 of createFromFormat() is less than 11:00 o'clock, then it is set to 12:00 of the previous day hence the 24h difference in timestamps.

mpdonadio’s picture

Confirmed.

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

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
50.38 KB
974 bytes

This should fail reliably.

Note that all of the assertions in DateTimeIso8601NormalizerTest::testDenormalizeValidFormats() fail.

mpdonadio’s picture

And per the comment on DateTimeIso8601Normalizer::denormalize() we should be noon UTC and not system, correct?

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

Status: Needs review » Needs work

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

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
50.28 KB
875 bytes

Ok, 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.

mpdonadio’s picture

+++ b/core/modules/serialization/tests/src/Unit/Normalizer/DateTimeIso8601NormalizerTest.php
@@ -0,0 +1,208 @@
+    $this->assertEquals($expected->format(\DateTime::RFC3339), $denormalized->format(\DateTime::RFC3339));
+    $this->assertEquals('UTC', $denormalized->getTimezone()->getName());
+    $this->assertEquals('12:00:00', $denormalized->format('H:i:s'));

I 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.

tacituseu’s picture

Unfortunately 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 in DateTimeNormalizer::denormalize() to depend on ::getNormalizationTimezone() and then overload it in DateTimeIso8601Normalizer to return \DateTimeZone('UTC').

Status: Needs review » Needs work

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

mpdonadio’s picture

Reconfirmed. 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.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
50.86 KB
2.72 KB

How about this? Works locally for me, and I am still in the fail window.

tacituseu’s picture

A bit 'string-wrangly' but will work ;), I'll retest at various times and manually too to be sure.

tacituseu’s picture

Works 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.

mpdonadio’s picture

That'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.

Wim Leers’s picture

Status: Needs review » Needs work

I 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.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.4 KB
50.81 KB

Didn't mean to mark NW in the previous comment. Sorry.

+++ b/core/modules/serialization/src/Normalizer/DateTimeIso8601Normalizer.php

+++ b/core/modules/serialization/src/Normalizer/TimestampNormalizer.php

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 :)

mpdonadio’s picture

Assigned: mpdonadio » Unassigned

TBH, 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.

Wim Leers’s picture

#112++

Wim Leers’s picture

I'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!

Wim Leers’s picture

Forgot one hunk: #114 updated serialization's TimestampItemNormalizer, we also need to update hal's.

Wim Leers’s picture

+++ b/core/modules/serialization/src/Normalizer/TimestampItemNormalizer.php
@@ -41,7 +41,7 @@ protected function constructValue($data, $context) {
-    return $this->serializer->denormalize($data, Timestamp::class, NULL, $context);
+    return ['value' => $this->serializer->denormalize($data['value'], Timestamp::class, NULL, $context)];

+

+++ b/core/modules/hal/src/Normalizer/TimestampItemNormalizer.php
@@ -41,7 +41,7 @@ protected function constructValue($data, $context) {
-    return $this->serializer->denormalize($data, Timestamp::class, NULL, $context);
+    return ['value' => $this->serializer->denormalize($data['value'], Timestamp::class, NULL, $context)];

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:

    // We normalize each individual property, so each can do their own casting,
    // if needed.
    /** @var \Drupal\Core\TypedData\TypedDataInterface $property */
    foreach (TypedDataInternalPropertiesHelper::getNonInternalProperties($field_item) as $property_name => $property) {
      $normalized[$property_name] = $this->serializer->normalize($property, $format, $context);
    }

The last submitted patch, 114: 2926508-114.patch, failed testing. View results

Wim Leers’s picture

Good 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

  1. DateTimeNormalizer
  2. DateTimeIso8601Normalizer
  3. 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!

Wim Leers’s picture

Assigned: Unassigned » mpdonadio

Keeping at RTBC because this is a trivial change.

This is still true.

Wim Leers’s picture

Assigned: mpdonadio » Unassigned

d.o was being weird.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 115: 2926508-115.patch, failed testing. View results

tacituseu’s picture

Status: Needs work » Reviewed & tested by the community

Unrelated (yarn_install network error).

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 115: 2926508-115.patch, failed testing. View results

tacituseu’s picture

Status: Needs work » Reviewed & tested by the community

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

effulgentsia’s picture

Assigned: Unassigned » tedbow
Status: Reviewed & tested by the community » Needs review

From the issue summary:

@FieldType=datetime fields configured to store date + time:
Before
'2017-03-01T20:02:00'
After
'2017-03-01T20:02:00+11:00'

@FieldType=datetime fields configured to store date only:
Before
'2017-03-01T20:02:00'
After
'2017-03-01'

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.

tedbow’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/Timestamp.php
    --- a/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDatetimeTest.php
    +++ b/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDatetimeTest.php
    
    +++ b/core/modules/serialization/serialization.services.yml
    @@ -58,6 +58,16 @@ services:
    +  serializer.normalizer.timestamp:
    +    class: Drupal\serialization\Normalizer\TimestampNormalizer
    +    tags:
    +      # Priority must be higher than serializer.normalizer.primitive_data.
    +      - { name: normalizer, priority: 20, bc: bc_timestamp_normalizer_unix, bc_config_name: 'serialization.settings' }
    

    It 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.

  2. +++ b/core/modules/serialization/serialization.services.yml
    @@ -58,6 +58,16 @@ services:
    +  serializer.normalizer.datetimeiso8601:
    +    class: \Drupal\serialization\Normalizer\DateTimeIso8601Normalizer
    +    tags:
    +      # Priority must be higher than serializer.normalizer.primitive_data.
    +      - { name: normalizer, priority: 20 }
    

    This normalizer doesn't have BC flag

    so you can't opt out of

    @FieldType=datetime fields configured to store date only:
    Before
    '2017-03-01T20:02:00'
    After
    '2017-03-01'
    

    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.

  3. from the "proposed solution"

    Deprecate \Drupal\serialization\Normalizer\TimestampItemNormalizer
    Deprecate \Drupal\serialization\Normalizer\TimeStampItemNormalizerTrait
    Deprecate \Drupal\hal\Normalizer\TimestampItemNormalizer
    Deprecate serializer.normalizer.timestamp_item
    Deprecate serializer.normalizer.timestamp_item.hal

    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?

tedbow’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

Looking back at #72
where @Wim Leers

So let's make this patch more understandable/reviewable.

I think I understand the patch better. Thanks!

We only need to deprecate TimeStampItemNormalizerTrait which is no longer used by the 2 TimestampItemNormalizer 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.

tedbow’s picture

Assigned: tedbow » Unassigned

Unsigning 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

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review

Is 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?

effulgentsia’s picture

Status: Needs review » Needs work

For API change #2, i.e., this one:

+++ b/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDatetimeTest.php
@@ -90,7 +90,12 @@ protected function getExpectedNormalizedEntity() {
-          'value' => $this->entity->get(static::$fieldName)->value,
+          // static::$dateString is in the format specified by the DB storage
+          // format: \Drupal\datetime\Plugin\Field\FieldType\DateTimeItemInterface::DATETIME_STORAGE_FORMAT,
+          // which happens to be RFC3339 minus the timezone information.
+          // But the normalized value is an RFC3339 datetime string, and so we
+          // must still append the timezone information.
+          'value' => static::$dateString . '+11:00',

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 in serialization.settings.yml.

joelstein’s picture

FileSize
763 bytes

Thanks 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:

/entity/node/1?_format=json
expected = "2018-09-16T07:00:00-05:00"
actual = "2018-09-16T12:00:00-05:00"

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:

public function getDateTime() {
  if ($this->value) {
    if (is_array($this->value)) {
      $datetime = DrupalDateTime::createFromArray($this->value);
    }
    else {
      // Set timezone to UTC if no timezone info is present.
      $timezone = strlen($this->value) === 19 ? 'UTC' : NULL;
      $datetime = new DrupalDateTime($this->value, $timezone);
    }
    return $datetime;
  }
}
mpdonadio’s picture

#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.

joelstein’s picture

Another 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:

"The specified date "2018-09-19T01:00:00-05:00" is not in an accepted format: "Y-m-d" (date-only)."

Wim Leers’s picture

@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. 🙏 🤘

#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.

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's \Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDatetimeTest is also not testing this, is also getting it wrong. Actually, 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.

+++ b/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDatetimeTest.php
@@ -90,7 +90,12 @@ protected function getExpectedNormalizedEntity() {
-          'value' => $this->entity->get(static::$fieldName)->value,
+          // static::$dateString is in the format specified by the DB storage
+          // format: \Drupal\datetime\Plugin\Field\FieldType\DateTimeItemInterface::DATETIME_STORAGE_FORMAT,
+          // which happens to be RFC3339 minus the timezone information.
+          // But the normalized value is an RFC3339 datetime string, and so we
+          // must still append the timezone information.
+          'value' => static::$dateString . '+11:00',

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 does

    foreach ($items as $delta => $item) {
      if ($item->date) {
        /** @var \Drupal\Core\Datetime\DrupalDateTime $date */
        $date = $item->date;
        $elements[$delta] = $this->buildDateWithIsoAttribute($date);

This is iterating over all deltas in a datetime field and accessing the date computed property.. This means we need to look at \Drupal\datetime\DateTimeComputed::getValue() and that indeed specifies the storage timezone:

$date = DrupalDateTime::createFromFormat($storage_format, $value, DateTimeItemInterface::STORAGE_TIMEZONE);

Remember, @FieldType=datetime has two properties defined:

  public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
    $properties['value'] = DataDefinition::create('datetime_iso8601')
      ->setLabel(t('Date value'))
      ->setRequired(TRUE);

    $properties['date'] = DataDefinition::create('any')
      ->setLabel(t('Computed date'))
      ->setDescription(t('The computed DateTime object.'))
      ->setComputed(TRUE)
      ->setClass('\Drupal\datetime\DateTimeComputed')
      ->setSetting('date source', 'value');

    return $properties;
  }

So the fun fact is that the field formatter for @FieldType=datetime does NOT look at the value property (which contains the raw stored data); it looks ONLY at the date 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 the value 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 incorrect DrupalDateTime 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:

  1. update the EntityTestDatetimeTest test coverage to have correct expectations (same for EntityTestDateRangeTest)
  2. modify \Drupal\Core\TypedData\Plugin\DataType\DateTimeIso8601::getDateTime() to hardcode the UTC timezone to ensure the DrupalDateTime 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!)
Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.17 KB
49.32 KB

This updates the test coverage. This should fail.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
FileSize
965 bytes
50.2 KB

This should fix it.

The last submitted patch, 136: 2926508-136.patch, failed testing. View results

mpdonadio’s picture

Wow. I just read #135 about five times.

+++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/DateTimeIso8601.php
@@ -23,10 +23,10 @@ class DateTimeIso8601 extends StringData implements DateTimeInterface {
   public function getDateTime() {
     if ($this->value) {
       if (is_array($this->value)) {
-        $datetime = DrupalDateTime::createFromArray($this->value);
+        $datetime = DrupalDateTime::createFromArray($this->value, 'UTC');
       }
       else {
-        $datetime = new DrupalDateTime($this->value);
+        $datetime = new DrupalDateTime($this->value, 'UTC');
       }
       return $datetime;
     }
 

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.

Wim Leers’s picture

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.

Right, 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.

Wim Leers’s picture

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Reading 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.

mpdonadio’s picture

First read. Next read will be with the patch applied so I can see all of the tests in situ.

  1. +++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/DateTimeIso8601.php
    @@ -23,10 +23,10 @@ class DateTimeIso8601 extends StringData implements DateTimeInterface {
    -        $datetime = DrupalDateTime::createFromArray($this->value);
    +        $datetime = DrupalDateTime::createFromArray($this->value, 'UTC');
           }
           else {
    -        $datetime = new DrupalDateTime($this->value);
    +        $datetime = new DrupalDateTime($this->value, 'UTC');
           }
    

    This is here now to make the test pass, but will be removed once other patch lands.

  2. +++ b/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDatetimeTest.php
    @@ -90,7 +90,7 @@ protected function getExpectedNormalizedEntity() {
    -          'value' => $this->entity->get(static::$fieldName)->value,
    +          'value' => '2017-03-02T07:02:00+11:00',
    

    OK, rather than work with the field value and assume system TZ, just hardcode what we expect. We out fancied ourselves here.

  3. +++ b/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDatetimeTest.php
    @@ -103,6 +103,8 @@ protected function getNormalizedPostEntity() {
    +          // Omitting the timezone is allowed, this should result in the site's
    +          // timezone being used automatically.
               'value' => static::$dateString,
             ],
    

    Yup.

  4. +++ b/core/modules/datetime_range/tests/src/Functional/EntityResource/EntityTest/EntityTestDateRangeTest.php
    @@ -0,0 +1,154 @@
    +  protected function getExpectedNormalizedEntity() {
    +    return parent::getExpectedNormalizedEntity() + [
    +      static::$fieldName => [
    +        [
    +          'value' => '2017-03-02T07:02:00+11:00',
    +          'end_value' => '2017-03-02T07:02:00+11:00',
    +        ],
    +      ],
    +    ];
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function getNormalizedPostEntity() {
    +    return parent::getNormalizedPostEntity() + [
    +      static::$fieldName => [
    +        [
    +          'value' => '2017-03-01T20:02:00',
    +          'end_value' => '2017-03-01T20:02:00',
    +        ],
    +      ],
    +    ];
    +  }
    +
    

    2017-03-02T07:02:00 is eleven hours after 2017-03-01T20:02:00. Good.

  5. +++ b/core/modules/serialization/src/Normalizer/DateTimeIso8601Normalizer.php
    @@ -0,0 +1,70 @@
    +    // Rebuild the date object using the extracted year, month, and day, but
    +    // for consistency set the time to 12:00:00 UTC upon creation for date-only
    +    // fields. Rebuilding, instead of using the object methods, is done to
    +    // avoid the initial date object picking up the local time and time zone
    +    // from an input value with a missing or partial time string, and then
    +    // rolling over to a different day when changing the object to UTC.
    +    // @see \Drupal\Component\Datetime\DateTimePlus::setDefaultDateTime()
    +    // @see \Drupal\datetime\Plugin\views\filter\Date::getOffset()
    +    // @see \Drupal\datetime\DateTimeComputed::getValue()
    +    // @see http://php.net/manual/en/datetime.createfromformat.php
    +    return \DateTime::createFromFormat('Y-m-d\TH:i:s e', $ymd . 'T12:00:00 UTC');
    +  }
    

    Nice comment. PHP is so awesome.

  6. +++ b/core/modules/serialization/src/Normalizer/TimeStampItemNormalizerTrait.php
    @@ -4,8 +4,12 @@
    +@trigger_error(__NAMESPACE__ . '\TimeStampItemNormalizerTrait is deprecated in Drupal 8.6.0 and will be removed in Drupal 9.0.0. Use \Drupal\serialization\Normalizer\TimestampNormalizer instead.', E_USER_DEPRECATED);
    +
     /**
    

    When we remove the code from the other patch, should also update the deprecations.

  7. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/DateTimeNormalizerTest.php
    @@ -0,0 +1,184 @@
    +    $data['Y/m/d H:i:s'] = ['09:02:00 2016/11/06', 'H:i:s Y/m/d', new \DateTimeImmutable('2016-11-06T09:02:00+11:00')];
    

    No offset specified, so it picked up the +11:00.

  8. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/TimestampNormalizerTest.php
    @@ -0,0 +1,146 @@
    +    $data['RFC3339'] = ['2016-11-06T09:02:00+00:00', $expected_stamp];
    +    $data['RFC3339 +0100'] = ['2016-11-06T09:02:00+01:00', $expected_stamp - 1 * 3600];
    +    $data['RFC3339 -0600'] = ['2016-11-06T09:02:00-06:00', $expected_stamp + 6 * 3600];
    +
    +    $data['ISO8601'] = ['2016-11-06T09:02:00+0000', $expected_stamp];
    +    $data['ISO8601 +0100'] = ['2016-11-06T09:02:00+0100', $expected_stamp - 1 * 3600];
    +    $data['ISO8601 -0600'] = ['2016-11-06T09:02:00-0600', $expected_stamp + 6 * 3600];
    

    Under normal circumstances, math like this may be bad, but with a set date in the past, this is OK.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
FileSize
2.81 KB
+++ b/core/modules/serialization/tests/src/Unit/Normalizer/DateTimeIso8601NormalizerTest.php
@@ -0,0 +1,207 @@
+    $data["denormalized dates have the UTC timezone"] = ['2016-11-06', new \DateTimeImmutable('2016-11-06T12:00:00', new \DateTimeZone('UTC'))];

Super 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 ^^

Wim Leers’s picture

Title: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON API » [PP-1] Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON API
Status: Needs review » Postponed

WOOOT! Thanks so much! Agreed on fixing the nit and updating the deprecation notice of course. Can’t wait for that other patch to land!

Wim Leers’s picture

Title: [PP-1] Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON API » Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON API
Status: Postponed » Reviewed & tested by the community
FileSize
2.24 KB
49.32 KB

As 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 :)

Wim Leers’s picture

Change 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 :)

The last submitted patch, 115: 2926508-115.patch, failed testing. View results

Wim Leers’s picture

Another 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:

+++ b/core/modules/serialization/src/Normalizer/DateTimeIso8601Normalizer.php
@@ -0,0 +1,70 @@
+      return $datetime->getDateTime()->format($this->allowedFormats['date-only']);

+++ b/core/modules/serialization/src/Normalizer/DateTimeNormalizer.php
@@ -0,0 +1,103 @@
+    return $datetime->getDateTime()
…
+      ->format(\DateTime::RFC3339);

In both of these cases, $datetime is a @DataType=datetime_iso8601 instance.

In both cases, we invoke the getDateTime() method (defined by DateTimeInterface::getDateTime()) on it to cast it to a DrupalDateTime 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 a DrupalDateTime object. And invoking a method on NULL of course results in a fatal error.


I hear you thinking: that doesn't seem obscure at all!.

Except it is, because this bug is impossible to reproduce this with an optional datetimefield (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 optional datetime_iso8601property on a non-empty field.. In the case of \Drupal\webform\Plugin\Field\FieldType\WebformEntityReferenceItem, the target_id property is not empty (because it's required), the open and closed 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 :)

The last submitted patch, 149: 2926508-149-tests_only_FAIL.patch, failed testing. View results

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.59 KB
52.48 KB
6.64 KB
11.18 KB
52.31 KB

This 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 module

+++ b/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDatetimeTest.php
@@ -103,6 +103,8 @@ protected function getNormalizedPostEntity() {
+          // Omitting the timezone is allowed, this should result in the site's
+          // timezone being used automatically.
           'value' => static::$dateString,

In 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 and POST 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 when PATCHing or POSTing!

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 through

This issue is making DateTimeNormalizer the standardized foundation. class TimestampNormalizer extends DateTimeNormalizer needs to do barely anything, and the same goes for DateTimeIso8601Normalizer extends DateTimeNormalizer.

But DateTimeNormalizer::denormalize() returns a \DateTime object. TimestampNormalizer::denormalize() maps this to an integer, as is expected by TimestampItem's value property (which is a @DataType=timestamp).

Similarly, DateTimeIso8601Normalizer::denormalize() should have been mapping this to a string, as is expected by DateTimeItem's value 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 of DateTimeItem, but that means it becomes overridable/configurable, when in reality it is not.)

Wim Leers’s picture

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

Wim Leers’s picture

First: fix the one CS violation that #151 introduced and remove a test coverage comment that is no longer accurate.

Wim Leers’s picture

Second:

+++ b/core/modules/serialization/src/Normalizer/DateTimeNormalizer.php
@@ -87,7 +78,10 @@ public function denormalize($data, $class, $format = NULL, array $context = [])
-    foreach ($this->allowedFormats as $format) {
+    $allowed_formats = isset($context['datetime_allowed_formats'])
+      ? $context['datetime_allowed_formats']
+      : $this->allowedFormats;
+    foreach ($allowed_formats as $format) {

$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'].

Wim Leers’s picture

#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:

+++ b/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDatetimeTest.php
@@ -103,6 +103,8 @@ protected function getNormalizedPostEntity() {
+          // Omitting the timezone is allowed, this should result in the site's
+          // timezone being used automatically.
           'value' => static::$dateString,

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:

1) Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDatetimeTest::testPost
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'{"message":"Unprocessable Entity: validation failed.\nname: Name: this field cannot hold more than 1 values.\n"}'
+'{"message":"Unprocessable Entity: validation failed.\nname: Name: this field cannot hold more than 1 values.\nfield_datetime.0: The datetime value \u00272017-03-01T20:02:00+00:00\u0027 is invalid for the format \u0027Y-m-d\\TH:i:s\u0027\n"}'

/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/ResourceTestBase.php:393
/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/ResourceTestBase.php:455
/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:906

(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.

Status: Needs review » Needs work

The last submitted patch, 156: 2926508-156-FAIL.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
21.29 KB
72.96 KB

So 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:

1) Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDatetimeTest::testPost
Exception: Warning: DateTime::createFromFormat() expects parameter 2 to be string, array given
Drupal\serialization\Normalizer\DateTimeNormalizer->denormalize()() (Line: 79)


/Users/wim.leers/Work/d8/core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php:51
/Users/wim.leers/Work/d8/vendor/guzzlehttp/promises/src/Promise.php:203
/Users/wim.leers/Work/d8/vendor/guzzlehttp/promises/src/Promise.php:156
/Users/wim.leers/Work/d8/vendor/guzzlehttp/promises/src/TaskQueue.php:47
/Users/wim.leers/Work/d8/vendor/guzzlehttp/promises/src/Promise.php:246
/Users/wim.leers/Work/d8/vendor/guzzlehttp/promises/src/Promise.php:223
/Users/wim.leers/Work/d8/vendor/guzzlehttp/promises/src/Promise.php:267
/Users/wim.leers/Work/d8/vendor/guzzlehttp/promises/src/Promise.php:225
/Users/wim.leers/Work/d8/vendor/guzzlehttp/promises/src/Promise.php:62
/Users/wim.leers/Work/d8/vendor/guzzlehttp/guzzle/src/Client.php:131
/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/ResourceTestBase.php:352
/Users/wim.leers/Work/d8/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDatetimeTest.php:128
/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:927

But we're getting quite a bit further! Line 927 instead of line 906 in EntityResourceTestBase, which is the line that calls the optional assertNormalizationEdgeCases() 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.

Wim Leers’s picture

Now we're failing a bit further in EntityTestDatetimeTest::assertNormalizationEdgeCases():

1) Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDatetimeTest::testPost
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'{"message":"Unprocessable Entity: validation failed.\nfield_datetime.0: The datetime value \u00272017-03-01\u0027 is invalid for the format \u0027Y-m-d\\TH:i:s\u0027\n"}'
+'{"message":"The specified date \u00222017-03-01\u0022 is not in an accepted format: \u0022Y-m-d\\TH:i:sP\u0022 (RFC 3339), \u0022Y-m-d\\TH:i:sO\u0022 (ISO 8601)."}'

/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/ResourceTestBase.php:393
/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/ResourceTestBase.php:455
/Users/wim.leers/Work/d8/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDatetimeTest.php:140
/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:927

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.

Wim Leers’s picture

With 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:

1) Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDateonlyTest::testPost
Failed asserting that 201 is identical to 422.

/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/ResourceTestBase.php:381
/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/ResourceTestBase.php:455
/Users/wim.leers/Work/d8/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDateonlyTest.php:150
/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:927
/Users/wim.leers/Work/d8/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDateonlyTest.php:155

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() and EntityTestDateRangeTest::assertNormalizationEdgeCases() as well.

The last submitted patch, 158: 2926508-158--includes-2957385-20.patch, failed testing. View results

Wim Leers’s picture

We 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() and EntityTestDateRangeTest::assertNormalizationEdgeCases() as well. IOW: this patch should have more failures than the previous one.

The last submitted patch, 160: 2926508-160--includes-2957385-20.patch, failed testing. View results

The last submitted patch, 159: 2926508-159--includes-2957385-20.patch, failed testing. View results

Wim Leers’s picture

So, why is this happening? Well, thank PHP:

$datetime = \DateTime::createFromFormat('Y-m-d', '2017-13-55');
print $datetime->format('Y-m-d') . "\n\n";
var_dump(\DateTime::getLastErrors());

prints:

2018-02-24

array(4) {
  ["warning_count"]=>
  int(1)
  ["warnings"]=>
  array(1) {
    [10]=>
    string(27) "The parsed date was invalid"
  }
  ["error_count"]=>
  int(0)
  ["errors"]=>
  array(0) {
  }
}

😱 🙃🙃🙃🙃

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.

Wim Leers’s picture

If 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 means PATCHing and POSTing 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.

Wim Leers’s picture

And this then should make it green again! ✅

Includes an explicit deprecation test. 💪

The last submitted patch, 162: 2926508-162--includes-2957385-20.patch, failed testing. View results

The last submitted patch, 166: 2926508-166--includes-2957385-20.patch, failed testing. View results

Wim Leers’s picture

Issue summary: View changes

Issue summary updated.

Wim Leers’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 167: 2926508-167--includes-2957385-20.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
989 bytes
59.36 KB
80.55 KB

So close!

No failed assertions, it only failed because the functional test is not catching the deprecation error.

Wim Leers’s picture

Pinged @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.

mpdonadio’s picture

FileSize
26.58 KB

So 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.

jhedstrom’s picture

This all looks great and I think ready to go. Just that one tiny question below.

  1. +++ b/core/modules/hal/src/Normalizer/FieldItemNormalizer.php
    @@ -11,6 +12,10 @@
    +  use FieldableEntityNormalizerTrait {
    +    constructValue as protected;
    +  }
    

    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?

  2. +++ b/core/modules/serialization/tests/modules/test_datatype_boolean_emoji_normalizer/src/Normalizer/BooleanNormalizer.php
    @@ -0,0 +1,36 @@
    + * Normalizes boolean data weirdly: renders them as 👍 (TRUE) or 👎 (FALSE).
    

    Ha! This is great :)

Wim Leers’s picture

Title: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON API » [PP-1] Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON API
Assigned: Wim Leers » Unassigned
Status: Needs review » Reviewed & tested by the community
FileSize
59.28 KB
80.98 KB

#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 Reviewed & tested by the community instead of Postponed to signal that both of these RTBC issues are blockers to #2843147: Add JSON:API to core as a stable module.

Wim Leers’s picture

Title: [PP-1] Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON API » Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON API
FileSize
59.28 KB

#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.

mpdonadio’s picture

Still +1 from me.

Wim Leers’s picture

Thanks :)

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

This looks great, test coverage is thorough - great work
Couple of minor observations

  1. +++ b/core/modules/serialization/src/Normalizer/DateTimeIso8601Normalizer.php
    @@ -0,0 +1,97 @@
    +    else {
    

    nit: we don't need the else here, because early returns 💪

  2. +++ b/core/modules/serialization/src/Normalizer/DateTimeNormalizer.php
    @@ -0,0 +1,102 @@
    +    $default_site_timezone = \Drupal::config('system.date')->get('timezone.default');
    

    should we inject the config factory so the dependency is explicit?

  3. +++ b/core/modules/serialization/src/Normalizer/DateTimeNormalizer.php
    @@ -0,0 +1,102 @@
    +      $date = \DateTime::createFromFormat($format, $data);
    

    do we need @ here to suppress warnings?

  4. +++ b/core/modules/serialization/src/Normalizer/TimeStampItemNormalizerTrait.php
    @@ -4,8 +4,12 @@
    +@trigger_error(__NAMESPACE__ . '\TimeStampItemNormalizerTrait is deprecated in Drupal 8.7.0 and will be removed in Drupal 9.0.0. Use \Drupal\serialization\Normalizer\TimestampNormalizer instead.', E_USER_DEPRECATED);
    

    could be wrong, but I don't see a deprecation test for this, should be straight forward - NW for that

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Needs work » Needs review
FileSize
10.57 KB
60.03 KB
  1. 😀 GREAT! This will make #2958416: Consider making "date only" a separate @DataType simpler too! 👍
  2. 🤯 I can't believe none of us have thought about this so far! 180 comments without anybody pointing that out. This allows me to get rid of some ugly container stuff in the unit tests too. Very grateful that you pointed this out 🙏
  3. No, because that's not how PHP designed this to work. It never fails with an exception or error — at most it returns FALSE. 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:

          $date = \DateTime::createFromFormat($format, $data);
          $errors = \DateTime::getLastErrors();
    
  4. Will do that next.
Wim Leers’s picture

Assigned: Wim Leers » Unassigned
FileSize
3.01 KB
62.3 KB
  1. This wasn't very straightforward. In fact, this is the first deprecated trait with explicit deprecation test coverage. A lot of traits have been deprecated, but none of them have explicit test coverage.

The last submitted patch, 182: 2926508-182.patch, failed testing. View results

Wim Leers’s picture

Title: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON API » Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API

#182 had a random fail in Drupal\Tests\system\FunctionalJavascript\OffCanvasTest. Retesting.

Also: fixing title 😅

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Passed tests, back to RTBC, since it's all trivial changes :)

knyshuk.vova’s picture

The patch looks good and applies successfully. +1 for RTBC.

mradcliffe’s picture

I 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 :-)

Wim Leers’s picture

@mradcliffe WONDERFUL! That is super super helpful feedback, thank you so much! 🙏

plach’s picture

This looks great! I only have a few nits and one question:

  1. +++ b/core/modules/datetime_range/tests/src/Functional/EntityResource/EntityTest/EntityTestDateRangeTest.php
    @@ -0,0 +1,164 @@
    +  protected static $dateString = '2017-03-01T20:02:00';
    ...
    +  protected static $fieldName = 'field_daterange';
    

    Why static?

  2. +++ b/core/modules/serialization/src/Normalizer/DateTimeIso8601Normalizer.php
    @@ -0,0 +1,96 @@
    +    $field_item = $datetime->getParent();
    
    +++ b/core/modules/serialization/src/Normalizer/DateTimeNormalizer.php
    @@ -0,0 +1,120 @@
    +    $drupal_date_time = $datetime->getDateTime();
    

    What about adding an assertion that we're dealing with a DateTimeInterface here?

  3. +++ b/core/modules/serialization/src/Normalizer/DateTimeIso8601Normalizer.php
    @@ -0,0 +1,96 @@
    +    $datetime_type = $field_definition->getSetting('datetime_type');
    

    $field_definition may be null.

  4. +++ b/core/modules/serialization/src/Normalizer/DateTimeIso8601Normalizer.php
    @@ -0,0 +1,96 @@
    +      unset($context['datetime_allowed_formats']);
    ...
    +    unset($context['datetime_allowed_formats']);
    

    Why are we unsetting these if $context is no longer used afterwards?

  5. +++ b/core/modules/serialization/src/Normalizer/DateTimeNormalizer.php
    @@ -0,0 +1,120 @@
    +        return $date;
    

    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.

Wim Leers’s picture

  1. A) because it is meant to immutable, but overridable by subclasses, B) \Drupal\Tests\rest\Functional\ResourceTestBase + \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase started doing this years ago :)
  2. ✅ I like that :) Done!
  3. ✅ Well spotted! Core's existing \Drupal\serialization\Normalizer\FieldItemNormalizer has a similar if-test, but throws an exception. We should do the same here. Done!
  4. Because $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 explicitly unset() 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".
  5. If you look at the preceding lines:
          $date = \DateTime::createFromFormat($format, $data);
          $errors = \DateTime::getLastErrors();
          if ($date !== FALSE && empty($errors['errors']) && empty($errors['warnings'])) {
            return $date;
          }
    

    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.)

plach’s picture

[...] then you can see this is returning a \DateTimeInterface instance.

I meant Drupal\Core\TypedData\Type\DateTimeInterface, sorry for the confusion. Aren't we dealing with the typed data item here?

plach’s picture

Because $context is effectively a global in the Symfony Serialization component [...]

How can we affect the caller code if $context is not passed by reference?

plach’s picture

+++ b/core/modules/serialization/src/Normalizer/DateTimeIso8601Normalizer.php
@@ -37,6 +37,7 @@ class DateTimeIso8601Normalizer extends DateTimeNormalizer {
+    assert($datetime instanceof DateTimeIso8601);

Sorry for the spamminess, we're missing DateTimeNormalizer::normalize() :)

Wim Leers’s picture

#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 a FieldItem object and returning a FieldItem, it:

  1. passes a primitive into FieldItem::setValue(), because that method doesn't accept typed data items. (This is also visible when using Node::create(['field_name' => …]): that must always be a primitive or an array of primitives, never an object.)
  2. itself also doesn't return a FieldItem, it receives an existing FieldItem 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: ✅

plach’s picture

Thanks for the thorough explanations and updates, this is ready to go as far as I'm concerned!

Wim Leers’s picture

Added explicit test for the exception added in #191.

plach’s picture

RTBC +1, thanks!

mradcliffe’s picture

Status: Reviewed & tested by the community » Needs review

I 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.

Wim Leers’s picture

Talked 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.

mradcliffe’s picture

Status: Needs review » Reviewed & tested by the community

Agreed. Back to RTBC + 1.

plach’s picture

Saving credits

plach’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +8.7.0 release notes, +Needs issue summary update

Committed 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.

  • plach committed c8dc57e on 8.7.x
    Issue #2926508 by Wim Leers, mpdonadio, joelstein, tacituseu, jhedstrom...
Wim Leers’s picture

Assigned: Unassigned » Wim Leers

🥳

Will do!

plach’s picture

Awesome work, thanks everyone!

Wim Leers’s picture

#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! 🥳

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
joelstein’s picture

Congrats everyone! Thank you so much for your hard work on this issue.

mondrake’s picture

Wim Leers’s picture

Status: Fixed » Closed (fixed)

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

Sam152’s picture

Re: changes to @FieldType=datetime in the issue summary:

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.

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.