Proposal: add IntlDatePatternGenerator by deltragon · Pull Request #6771 · php/php-src

deltragon · 2021-03-14T14:32:42Z

ICU exposes the DateTimePatternGenerator class that allows generating a formatting pattern from a given "skeleton" for a given locale.
This allows more flexibility than the current $format argument of IntlDateFormatter::format(), which takes either a few pre-defined constants or a hard-coded format.
This functionality also has been requested in the past.

For example, if an application requires a format that will always use the long version of a year, but the short version of a month (eg. "MM/dd/YYYY" for "en_US", or "dd.MM.YYYY" for "de_DE"), one is out of luck.
IntlDateFormatter::SHORT will use the short year for "de_DE" ("dd.MM.YY"), and IntlDateFormatter::MEDIUM will use the long version of a month for "en_US" ("MMM dd, YYYY").

With IntlDatePatternGenerator::getBestPattern(), a skeleton can be provided to generate the appropriate pattern for a given locale.
In the use-case given above, the skeleton "YYYYMMdd" will generate the desired patterns for each locale, which can then be passed to IntlDateFormatter::format().

Note about the implementation:
This had been previously implemented as a static method on IntlDateFormatter, creating a new DateTimePatternGenerator on every call.
It is now a separate class that has to be instantiated before use. This leaves us open to implementing other methods/options provided by the underlying DateTimePatternGenerator. I am not sure if there is any use in those, however, as getBestPattern() seems to be the only one with a real-world use-case to me.

