Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Proposal: add IntlDatePatternGenerator #6771
Conversation
| --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'); ?> |
deltragon
Mar 14, 2021
•
Author
Contributor
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)?
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)?
kocsismate
Mar 14, 2021
•
Member
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 🤔
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
kocsismate
Mar 14, 2021
Member
And I don't think the version_compare is necessary for this test.
And I don't think the version_compare is necessary for this test.
deltragon
Mar 14, 2021
•
Author
Contributor
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
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)
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
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)
nikic
Mar 15, 2021
Member
Tests for other ICU versions are under _variant2, _variant3 etc.
Tests for other ICU versions are under _variant2, _variant3 etc.
nikic
Mar 15, 2021
Member
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?
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?
MaxSem
Mar 28, 2021
Contributor
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?
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?
|
Personally, I'd look forward to an |
| --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'); ?> |
kocsismate
Mar 14, 2021
Member
And I don't think the version_compare is necessary for this test.
And I don't think the version_compare is necessary for this test.
deltragon
force-pushed the
deltragon:intl-datetime-patterngenerator
branch
from
d3bb97c
to
2eaed3c
|
@kocsismate Thank you for the review! |
deltragon
changed the title
| 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); |
kocsismate
Mar 28, 2021
Member
What's the reason of this? I see that the check is from IntlDateFormatter, but I don't know why these are necessary.
What's the reason of this? I see that the check is from IntlDateFormatter, but I don't know why these are necessary.
deltragon
Mar 28, 2021
Author
Contributor
For IntlDateFormatter, there is this test:
php-src/ext/intl/tests/bug62081.phpt
Lines 12 to 13
in
8d743d5
How is that (anti-)pattern handled in other cases?
For IntlDateFormatter, there is this test:
php-src/ext/intl/tests/bug62081.phpt
Lines 12 to 13 in 8d743d5
How is that (anti-)pattern handled in other cases?
|
|
||
| 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"); |
kocsismate
Mar 28, 2021
Member
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
| 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
kocsismate
Mar 28, 2021
Member
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.
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.
deltragon
Mar 28, 2021
Author
Contributor
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.
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.
deltragon
force-pushed the
deltragon:intl-datetime-patterngenerator
branch
2 times, most recently
from
cb8c1c5
to
5084f32
deltragon
force-pushed the
deltragon:intl-datetime-patterngenerator
branch
from
5084f32
to
068cceb
deltragon
changed the title
deltragon
force-pushed the
deltragon:intl-datetime-patterngenerator
branch
from
05f307a
to
5b74562
|
Rebased and removed the legacy procedural API. |
|
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
force-pushed the
deltragon:intl-datetime-patterngenerator
branch
from
7e50a00
to
19ffd97
|
Rebased. |
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
$formatargument ofIntlDateFormatter::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::SHORTwill use the short year for "de_DE" ("dd.MM.YY"), andIntlDateFormatter::MEDIUMwill 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 newDateTimePatternGeneratoron 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, asgetBestPattern()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).