Open jiripudil opened 2 years ago
@jiripudil friendly pring :slightly_smiling_face: , do you plan to keep working on this ?
Hi, yes, sure, as soon as I'm able :) I was on a few vacations recently and now I need some time to catch up with work stuff first
I had a need for this today. looks like a great PR!
@BenMorel what do you think about this PR? Maybe some one should help @jiripudil to make it mergeble? I think, custom formatting is one of the things that this lib is really miss.
Admittedly, this doesn't scratch my itch that much anymore, and I keep putting it off because it feels scary to get back into all of it after so much time. But I guess it's one of those things where you just need to start and then it gets done – which I'd like to do because I've already set this off, and I too believe this library and its users could benefit from a formatting API.
I'd like to hear @BenMorel's thoughts on the direction of it too. There are some great suggestions regarding the API in the review comments, such as
Also from a DX perspective it could become daunting to type the Formatter class every time. I'm wondering if a
toNativeFormat
andtoIntlFormat
could be considered.
I think that's a discussion that needs to be had before we can move this forward properly.
@jiripudil First of all a big thank you for your PR and sorry for the (huge) delay!
I took the time to review your PR today, this is excellent work, so let's move forward with this API.
I learned from your PR that the format Java Time uses is actually the ICU format, and it's super cool that php-intl already has support for it and that we don't have to re-implement it!
A bit of early feedback:
v0.6
branch and rebase it?PHP_VERSION_ID < 80100
DateTimeFormatContext
with a factory method for each supported date/time class, I'd like to move forward with #67 first, and have each date/time class report directly the fields it supports. This will bring two improvements:
DateTimeFormatContext
at allYearMonth
: instead of determining the date/time classes supported by each symbol, we should be able to map each symbol to a date/time field that's required for formatting!I'd like to hear @BenMorel's thoughts on the direction of it too. There are some great suggestions regarding the API in the review comments, such as
Also from a DX perspective it could become daunting to type the Formatter class every time. I'm wondering if a
toNativeFormat
andtoIntlFormat
could be considered.
That's my point of view as well. format(DateTimeFormatter)
may be left as is (but may just become a proxy for $formatter->format($this)
if all objects implement a Temporal
interface), but adding convenience methods such as formatNative()
and formatIntlPattern()
will be appreciable. Proposals for namings welcome.
A couple questions:
$locale
affect IntlFormatter::ofPattern()
? Should we keep this parameter, or just hardcode an arbitrary value if the locale doesn't affect the output?$dateType
and $timeType
affect an IntlDateFormatter
when $pattern
is set to a non-empty string? From my early testing it looks like these parameters are simply ignored in this case, but this doesn't seem to be documented.(It looks like IntlDateFormatter
's constructor accepts too many parameters at the same time, and would have benefited from factory methods similar to those you created on IntlFormatter
!)
Hi, thanks for the feedback!
let's target the next version (0.6) that will require PHP 8.1
Ok, sure, I'll rebase the PR and update the code accordingly.
instead of introducing a DateTimeFormatContext with a factory method for each supported date/time class, I'd like to move forward with #67 first
That sounds great, looking forward to it!
How does
$locale
affectIntlFormatter::ofPattern()
?
It does greatly, because some patterns need to be localized, such as day-of-week (eeee
) or month names (LLLL
).
How do
$dateType
and$timeType
affect anIntlDateFormatter
when$pattern
is set to a non-empty string? From my early testing it looks like these parameters are simply ignored in this case, but this doesn't seem to be documented.
Yep, I couldn't find it documented either, but my understanding is the same as yours: they are used to create the internal formatter and assemble the initial pattern, which then gets overridden by a custom pattern if you provide one (see code).
How does $locale affect IntlFormatter::ofPattern()?
It does greatly, because some patterns need to be localized, such as day-of-week (
eeee
) or month names (LLLL
).
Could we detect if the given pattern contains symbols that need to be localized, and make $locale
optional if none are detected?
Yep, I couldn't find it documented either, but my understanding is the same as yours: they are used to create the internal formatter and assemble the initial pattern, which then gets overridden by a custom pattern if you provide one (see code).
Thank you for digging into the php-src code! :slightly_smiling_face:
Could we detect if the given pattern contains symbols that need to be localized, and make
$locale
optional if none are detected?
Well, then there are locales that use different numeral systems (mainly Eastern Arabic numerals), so pretty much every symbol might be affected by the locale...
This is my take on the formatting API (#3). It adds the
format()
method toLocal(Date|DateTime|Time)
andZonedDateTime
. The method accepts an implementation of theDateTimeFormatter
interface.The API is designed pretty much like parsing, only backwards. The
format()
method in each date-time class creates an instance ofDateTimeFormatContext
that holds individual fields of the value. This context is then sent into the formatter'sformat()
method and a formatted string is returned.The context contains the individual fields, but also exposes the original value as a whole. This gives custom implementations finer control over the specifics of their formatting.
Built-in formatters
This PR provides two elementary implementations of a
DateTimeFormatter
:NativeFormatter
is the go-to implementation for developers used to the native PHP's formatting options and format string alphabet. It delegates to the nativeDateTime
'sformat()
method, making sure that the format string doesn't refer to any fields that are not available on the formatted value.IntlFormatter
works on top ofext-intl
. It has several factory methods:ofDate()
,ofTime()
, andofDateTime()
take a locale and one of the predefined ICU formats for date, time, or both, respectively. Again, it prevents you from formatting aLocalTime
with a date-based formatter.ofPattern()
gives you the liberty to set your own custom pattern. This uses the ICU SimpleFormat a.k.a. the one Java uses:ofSkeleton()
which utilizes theIntlDatePatternGenerator
added in PHP 8.1. This method only requires you to specify what kind of data you want in the pattern, and it generates the most fitting pattern for you:Parity with parser
I was shortly considering sharing the same classes for formatting and parsing, like Java does. I decided not to, though.
I've skimmed through the several discussions in ECMAScript's Temporal API proposal and the outcome appears to be that parsing non-standard localized date strings is fragile, undeterministic guesswork and should be left to the application code that knows best what formats it works with. And there's already
PatternParser
for that.While we technically could write a strict parser that works on top of
DateTime::createFromFormat
orIntlDateFormatter::parse
, it would add additional difficulties such as splitting the result of these method calls to a set of fields inDateTimeParseResult
. We can revisit the idea later if it is desired, and introduce it as a newDateTimeParser
implementation.