Skip to content

Conversation

heiglandreas
Copy link
Contributor

After a twitter-exchange with Dries Vints it looked like there should be a way to easily handle ISO8601- and RFC3339-related datetimes containing fractions.

And after a bit of digging I realized that there already was a change done for RFC3339 by adding the microseconds. But the milliseconds where not yet part of the constants.

This PR tries to fix that by adding new constants:

  • DATE_ATOM_FRACTION
  • DATE_RFC3339_FRACTION
  • DATE_ISO8601_FRACTION
  • DateTimeInterface::ATOM_FRACTION
  • DateTimeInterface::RFC3339_FRACTION
  • DateTimeInterface::ISO8601_FRACTION

All these are based on their fraction-less counterparts and have added the ".u" modifier after the "s" and before the offset-notation

@cmb69 cmb69 requested a review from derickr November 19, 2019 17:16
@cmb69 cmb69 added the Feature label Nov 19, 2019
@cmb69
Copy link
Member

cmb69 commented Nov 19, 2019

The test failure is legit; could you please fix it?

@heiglandreas
Copy link
Contributor Author

Sure. Will do ASAP

be-heiglandreas and others added 2 commits February 25, 2020 14:46
After a twitter-exchange with Dries Vints
(https://twitter.com/driesvints/status/1196780528302776320) it looked
like there should be a way to easily handle ISO8601- and RFC3339-related
datetimes containing fractions.

And after a bit of digging I realized that there already was a change
done for RFC3339 by adding the microseconds. But the milliseconds where
not yet part of the constants.

This commit fixes that by adding new constants:

* DATE_ATOM_FRACTION
* DATE_RFC3339_FRACTION
* DATE_ISO8601_FRACTION
* DateTimeInterface::ATOM_FRACTION
* DateTimeInterface::RFC3339_FRACTION
* DateTimeInterface::ISO8601_FRACTION

All these are based on their fraction-less counterparts and have added
the ".u" modifier after the "s" and before the offset-notation
@heiglandreas heiglandreas force-pushed the addFractionDateConstants branch from e2f6dc5 to d9679fc Compare February 25, 2020 13:52
@XedinUnknown
Copy link

XedinUnknown commented Feb 28, 2020

Fractions are part of the original RFC3339, and are optional. Why does there need to be a separate constant for this?

Also, see bug report.

@driesvints
Copy link

@XedinUnknown because the new constants will provide the greatest datetime precision that's possible with ISO 8601. The current ones lack fractions. The new constant's value is one that's used regularly in, for example, api responses with datetime values.

@heiglandreas
Copy link
Contributor Author

@XedinUnknown Because otherwise it will be a BC break.

@XedinUnknown
Copy link

What's breaking about fixing support broken support for a format?

@kylekatarnls
Copy link
Contributor

kylekatarnls commented Feb 28, 2020

@XedinUnknown an API may extract the result produced by your constant with some RegExp: ^\d+-\d+-\d+ \d+:\d+:\d+$ add the fraction and this extraction suddenly stop working. That's why it's safer to require the user to explicitly add the fraction by selecting the new constant.

@heiglandreas
Copy link
Contributor Author

You won't believe it but people are using those constants for some time now and wrote code that expects the output to be of a certain format. By modifying the existing constant this expectation is broken and therefore the code is not working anymore. So changing that constant is breaking backward compatibility.

That the current constant is not doing what you expect might be due to the fact that you expect something from the constant that is not even described by the standard. The ISO standard says that you can add fractions, but you do not have to add them. So the current constant does exactly that. And if you want to use fractions you can use the new constant.

@XedinUnknown
Copy link

The fact that you can add fractions means that the standard supports fractions. And PHP claims to support the standard. In practice, however, an RFC3339-compliant timestamp that contains a fraction (i.e. is standards-compliant) cannot be parsed by PHP using the DATE_ATOM format, which means that support for it is broken. Fixing something that is broken does not constitute a BC break, because the specification does not change. If somebody has depended on implementation, instead of the specification, then they had made a mistake, and it is up to them to fix it. RFC3339 supports optional fraction, and if I made an expression that does not cater for that, then my expression does not support the RFC3339 standard. Same for the PHP DateTime implementation: if it does not cater for the optional fraction, then it does not support the standard. Adding support for the standard would simply fix that. It is not a BC break.

@heiglandreas
Copy link
Contributor Author

It means your implementation is broken if you relly on the DATE_ATOM constant. Passing an ATOM-Timestamp with fractions into new DateTimeImmutable works fairly well so the PHP-Implementation is not broken. But as a Constant can only hold one format and not contain an either/or along with the fact that PHP, until a certain version didn't support fractions at all, leads to the fact that we have a fraction-less Constant and (hopefully soon) a constant that contains fractions

@kylekatarnls
Copy link
Contributor

@XedinUnknown the fact it fixes a bug (which I disagree anyway) does not mean it's not a breaking change. Applications that properly work relying the current constant format will stop working on update. That's an obvious BC.

@XedinUnknown
Copy link

Passing an ATOM-Timestamp with fractions into new DateTimeImmutable works fairly well so the PHP-Implementation is not broken

It does not, as I demonstrated in the bug report.

But as a Constant can only hold one format and not contain an either/or

An RFC3339 timestamp with a fraction and without a fraction is the same format: the RFC3339 format.

@php php locked as too heated and limited conversation to collaborators Feb 28, 2020
@nikic
Copy link
Member

nikic commented Feb 28, 2020

Thanks everyone, further input will not be needed at this point in time. Please let @derickr review this change.

@cmb69
Copy link
Member

cmb69 commented Dec 28, 2021

@heiglandreas, could you please rebase onto master and force-push, so we get the full CI runs?

@derickr, what do you think about these new constants?

@derickr derickr self-assigned this Dec 15, 2022
@derickr
Copy link
Member

derickr commented May 22, 2024

I am closing this, as there are now too many little function additions to ext/date. Instead, the API should be better thought out and designed, and I'm collecting information at https://docs.google.com/document/d/1pxPSRbfATKE4TFWw72K3p7ir-02YQbTf3S3SIxOKWsk/edit?usp=sharing

@derickr derickr closed this May 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants