Intl recommendation + PHP81_BC\strftime + Symfony ICU Polyfill

[kamilwylegala]

No description provided.

[diegosurita]

Hey! I see that you only updated the README.md and composer.json files. There are some places that are using strftime function. Will you work on the removal of thoses functions or you want me to handle that in another PR?

[pbarabe]

Hi! Sorry to chime in so late, but regarding updating use of strftime...

If this MR is going to go that far, I'm wondering if it could be made to be less of a breaking change by preferring IntlDateFormatter or datefmt_create and falling back to date if a project does not (or cannot) require intl?

Maybe something like this in Utility\CakeTime:

protected static function _strftime($format, $timestamp) {

    // $format = strftime($format, $timestamp);

    if (function_exists('datefmt_create')) {
        $format = datefmt_create(/* args */);

    } else if (class_exists('IntlDateFormatter')) {
        $format = new IntlDateFormatter(/* args */);

    } else {
        // fallback to date() instead of strftime()
        $format = date($format, $timestamp);
    }
    
    // ...
    // ...the rest of _strftime()
    // ...
}

That strikes me as a bit friendlier approach for anyone who may have difficulty installing intl for some reason.

[kamilwylegala]

@pbarabe @diegosurita Thank you for your comments. Maybe we could use Symfony's polyfill for ithttps://github.com/symfony/polyfill-intl-icu ? What do you think?

[pbarabe]

Hmmm, so looking into this a bit more, I realize the code I suggested previously won't really work (not as suggested, anyway), since PHP's strftime, date, datefmt_create, and IntlDateFormatter all take different format arguments. (Sorry about that - I'd assumed their respective format args were the same or similar, but I was definitely wrong.)

I think that adding ext-intl under the composer.php's suggest, rather than require, is already fairly "nice", since someone can elect to install intl but isn't obliged to.

But since the real problem appears to be that strftime is deprecated in PHP 8.1, I don't think I understand how adding intl helps with that. Won't any use of strftime trigger deprecation warnings, regardless of whether intl is installed or not?

polyfill-intl-icu sounds useful, although since so many of CakeTime's methods wrap strftime in one way or another, it seems like you're then committing to a more substantial rewrite. Maybe that's OK?

I wonder if another path worth considering would be to simply deprecate the entire CakeTime class, given its reliance on strftime? That's effectively suggesting that client applications refactor on something else, though it would seem that one way or another, that'll eventually be necessary anyway.

[pbarabe]

But since the real problem appears to be that strftime is deprecated in PHP 8.1, I don't think I understand how adding intl helps with that. Won't any use of strftime trigger deprecation warnings, regardless of whether intl is installed or not?

Nevermind this. I just looked through the commit history of @diegosurita's PR #64 and saw where he'd started to refactor by using IntlDateFormatter::format() but backed out b/c ext-intl is not required. Sorry for my confusion... I get where that was all going now.

[diegosurita]

Hello guys!

Maybe we could use Symfony's polyfill for ithttps://github.com/symfony/polyfill-intl-icu ? What do you think?

@kamilwylegala Yeah, we definitely could use that polyfill.

[kamilwylegala]

Hello guys!

Maybe we could use Symfony's polyfill for ithttps://github.com/symfony/polyfill-intl-icu ? What do you think?

@kamilwylegala Yeah, we definitely could use that polyfill.

OK, I will work on extending this branch next week. 😁

[kamilwylegala]

@diegosurita Done ✔️ I had to also install PHP81_BC\strftime. Covering all features of strftime using Intl is tricky job. 😁

[pbarabe]

Hi @kamilwylegala, I'm curious about the change in this line. I see that instead of updating the value of $format, you're instantiating a new variable $formatted. But it doesn't seem like $formatted gets used anywhere, and then the method just returns $format. What am I missing?

[kamilwylegala]

Good catch @pbarabe ! I initially experimented with mimicking the behavior of strftime on my own and after that I discovered PHP81_BC/strftime - this is a leftover. I will revert this code. Thanks!

[pbarabe]

Oh good deal - glad I mentioned it. And thanks for finding an easy(ish) solution!

[pbarabe]

Looks like PHP81_BC\strftime is saving the day! I think this all looks great (but still curious about the intent of the change in my previous comment).

[kamilwylegala]

Looks like PHP81_BC\strftime is saving the day! I think this all looks great (but still curious about the intent of the change in my previous comment).

Thank you! I will soon merge this PR 😊

[kamilwylegala]

Thank you @diegosurita @pbarabe Merged!