modxcms / revolution

MODX Revolution - Content Management Framework
https://modx.com/
GNU General Public License v2.0
1.36k stars 529 forks source link

PHP 8.1: strftime() is deprecated #15864

Open JoshuaLuckers opened 3 years ago

JoshuaLuckers commented 3 years ago

The strftime() function is called by the core in various places.

In PHP 8.1 this will trigger a deprecation warning because the strftime() and gmstrftime() functions have been deprecated in favor of date()/DateTime::format() (for locale-independent formatting) or IntlDateFormatter::format() (for locale-dependent formatting).

The strftime() and gmstrftime() functions exhibit similar issues as strptime(), in that the formats they support, as well as their behavior, is platform-dependent. Unlike strptime(), these functions are available on Windows, though with a different feature set than on Linux. Musl-based distributions like Alpine do not support timezone-related format specifiers correctly. These functions are also locale-based, and as such may exhibit thread-safety issues.

Once again date() or DateTime::format() provide portable alternatives, and IntlDateFormatter::format() provides a more sophisticated, localization-aware alternative.

References:

Environment

muzzwood commented 3 years ago

I've been experimenting with this. It looks like the PHP extension intl would be a requirement if we're going to use IntlDateFormatter. Maybe we need a helper method/class that determines if it's installed and uses DateTime::format which isn't locale aware otherwise.

Usage of IntlDateFormatter would be something along the lines of:

$locale = $this->modx->getOption('locale') ?? '';
$timezone = $this->modx->getOption('timezone') ?? '';

$formatter = new IntlDateFormatter($locale, IntlDateFormatter::SHORT, IntlDateFormatter::SHORT, $timezone);
return $formatter->format(new DateTime());
rthrash commented 2 years ago

This issue has been mentioned on MODX Community. There might be relevant details there:

https://community.modx.com/t/whats-happening-with-the-strftime-and-date-output-modifiers/4826/3

Mark-H commented 2 years ago

Two weeks ago I released a SiteDash update that tracks available PHP extensions to see if requiring intl would be feasible. On a sample size so far of about 18% of sites connected to SiteDash, roughly 81% of those have the intl extension enabled. That's more than imagick (75%) but less than fileinfo (91%).

At those numbers I'd personally say we can conditionally use it in core; fallback to non-localised date() with a warning-level error suggesting people to install intl for locale support.

JoshuaLuckers commented 2 years ago

Sounds good to me!

JoshuaLuckers commented 2 years ago

@Mark-H I'm proposing to make a start with this in 3.1. What do you think?

opengeek commented 2 years ago

@Mark-H I'm proposing to make a start with this in 3.1. What do you think?

Just keep in mind this is only a deprecation warning and will continue to work until PHP 9 is released. We also need to consider places where this will cause BC breaks and keep compatibility for those on PHP <= 8.x for the MODX 3.x branch.