Open yookoala opened 2 years ago
yookoala
commented
2 years ago The difference between the 2 replacements:
SKuipers
commented
2 years ago Huge thanks for looking into this! Luckily, since much of the formatting is contained within the Format class, I think we can definitely start making changes to get inline for 8.1 and 9+ compatibility. As you've noted, the "readable" methods have possibly exposed some of the formatting strings, which is not ideal, but something we should be able to handle. I agree with your suggestion to use the recommended PHP's core function / class format.
My thinking is that, since the Format class gets initialized with access to the session and i18n information, that the class could determine if it needs the IntlDateFormatter functionality, setting a boolean if it does and if that class is available, falling back to the standard DateTimeInterface if not.
Then, in places where the Format class uses strftime internally, it could use a private method to determine how to handle the date formatting based on the boolean set. Anywhere outside of the Format class currently using strftime should likely be refactored to use the Format class.
What do you think? I will have a bit more development time in the coming weeks, I'm happy to start taking a look, but if you're also interested in putting together some suggested fixes I would most certainly welcome them.
Thanks for getting the ball rolling on this! 😃
yookoala
commented
2 years ago I already have something in the work for the internally usages of strftime() (i.e. methods that do not expose strftime() format string to parameter). Will send a PR real soon.
For the Gibbon\Service\Format::dateReadable() and Gibbon\Service\Format::dateTimeReadable() calls, however, I have nothing yet. It's tricky because we have to deal with all the usages of these 2 methods. Also we don't want to simply change the format requirement of these 2 methods and catch other module developers by surprise.
I think the safe way is to:
These are the common formatting used:
| Meaning | strftime() (ref) | IntlDateFormatter::format() (ref) | DateTimeInterface::format() (ref) |
|---|---|---|---|
| Time | -- | -- | -- |
| Full time representation, 12-hours (e.g. "09:34:17 PM") | %r or "%I:%M:%S %p" | "hh:mm:ss a" | "h:i:s a" |
| Full time representation, 24-hours (e.g. "21:34:17") | %T or "%H:%M:%S" | "HH:mm:ss" | "H:i:s" |
| Day | -- | -- | -- |
| Day of the Week, abbreviated (Mon - Sun) | %a | E or EE or EEE | D |
| Day of the Week, full (Monday - Sunday) | %A | EEEE | l |
| Day of the Month (1 - 31) | %e | d | j |
| Day of the Month, two digits (01 - 31) | %d | dd | d |
| Month | -- | -- | -- |
| Month represented in Digits (1 - 12) | none | M | n |
| Month represented in Digits, two digits (01 - 12) | %m | MM | m |
| Month Name, abbreviated (Jan - Dec) | %b | MMM | M |
| Month Name, full (January - December) | %B | MMMM | F |
| Year | -- | -- | -- |
| Year, two digits (e.g. 19) | %y | yy | y |
| Year, four digits (e.g. 2019) | %Y | y or yyyy | Y |
Problem
Some of the code is still using the deprecated function strftime. Includes:
The function strftime() and gmtstrftime() are marked deprecated in 8.1 and set to be removed in 9.0. The usage should be removed.
Proposed Solution
The recommended way is to rewrite them:
One big issue is Gibbon\Services\Format relies heavily on the strftime() time formatting string. That means all usage of the methods of this class would rely on strftime(). That means the usages will all needed to be converted to date format of either DateTimeInterface::format() or IntlDateFormatter::format().
Alternatives
Additional Context
Gibbon\Service\Format methods that are directly using strftime internally:
Gibbon\Service\Format::dateRangeReadable()Gibbon\Service\Format methods that are directly exposing strftime formatting string format to parameter:
Gibbon\Service\Format::dateReadable()Gibbon\Service\Format::dateTimeReadable()Files that are using
Gibbon\Service\Format::dateReadable():Files that are using
Gibbon\Service\Format::dateTimeReadable():