Skip to content

Conversation

cedric-anne
Copy link
Contributor

This is a fix for bug #81606.

According to documentation, $dateType and $timeType should accept null values.

Date type to use (none, short, medium, long, full). This is one of the IntlDateFormatter constants. It can also be null, in which case ICUʼs default date type will be used.

@cmb69
Copy link
Member

cmb69 commented Nov 10, 2021

Hmm, to get the default, one would need to specify UDAT_DEFAULT, but there appears to be no PHP constant for that. Anyway, the value would be 4, and resolves to UDAT_MEDIUM (currently). Prior to PHP 8.1.0, null was coerced to 0 in coercive type mode (strict_types=0), but already rejected in strict type mode (strict_types=1) in prior PHP versions. Supporting null would require to treat it like 0 for BC with coercive type mode. That would still render the documentation as erroneous. I think it's best to just fix the docs (s/null/0), and maybe to add IntlDateFormatter::DEFAULT.

@nikic
Copy link
Member

nikic commented Nov 11, 2021

In #6914 we made these arguments optional with ::FULL default, on the premise that that's what you would get through coercion of null to 0 to ::FULL. Do you think it would make more sense to change the defaults to ::MEDIUM to match the actual ICU default?

@cmb69
Copy link
Member

cmb69 commented Nov 11, 2021

Ah! I still think it's fine as is, and we should fix the docs. Probably, not s/null/0, though, but rather remove that sentence ("It can also be null, in which case ICUʼs default date type will be used.")

@cedric-anne
Copy link
Contributor Author

I hadn't searched until now, and indeed, this seems to be a documentation issue. I guess it's best not to change the current behavior.

Documentation fixed in php/doc-en#1086

cmb69 pushed a commit to php/doc-en that referenced this pull request Nov 11, 2021
@cedric-anne cedric-anne deleted the bugfix/81606 branch November 12, 2021 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants