Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Format: new functions for readable date-time conversion #1758

Merged
merged 2 commits into from
May 17, 2024

Conversation

yookoala
Copy link
Member

Description

  • dateIntlReadable to replace dateReadable
  • dateTimeIntlReadable to replace dateTimeReadable
  • Add test coverage to Format::dateTimeReadable, Format::dateIntlReadable and Format::dateTimeIntlReadable.

Motivation and Context

How Has This Been Tested?

  • Locally.
  • CI environment.

* dateIntlReadable to replace dateReadable
* dateTimeIntlReadable to replace dateTimeReadable
* Add test coverage to Format::dateTimeReadable,
  Format::dateIntlReadable and Format::dateTimeIntlReadable.
tests/unit/Services/FormatTest.php Outdated Show resolved Hide resolved
if (empty($dateString)) {
return '';
}
if (!static::$intlFormatterAvailable) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we can handle this with an exception, because currently there will be lots of installations that don't have the PHP intl library (since it was added as a requirement in recent versions), so needs to fallback to regular date formatting. I wonder about a third parameter, called "fallbackPattern" that uses a plain date() format for what to fallback to. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strftime() is set to be deprecated in PHP 8.1 and will probably be removed in PHP 9. To work with international dates, I think intl extension is probably the only way forward. Besides, it is more sensible for user to find out there's a missing dependency by exception than the date silently not working.

One way to work-around the hard dependency-change is to add symfony/polyfill-intl-icu as a project requirement. That way if intl is not installed to the system, the polyfill will provide an IntlDateFormatter implementation to keep everything running.

Copy link
Member

@SKuipers SKuipers Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, agreed that intl is the way forward. For the fallback, I was thinking perhaps a plain, non-internationalised Y-m-d using the date method, as a simple way to ensure that the date will still display even if the intl method isn't available, and avoid adding an exception. I know there are installations that don't have intl, so we can't rely on it 100%, but I think it's okay to not have translated strings in this case, as long as the date displays in a basic form. Most dates in Gibbon are already in the basic form, so the readable method will be an added bonus for systems with intl, for now.

Copy link
Member Author

@yookoala yookoala Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SKuipers: The major problem I can see is that both dateReadable() and dateTimeReadable() accept a $pattern for user to specify the localized date / time format. And some usage of these 2 functions do specify the pattern.

Unfortunately, the formatting string used by IntlDateFormatter, as specified in ICU standard by Unicode Consortium, is not compatible with that of PHP's own date() or DateTimeInterface::format().

That means if the function user specify the localized date with ICU format, it is not compatible with date() and thus cannot be used with date() as a fallback.

So without intl extension, I see 3 behaviours that might work:

  1. the new formatting function negate the provided $pattern and returns date / time string of fixed format; or
  2. provide compatible IntlDateFormatter implementation by polyfill library; or
  3. raise an exception.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Koala, 1 is what I was thinking of. I realize they're not compatible, which is why I was thinking of a fallback parameter as the third param. For 2, I don't see that it's worth it to add a polyfill for such a small piece of functionality when we're moving towards all systems having intl. For 3, we have to be backwards compatible, which leads back to 1. If dateIntlReadable had a simple date('Y-m-d') fallback when it's not possible to display an internationalised string, I think that's perfectly fine, as the important data is still displayed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So something like this?

    public static function dateIntlReadable($dateString, $pattern = 'MMM d, yyyy', $fallbackPattern = 'M d, Y'): string {
        ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this eases the library install transition, the code would be quite painful to maintain in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Yes, I realize it's a bit more cumbersome in the meantime, but important to maintain backwards compatibility, as we have thousands of installs out there, and php extension requirements are some of the harder changes for IT personnel to make, so we have to ease things in gently and keep everything backwards compatible.

Copy link
Member Author

@yookoala yookoala Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the stable way forward would be:

  1. Have the dateIntlReadable() and dateTimeIntlReadable() with the fallback string variable with comparable DateTime formatting string.
  2. If intl is not installed, fallback to use DateTime() and $fallbackPattern to format the date in English only.

Correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sounds good. Thanks for moving this one forward! 😄

Copy link
Member

@SKuipers SKuipers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Koala, my apologies for the delay on this one, it's been a crazy busy school year. I'm going to merge it in and make some of the backwards-compatibility tweaks post-merge. You've done some excellent work moving this forward, thanks again!

@SKuipers SKuipers merged commit 0bf2266 into GibbonEdu:v27.0.00 May 17, 2024
5 checks passed
@yookoala
Copy link
Member Author

@SKuipers: Thanks. Take care.

@yookoala yookoala deleted the feat/intl-readable-datetime branch May 17, 2024 02:47
@yookoala
Copy link
Member Author

yookoala commented May 17, 2024

@SKuipers: Sorry, but I haven't implemented the fallback yet.

The last I checked, the amount of code needed would be huge, and I don't yet have the time to do it. I'd suggest to simply use https://github.com/symfony/polyfill-intl-icu in our case. The polyfill will only be loaded when IntlDateFormatter does not exists in the environment. The package size of Gibbon would be a bit bigger, unfortunately, but this will elegantly solve the backward compatibility issue with minimal work.

yookoala added a commit to yookoala/GibbonEduCore that referenced this pull request May 17, 2024
* Provide fallback to older installations. Supports system without
  php's ext-intl installed.
* Helps with moving forward the GibbonEdu#1758 changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants