Update TimestampBase to allow for DateTimeInterface rather than specific implementation and allow fluent setter for factory usage.

[shrikeh]

• Allow Timestamp to be populated by any class that matches the built-in DateTimeInterface interface (ie DateTimeImmutable).

• Change to fluent setter to more easily use as a factory.

[google-cla[bot]]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[shrikeh]

Widening to DateTimeInterface is great, but I'm not sold on making this method fluid, as thats considered a breaking change.

@bshaffer I'm surprised it's considered a breaking change, given that the previous method had no void return type hint (and indeed returned nothing). As such, I'm curious to the reasoning - is there a use case that I've overlooked?

[shrikeh]

@bshaffer Having had a look about and discussing with fellow PHP engineers, this feels more like "policy" rather than an actual concern. That makes it no less blocking of course - policies are policies.

Would you prefer that I refactor to have a static named constructor of something like Timestamp::createFromDateTimeInterface(DateTimeInterface $dateTime): static to act as a factory method, and keep the behaviour of fromDateTime as is? Also, if so, should fromDateTime() explicitly have the void return type?

[haberman]

Adding a return value doesn't seem like a breaking change unless it would break existing code somehow.

However I have a different concern about making it fluent: consistency. I'm not aware of any other places in the PHP API where we use fluent APIs. If we add it in this one place, it will be inconsistent with everything else, and would naturally lead to ad hoc PRs suggesting that we add it in other places.

[shrikeh]

Adding a return value doesn't seem like a breaking change unless it would break existing code somehow.

However I have a different concern about making it fluent: consistency. I'm not aware of any other places in the PHP API where we use fluent APIs. If we add it in this one place, it will be inconsistent with everything else, and would naturally lead to ad hoc PRs suggesting that we add it in other places.

I see, a "sets precedent" reservatioon. I haven't yet found another protobuf type that uses an in-built PHP class such as the DateTime family as yet, which may mean it truly is unique - it certainly appears to be the only fromX I have encountered so far.

Regardless, I do believe it should have a factory of some sort. Mutability isn't one of my favourite things, but a static named constructor createFromDateTime which calls the method with the expanded type would be good. What are your thoughts?

[github-actions[bot]]

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active, please add a comment.

This PR is labeled inactive because the last activity was over 90 days ago. This PR will be closed and archived after 14 additional days without activity.

[github-actions[bot]]

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active or becomes active again, please reopen it.

This PR was closed and archived because there has been no new activity in the 14 days since the inactive label was added.