Problem/Motivation

When an empty field value goes in, the normalizer can not fetch a valid date instance and then fails calling methods on it, resulting in the following exception:

The website encountered an unexpected error. Please try again later.
Error: Call to a member function setTimezone() on null in Drupal\jsonapi\ForwardCompatibility\Normalizer\DateTimeNormalizer->normalize() (line 49 of modules/contrib/jsonapi/src/ForwardCompatibility/Normalizer/DateTimeNormalizer.php).
Drupal\jsonapi\ForwardCompatibility\Normalizer\DateTimeNormalizer->normalize(Object, 'api_json', Array) (Line: 42)
Drupal\jsonapi\ForwardCompatibility\Normalizer\DateTimeIso8601Normalizer->normalize(Object, 'api_json', Array) (Line: 143)
Symfony\Component\Serializer\Serializer->normalize(Object, 'api_json', Array) (Line: 61)
Drupal\jsonapi\Serializer\Serializer->normalize(Object, 'api_json', Array) (Line: 118)
Drupal\jsonapi\Normalizer\EntityReferenceFieldNormalizer->normalize(Object, 'api_json', Array) (Line: 143)
...

Proposed resolution

Add an if condition to check for that and return NULL in case we do not have data to format.

Remaining tasks

Discussion, patch etc.

User interface changes

None (maybe)

API changes

TBD.

Data model changes

TBD.

Release notes snippet

TBD.

CommentFileSizeAuthor
#14 3017945-14.patch1.82 KBwim leers
#13 3017945-13.patch2.13 KBwim leers
#8 3017945-8.patch883 byteswim leers
#7 3017945-7.patch857 bytesndobromirov
#2 3017945-2.patch1.45 KBndobromirov

Comments

ndobromirov created an issue. See original summary.

ndobromirov’s picture

ndobromirov commented
Status: Active » Needs review
StatusFileSize
new 3017945-2.patch1.45 KB

Here is a simple patch that resolved exception for me. Please review :).

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work
Issue tags: +Needs steps to reproduce, +Needs tests, +API-First Initiative

Good find!

So this is failing on optional @FieldType=datetime instances?

ndobromirov’s picture

ndobromirov commented

Not sure I can't recall it, as I did not bother much to look into the data issue (sorry).

Will try to find an exact reproduce scenario...

ndobromirov’s picture

ndobromirov commented

From what I am finding this should be related to webforms.
How it's happening to trigger a date normalizer - no idea.

The data structure that gets normalized is:
1. Node type basic page.
2. paragraph reference field.
3. Paragraph type.
4. Webform reference field Drupal\webform\Plugin\Field\FieldType\WebformEntityReferenceFieldItemList
5. Some strange property - open. That has empty string value and for some reason it was mapped to Drupal\Core\TypedData\Plugin\DataType\DateTimeIso8601 and thus failing.

If you need more details on this one just ping back...

ndobromirov’s picture

ndobromirov commented

More context:
- Drupal 8.6.3
- jsonapi 2.x-dev (latest)
- jsonapi_extras 3.x-dev (latest)
- webform (8.x-5.0-rc26) (maybe should try an update there is RC29...)
- webform_rest (8.x-2.0-beta1) (same here).

ndobromirov’s picture

ndobromirov commented
StatusFileSize
new 3017945-7.patch857 bytes

Old patch does not apply for some reason. Here is a new (smaller one) against latest dev.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Exception in src/ForwardCompatibility/Normalizer/DateTimeNormalizer.php » WebformEntityReferenceItem has non-required "open" and "closed" datetime_iso8601 properties
Project: JSON:API » Webform
Version: 8.x-2.x-dev » 8.x-5.x-dev
Priority: Major » Normal
Status: Needs work » Needs review
StatusFileSize
new 3017945-8.patch883 bytes

That seems like a bug in Webform. Moving to their issue queue, along with an initial patch.

It also seems that validation constraints are missing from \Drupal\webform\Plugin\Field\FieldType\WebformEntityReferenceItem, it doesn't list any constraints in its annotation. See \Drupal\file\Plugin\Field\FieldType\FileItem for an example.

Status: Needs review » Needs work

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

jrockowitz’s picture

jrockowitz commented

I am stumped because the webform entity reference open/close are optional.

The 'datetime_iso8601' datatype is seldom used is core and always required. For example, the date range field requires both start and end dates.

I feel like the patch #2 makes sense unless we always want to require a value for the 'datetime_iso8601' datatype.

jrockowitz’s picture

jrockowitz commented
Project: Webform » JSON:API
Version: 8.x-5.x-dev » 8.x-2.x-dev

With #2794481: Allow end date to be optional the range's end date would become an optional 'datetime_iso8601' datatype. I think we are going to have the same issue with daterange field.

@Wim Leers I am not sure how Webform can fix this issue. For now, I am moving this ticket back into the JSON API issue.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: WebformEntityReferenceItem has non-required "open" and "closed" datetime_iso8601 properties » Field types with a required property *and* an optional @DataType=datetime_iso8601 property trigger fatal error in DateTimeNormalizer when optional property is empty
Assigned: Unassigned » wim leers
Issue tags: -Needs steps to reproduce

@jrockowitz: Thanks for moving it back to this issue queue! (And congrats on releasing Webform 5.0 stable! 🎉)

So I started working on test coverage. Only to find out that I cannot reproduce this using "regular" (configurable) datetime fields. Hence I installed Webform 5.0 and added a Webform reference (@FieldType=webform, which is \Drupal\webform\Plugin\Field\FieldType\WebformEntityReferenceItem). Instantly it is reproducible.

It reminded me of #3014380-26: EntityReference base fields that are optional are not empty, but its sole item is empty, causing EntityReferenceFieldNormalizer to fail, but it's different.

In this case, there is a non-empty reference field, but there seems to be an empty open property (which is a @DataType=datetime_iso8601 instance). Turns out it's not empty: it's got '' (the empty string) as its value.

This is why it's impossible to reproduce this with an optional datetimefield (either base or configurable): because then they're considered empty. 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.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new 3017945-13.patch2.13 KB

Because of the need for a custom field type to reproduce this, I think writing test coverage for this is a bit excessive. Especially since this is just something that's a forward port of the #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API core issue.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new 3017945-14.patch1.82 KB

Rebased after #2999438: [upstream] Datetime field shown with wrong timezone offset landed.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Field types with a required property *and* an optional @DataType=datetime_iso8601 property trigger fatal error in DateTimeNormalizer when optional property is empty » [upstream] Field types with a required property *and* an optional @DataType=datetime_iso8601 property trigger fatal error in DateTimeNormalizer when optional property is empty
Issue tags: +Needs upstream bugfix

This is really a bug report for #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API in core. Hence marking Needs upstream bugfix.

This is the second bug found in that core patch thanks to JSON:API leaping ahead of core (#2999438: [upstream] Datetime field shown with wrong timezone offset was the other, which led to #3002164: DateTimeIso8601::getDateTime() does not specify the storage timezone when constructing DrupalDateTime object getting fixed in core).

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Bugfix added to the upstream patch at #2926508-149: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API, including tests. If/when that comes back green, I'm committing this.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

#2926508-149: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API came back green! ✅

  • Wim Leers committed d239360 on 8.x-2.x 
    Issue #3017945 by Wim Leers, ndobromirov: [upstream] Field types with a...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Fixed
jrockowitz’s picture

jrockowitz commented

@Wim Leers The patch makes sense. Thanks for taking care of this issue.

Status: Fixed » Closed (fixed)

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