(Note: I am aware that this needs to go to internals/an RFC first. I'm currently having issues with my email provider that prevent me from subscribing to/sending to the list. I will send this proposal once these are resolved).

deltragon · 2021-03-14T15:33:48Z

ext/intl/tests/dateformat_get_best_pattern.phpt

+--SKIPIF--
+<?php
+if (!extension_loaded('intl'))	die('skip intl extension not enabled'); ?>
+<?php if (version_compare(INTL_ICU_VERSION, '50.1.2', '<')) die('skip for ICU < 50.1.2'); ?>


As an aside:
This line is included in almost all intl dateformat tests - there, it is used in this form:

<?php if (version_compare(INTL_ICU_VERSION, '50.1.2') >= 0) die('skip for ICU < 50.1.2'); ?>

This, however, ends up skipping the test on my machine (INTL_ICU_VERSION === '67.1'). I assume version_compare is used incorrectly here? Should this be fixed for all other tests as well (and switched to what is the more readable form in my opinion)? Is this line still necessary (as PHP 7.4 requires ICU 50.1)?

Skip messages are a bit deceptive: if fact, die('skip for ICU < 50.1.2'); means that the test is skipped because it's for ICU < 50.1.2. It also took quite some time for me to get accustomed to this.

If ICU 50.1 is required now then these tests are probably not in use anymore 🤔

And I don't think the version_compare is necessary for this test.

Ah yes, that explains the message.
I still dont understand why these tests are not required for newer ICU versions - I would expect basic tests like

php-src/ext/intl/tests/dateformat_format.phpt

Line 5 in d4530f8

<?php if (version_compare(INTL_ICU_VERSION, '50.1.2') >= 0) die('skip for ICU < 50.1.2'); ?>

to still be in use for newer versions/after PHP 7.4.
(For an incomplete list of affected tests, this is the commit that starts skipping a bunch of them for ICU >= 51.2 (was later changed to 50.1): 4840b0a)

Tests for other ICU versions are under _variant2, _variant3 etc.

If ICU 50.1 is required now then these tests are probably not in use anymore thinking

I believe the reason why these tests still exist is that our requirement is ICU 50.1, while this checks for ICU 50.1.2.

Though maybe we can bump up the requirements slightly, it looks like RHEL 7 nowadays ships with ICU 50.2 (according to https://rpms.remirepo.net/rpmphp/zoom.php?rpm=icu). @remicollet Would a 50.2 requirement be safe?

The same RHEL version comes with PHP 5: https://rpms.remirepo.net/rpmphp/zoom.php?rpm=php so is RHEL even supposed to dictate what PHP 8 should do?

kocsismate

Personally, I'd look forward to an IntlDateTimePatternGenerator class, even though it would expose only a small subset of all the ICU capabilities. IMO the static method on IntlDateFormatter doesn't really fit there.

ext/intl/dateformat/dateformat.stub.php

ext/intl/tests/dateformat_get_best_pattern.phpt

kocsismate · 2021-03-14T21:21:45Z

ext/intl/tests/dateformat_get_best_pattern.phpt

+--SKIPIF--
+<?php
+if (!extension_loaded('intl'))	die('skip intl extension not enabled'); ?>
+<?php if (version_compare(INTL_ICU_VERSION, '50.1.2', '<')) die('skip for ICU < 50.1.2'); ?>


And I don't think the version_compare is necessary for this test.

deltragon · 2021-03-28T14:39:26Z

@kocsismate Thank you for the review!
I have switched to a IntlDateTimePatternGenerator class as suggested, and addressed your other comments as well.
I have put the new files in ext/intl/dateformat/ as well, since they belong there thematically - or would you put them in a separate folder?
There are also some names of internal methods/macros I am not entirely sure about, is it okay to leave them as is or should I try to be more consistent there? "DateTimePatternGenerator" is a rather unwieldy name, unfortunately.

ext/intl/dateformat/datetimepatterngenerator_methods.cpp

kocsismate · 2021-03-28T17:02:10Z

ext/intl/dateformat/datetimepatterngenerator_methods.cpp

+	DTPATTERNGEN_METHOD_FETCH_OBJECT_NO_CHECK;
+
+	if (dtpgo->dtpg != NULL) {
+		intl_errors_set(DTPATTERNGEN_ERROR_P(dtpgo), U_ILLEGAL_ARGUMENT_ERROR, "datetimepatterngenerator_create: cannot call constructor twice", 0);


What's the reason of this? I see that the check is from IntlDateFormatter, but I don't know why these are necessary.

For IntlDateFormatter, there is this test:

php-src/ext/intl/tests/bug62081.phpt

Lines 12 to 13 in 8d743d5

$x = new IntlDateFormatter('en', 1, 1);

var_dump($x->__construct('en', 1, 1));

How is that (anti-)pattern handled in other cases?

kocsismate · 2021-03-28T17:11:38Z

ext/intl/dateformat/datetimepatterngenerator_methods.cpp

+
+	intl_stringFromChar(skeleton_uncleaned, skeleton_str, skeleton_len, DTPATTERNGEN_ERROR_CODE_P(dtpgo));
+
+	INTL_METHOD_CHECK_STATUS(dtpgo, "datetimepatterngenerator_get_best_pattern: skeleton was not a valid UTF-8 string");


Suggested change

INTL_METHOD_CHECK_STATUS(dtpgo, "datetimepatterngenerator_get_best_pattern: skeleton was not a valid UTF-8 string");

INTL_METHOD_CHECK_STATUS(dtpgo, "Skeleton is not a valid UTF-8 string");

So the function name shouldn't be added (it's superfluous), the message should start with a capital character, and AFAIR we don't use past tense

Also, I'm wondering if we can consider an invalid UTF-8 skeleton a programmatic error? As far as I see, we can, because the input should come from the programmer, not from the user. If my hypothesis is ok, then we could throw an exception instead of these warnings, like this:
zend_argument_value_error(2, "must be a valid UTF-8 string") (the 2 should be adapted in case of the method counterpart, since $skeleton is the 1st parameter there).

Last year, we migrated hundreds of warnings to exceptions, so it would be nice to follow this practice here as well, if possible.

I have adjusted the messages.
I would agree that this is most likely a programming error - I used a warning here for consistency with similar cases (eg. IntlDateFormatter::setPattern()), but I can switch to using an exception if preferred.

ext/intl/dateformat/datetimepatterngenerator_class.cpp

ext/intl/dateformat/datepatterngenerator_class.cpp

deltragon · 2021-06-07T19:42:10Z

Rebased and removed the legacy procedural API.

ext/intl/dateformat/datepatterngenerator_class.cpp

ext/intl/dateformat/datepatterngenerator_class.h

ext/intl/dateformat/datepatterngenerator_methods.cpp

nikic · 2021-06-08T12:09:09Z

Looks like this needs a rebase. While at it, it would be great to add a mention of the new class in the UPGRADING file.

Co-authored-by: Nikita Popov <nikita.ppv@googlemail.com>

deltragon · 2021-06-08T12:34:56Z

Rebased.

jrfnl · 2022-02-27T19:05:34Z

Just checking - am I reading things correctly when I say that the two procedural functions mentioned in the RFC were not implemented in the final merged implementation ?
https://wiki.php.net/rfc/intldatetimepatterngenerator#proposal

cmb69 · 2022-02-27T19:30:37Z

@jrfnl, ugh, apparently not. I wonder whether that has been explicitly discussed, or was just forgotten.

deltragon · 2022-02-27T19:36:28Z

@jrfnl That was discussed and dropped during the vote, see: https://externals.io/message/114473#114658 and the messages following it.

jrfnl · 2022-02-27T20:25:00Z

Thanks @cmb69 and @deltragon for your quick responses. Just wanted to verify as I'm working on adding detecting for the PHP 8.1 features/deprecations to PHPCompatibility.

Note: in contrast to the original RFC proposal, [the procedural functions mentioned in the RFC were not included in the final implementation](php/php-src#6771 (comment)). Includes unit test. Refs: * https://www.php.net/manual/en/migration81.new-classes.php#migration81.new-classes.intl * https://wiki.php.net/rfc/intldatetimepatterngenerator * php/php-src#6771 * php/php-src@ae9f6e7

deltragon commented Mar 14, 2021

View reviewed changes

kocsismate reviewed Mar 14, 2021

View reviewed changes

deltragon force-pushed the intl-datetime-patterngenerator branch from d3bb97c to 2eaed3c Compare March 28, 2021 14:35

deltragon changed the title ~~Proposal: add IntlDateFormatter::getBestPattern()~~ Proposal: add IntlDateTimePatternGenerator Mar 28, 2021

kocsismate reviewed Mar 28, 2021

View reviewed changes

deltragon force-pushed the intl-datetime-patterngenerator branch 2 times, most recently from cb8c1c5 to 5084f32 Compare March 28, 2021 18:56

carusogabriel added the RFC label Apr 7, 2021

deltragon force-pushed the intl-datetime-patterngenerator branch from 5084f32 to 068cceb Compare April 30, 2021 15:56

deltragon changed the title ~~Proposal: add IntlDateTimePatternGenerator~~ Proposal: add IntlDatePatternGenerator Apr 30, 2021

kocsismate reviewed Jun 1, 2021

View reviewed changes

ext/intl/dateformat/datepatterngenerator_class.cpp Outdated Show resolved Hide resolved

deltragon force-pushed the intl-datetime-patterngenerator branch from 05f307a to 5b74562 Compare June 7, 2021 19:38

nikic reviewed Jun 8, 2021

View reviewed changes

nikic approved these changes Jun 8, 2021

View reviewed changes

Mel Dafert and others added 8 commits June 8, 2021 14:32

Add IntlDateTimePatternGenerator

22e7c07

IntlDateTimePatternGenerator: add clone test

68d7d0f

IntlDateTimePatternGenerator: adjust error messages

5ac4083

IntlDatePatternGenerator: rename

7c10df7

IntlDatePatternGenerator: remove {{{ comments

d38e29b

IntlDatePatternGenerator: remove legacy procedural API

36cb690

Apply suggestions from code review

f3a9e19

Co-authored-by: Nikita Popov <nikita.ppv@googlemail.com>

IntlDatePatternGenerator: coding style

19ffd97

deltragon force-pushed the intl-datetime-patterngenerator branch from 7e50a00 to 19ffd97 Compare June 8, 2021 12:33

IntlDatePatternGenerator: add note to UPGRADING

8fa78b7

nikic merged commit ae9f6e7 into php:master Jun 10, 2021

saundefined mentioned this pull request Oct 21, 2021

[PHP 8.1] Add IntlDatePatternGenerator description php/doc-en#1026

Merged

deltragon deleted the intl-datetime-patterngenerator branch February 27, 2022 20:35

jrfnl mentioned this pull request Mar 9, 2022

PHP 8.1 | NewClasses: account for PHP 8.1 changes PHPCompatibility/PHPCompatibility#1328

Merged

	$x = new IntlDateFormatter('en', 1, 1);
	var_dump($x->__construct('en', 1, 1));


		intl_stringFromChar(skeleton_uncleaned, skeleton_str, skeleton_len, DTPATTERNGEN_ERROR_CODE_P(dtpgo));

		INTL_METHOD_CHECK_STATUS(dtpgo, "datetimepatterngenerator_get_best_pattern: skeleton was not a valid UTF-8 string");

	INTL_METHOD_CHECK_STATUS(dtpgo, "datetimepatterngenerator_get_best_pattern: skeleton was not a valid UTF-8 string");
	INTL_METHOD_CHECK_STATUS(dtpgo, "Skeleton is not a valid UTF-8 string");

Conversation

deltragon commented Mar 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deltragon Mar 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kocsismate Mar 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deltragon Mar 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kocsismate left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deltragon commented Mar 28, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

deltragon commented Jun 7, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nikic commented Jun 8, 2021

Uh oh!

deltragon commented Jun 8, 2021

Uh oh!

jrfnl commented Feb 27, 2022

Uh oh!

cmb69 commented Feb 27, 2022

Uh oh!

deltragon commented Feb 27, 2022

Uh oh!

jrfnl commented Feb 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

deltragon commented Mar 14, 2021 •

edited

Loading

deltragon Mar 14, 2021 •

edited

Loading

kocsismate Mar 14, 2021 •

edited

Loading

deltragon Mar 14, 2021 •

edited

Loading

kocsismate left a comment •

edited

Loading