Unable to parse 7 digit precision in the "time" field

[avramovic]

I was searching for similar issue and found #35, but this SDK still can't parse 7 digit precision in the timestamp, for example 2023-04-12T12:50:04.2121108Z

CloudEvents\Utilities\TimeFormatter::decode(): Argument #1 ($time) is not a valid RFC3339 timestamp

Interestingly, the 7 digit precision comes from CloudEvents C# SDK. One would assume these SDKs would be compatible.

Can something be done about this? Right now, I have to write a custom Denormalizer class which parses the timestamp and formats it back to 6 digit precision, before passing it to the original (included) denormalizer.

[jlaswell]

Part of the issue with the precision comes from PHP's precision to 6 digits in versions less than 8. DateTimeImmutable is precise up to 9 digits in PHP 8+, so we are taking the approach of trimming to 6 digits as a workaround while we still support PHP 7.4. Open to feedback around our approach here as you seem to already have a Denormalizer handling some of this functionality.

---

We'll look to get this reviewed by EOW with some internal team members and hopefully have a solution here.

If you see more test cases you would like to add or account for, please go ahead and add code suggestions to the PR. I'll hold it open for a bit so you should have morning and early afternoon tomorrow to provide any feedback.

[avramovic]

Hm, I can only add that we're using PHP 8.1 and it doesn't work with more than 6 digits: https://3v4l.org/KYPtB#v8.1.18

[GrahamCampbell]

Is 7 digits of precision common in the .net world? I'm wondering why they made that implementation decision. If all the other libraries don't use 7 digits, maybe the correct thing to do is to "fix" the dotnet implementation. That said, I suppose all other libraries should also truncate off the extra digits, for good interoperability with existing serialized events.

[jlaswell]

Hm, I can only add that we're using PHP 8.1 and it doesn't work with more than 6 digits: 3v4l.org/KYPtB#v8.1.18

Seems you're right. Don't quite know where I got the assumption of 9 digits now. The issue with the previous implementation is the usage of createFromFormat, which I assume is where the breakdown of >6 digits comes from. Now, we are instantiating a new DateTimeImmutable. You can see the difference here.

The benefit is that decode now supports datetimes with greater than 6 digit precision, but PHP's implementation only retains 6 digits. You'll need to continue with a custom implementation if you'd like to maintain greater precision.

Is 7 digits of precision common in the .net world? I'm wondering why they made that implementation decision. If all the other libraries don't use 7 digits, maybe the correct thing to do is to "fix" the dotnet implementation. That said, I suppose all other libraries should also truncate off the extra digits, for good interoperability with existing serialized events.

This is a valid point, but I do think preferring support of the spec is most polite here.

[avramovic]

The issue with the previous implementation is the usage of createFromFormat, which I assume is where the breakdown of >6 digits comes from. Now, we are instantiating a new DateTimeImmutable.

That's exactly how my workaround works. Your Denormalizer class is final so I couldn't extend it, but I'm creating a decorator like this:

class MyCloudEventDenormalizer implements DenormalizerInterface
{
  protected DenormalizerInterface $originalDenormalizer;

  public function __construct()
  {
    $this->originalDenormalizer = new Denormalizer();
  }

  public function denormalize(array $payload): CloudEventInterface
  {
    $time = new DateTime($payload['time']);
    $payload['time'] = $time->format('Y-m-d\TH:i:s.uP');

    return $this->originalDenormalizer->denormalize($payload);
  }
}

And then, instead of calling JsonDeserializer::create(), I'm using new JsonDeserializer(new MyCloudEventDenormalizer) to instantiate JsonDeserializer - and it works.

If you want to switch to using new DateTimeImmutable, I think you can remove trimMicroseconds function.

[jlaswell]

Unfortunately, PHP 7.4 support requires the trimMicroseconds to prevent the exception on instantiation. There is a polyfill for the ValueError exception in the sdk but not on 3v4l.

[avramovic]

Oh, sorry, I completely forgot about 7.4 🤦 You're right, it needs to stay there for backwards compatibility.

[jlaswell]

I didn't remember but the unit tests caught it for us. :)

Sending this along now and will cut a release next week. Feel free to use mainline as a test until that's done.

Thanks for your report, help, and patience here.