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.

Sign up for GitHub

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

[REF][PHP8.1][INTL] Remove dependency on strftime function by using t… #24032

Merged
merged 2 commits into from
Jul 28, 2022
Merged

[REF][PHP8.1][INTL] Remove dependency on strftime function by using t… #24032

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
50b5edc
[REF][PHP8.1][INTL] Remove dependency on strftime function by using t…
seamuslee001 Jul 21, 2022
81c58aa
Update to use INTL Date formatter instead
seamuslee001 Jul 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[REF][PHP8.1][INTL] Remove dependency on strftime function by using t… 
…s to generate day full names and abbreviations and month abbreviations

Fix array order and add unit test to lock in fix
  • Loading branch information
@seamuslee001
seamuslee001 committed Jul 28, 2022
commit 50b5edc46edf14589399e4e0f22e0dcb5c030789
4 changes: 1 addition & 3 deletions CRM/Contribute/Form/ContributionCharts.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,7 @@ public function postProcess() {

$chartData = $abbrMonthNames = [];
if (is_array($chartInfoMonthly)) {
for ($i = 1; $i <= 12; $i++) {
$abbrMonthNames[$i] = strftime('%b', mktime(0, 0, 0, $i, 10, 1970));
}
$abbrMonthNames = CRM_Utils_Date::getAbbrMonthNames();

foreach ($abbrMonthNames as $monthKey => $monthName) {
$val = CRM_Utils_Array::value($monthKey, $chartInfoMonthly['By Month'], 0);
Expand Down
2 changes: 1 addition & 1 deletion CRM/Core/Payment/Realex.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ public function setRealexFields(&$params) {
}

// Create timestamp
$timestamp = strftime('%Y%m%d%H%M%S');
$timestamp = date('YmdHis');
$this->_setParam('timestamp', $timestamp);
}

Expand Down
83 changes: 53 additions & 30 deletions CRM/Utils/Date.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,20 +161,25 @@ public static function format($date, $separator = '', $invalidDate = 0) {
*/
public static function getAbbrWeekdayNames() {
$key = 'abbrDays_' . \CRM_Core_I18n::getLocale();
$days = &\Civi::$statics[__CLASS__][$key];
if (!$days) {
$days = [];
if (empty(\Civi::$statics[__CLASS__][$key])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this implementation would break the "first day of week" setting. Previously it indexed the array differently in order to implement that. Now it always starts on Sunday.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point @demeritcowboy I have pushed a fix for that and I have added a unit test to lock in that change

$days = [
0 => ts('Sun'),
1 => ts('Mon'),
2 => ts('Tue'),
3 => ts('Wed'),
4 => ts('Thu'),
5 => ts('Fri'),
6 => ts('Sat'),
];
Copy link
Contributor

Choose a reason for hiding this comment

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

Throwing out an idea (not sure it's a better idea, just an idea, and would solve the chicken/egg with the test and reduce the burden on translators): intl includes translations for all these things without needing to fiddle with OS locale packages. ts is good because it "understands civi", and bypassing it would bypass any custom translation function you've set, but these are standard things so the benefit of using intl may outweigh that downside.

So

 $intl_formatter = IntlDateFormatter::create(CRM_Core_I18n::getLocale(), IntlDateFormatter::MEDIUM, IntlDateFormatter::MEDIUM, null, IntlDateFormatter::GREGORIAN, 'E');
 $days = [
        0 => $intl_formatter->format(strtotime('Sunday')),
        1 => $intl_formatter->format(strtotime('Monday')),
        2 => $intl_formatter->format(strtotime('Tuesday')),
        3 => $intl_formatter->format(strtotime('Wednesday')),
        4 => $intl_formatter->format(strtotime('Thursday')),
        5 => $intl_formatter->format(strtotime('Friday')),
        6 => $intl_formatter->format(strtotime('Saturday')),
      ];

For the long day format it's EEEE instead, but otherwise identical. For month names it would be strtotime('January') etc, and it's MMM for short and MMMM for long.

ts() may have some languages that intl doesn't, but I tried a range of languages and it seems pretty good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mlutfy thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah - the only question is - does everyone always have intl - we could maybe require it for php8.1 (even if only in words) and fall back to what we do currently if not present?

Copy link
Member

Choose a reason for hiding this comment

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

I think relying on php-intl is a good idea. We already rely on less-frequently used packages, such as bcmath, and intl is pretty much always installed on systems I've seen.

And php-intl indeed does not seem to require the locale to be enabled by the OS. I tested locally on a system without the uk_UA locale:

$intl_formatter = IntlDateFormatter::create('uk_UA', IntlDateFormatter::MEDIUM, IntlDateFormatter::MEDIUM, null, IntlDateFormatter::GREGORIAN, 'E');
echo $intl_formatter->format(strtotime('Monday')) . "\n";

outputs: пн, which is correct.

// First day of the week
$firstDay = Civi::settings()->get('weekBegins');

// set LC_TIME and build the arrays from locale-provided names
// June 1st, 1970 was a Monday
CRM_Core_I18n::setLcTime();
for ($i = $firstDay; count($days) < 7; $i = $i > 5 ? 0 : $i + 1) {
$days[$i] = strftime('%a', mktime(0, 0, 0, 6, $i, 1970));
\Civi::$statics[__CLASS__][$key] = [];
for ($i = $firstDay; count(\Civi::$statics[__CLASS__][$key]) < 7; $i = $i > 5 ? 0 : $i + 1) {
\Civi::$statics[__CLASS__][$key][$i] = $days[$i];
}
}
return $days;
return \Civi::$statics[__CLASS__][$key];
}

/**
Expand All @@ -192,20 +197,25 @@ public static function getAbbrWeekdayNames() {
*/
public static function getFullWeekdayNames() {
$key = 'fullDays_' . \CRM_Core_I18n::getLocale();
$days = &\Civi::$statics[__CLASS__][$key];
if (!$days) {
$days = [];
if (empty(\Civi::$statics[__CLASS__][$key])) {
$days = [
0 => ts('Sunday'),
1 => ts('Monday'),
2 => ts('Tuesday'),
3 => ts('Wednesday'),
4 => ts('Thursday'),
5 => ts('Friday'),
6 => ts('Saturday'),
];
// First day of the week
$firstDay = Civi::settings()->get('weekBegins');

// set LC_TIME and build the arrays from locale-provided names
// June 1st, 1970 was a Monday
CRM_Core_I18n::setLcTime();
for ($i = $firstDay; count($days) < 7; $i = $i > 5 ? 0 : $i + 1) {
$days[$i] = strftime('%A', mktime(0, 0, 0, 6, $i, 1970));
\Civi::$statics[__CLASS__][$key] = [];
for ($i = $firstDay; count(\Civi::$statics[__CLASS__][$key]) < 7; $i = $i > 5 ? 0 : $i + 1) {
\Civi::$statics[__CLASS__][$key][$i] = $days[$i];
}
}
return $days;
return \Civi::$statics[__CLASS__][$key];
}

/**
Expand All @@ -219,19 +229,26 @@ public static function getFullWeekdayNames() {
*/
public static function &getAbbrMonthNames($month = FALSE) {
$key = 'abbrMonthNames_' . \CRM_Core_I18n::getLocale();
$abbrMonthNames = &\Civi::$statics[__CLASS__][$key];
if (!isset($abbrMonthNames)) {

// set LC_TIME and build the arrays from locale-provided names
CRM_Core_I18n::setLcTime();
for ($i = 1; $i <= 12; $i++) {
$abbrMonthNames[$i] = strftime('%b', mktime(0, 0, 0, $i, 10, 1970));
}
if (empty(\Civi::$statics[__CLASS__][$key])) {
\Civi::$statics[__CLASS__][$key] = [
1 => ts('Jan'),
2 => ts('Feb'),
3 => ts('Mar'),
4 => ts('Apr'),
5 => ts('May'),
6 => ts('Jun'),
7 => ts('Jul'),
8 => ts('Aug'),
9 => ts('Sep'),
10 => ts('Oct'),
11 => ts('Nov'),
12 => ts('Dec'),
];
}
if ($month) {
return $abbrMonthNames[$month];
return \Civi::$statics[__CLASS__][$key][$month];
}
return $abbrMonthNames;
return \Civi::$statics[__CLASS__][$key];
}

/**
Expand Down Expand Up @@ -287,7 +304,8 @@ public static function unixTime($string) {

/**
* Create a date and time string in a provided format.
*
* %A - Full day name ('Saturday'..'Sunday')
* %a - abbreviated day name ('Sat'..'Sun')
* %b - abbreviated month name ('Jan'..'Dec')
* %B - full month name ('January'..'December')
* %d - day of the month as a decimal number, 0-padded ('01'..'31')
Expand Down Expand Up @@ -318,6 +336,8 @@ public static function customFormat($dateString, $format = NULL, $dateParts = NU
// 1-based (January) month names arrays
$abbrMonths = self::getAbbrMonthNames();
$fullMonths = self::getFullMonthNames();
$fullWeekdayNames = self::getFullWeekdayNames();
$abbrWeekdayNames = self::getAbbrWeekdayNames();

if (!$format) {
$config = CRM_Core_Config::singleton();
Expand Down Expand Up @@ -381,6 +401,8 @@ public static function customFormat($dateString, $format = NULL, $dateParts = NU
$second = (int) substr($dateString, 12, 2);
}

$dayInt = date('w', strtotime($dateString));

if ($day % 10 == 1 and $day != 11) {
$suffix = 'st';
}
Expand Down Expand Up @@ -414,6 +436,8 @@ public static function customFormat($dateString, $format = NULL, $dateParts = NU
}

$date = [
'%A' => $fullWeekdayNames[$dayInt] ?? NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this PR is otherwise good but doesn't this change the output for anywhere/anyone currently using %A? Before it was AM/PM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it would but grepping in the universe I am not finding references to anyone calling this function and passing in "%A" so I think we are safe to fix it and looking at the git / svn history it was added here civicrm/civicrm-svn@30694cd . It looks like it might have been used as part of custom field formatting but that seems long gone now

Copy link
Contributor

Choose a reason for hiding this comment

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

I see your point, but it's used in message templates (via crmDate) which isn't a greppable thing. It's also used for these https://dmaster.demo.civicrm.org/civicrm/admin/setting/date?reset=1 (here's one example). Although, while I do know of sites that change those, they would usually use %P which is the same and already there. In that svn link it's kind of confusing why they would add %A when right above where they added it it already had %P. But there's still message templates, but yes it might be rare. Perhaps an upgrade message? Wouldn't have to be in this PR.

Is time travel a feature in civi yet? Maybe part of api4? Can we go back in time and fix the svn? I also had a need for time travel the other day.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@demeritcowboy The default templates are greppable and in none of the defaults they are using %A that gets passed through and the help text in that date format is pushing towards %P rather than %A but yes I can see that I think an upgrade status message would be sensible

'%a' => $abbrWeekdayNames[$dayInt] ?? NULL,
'%b' => $abbrMonths[$month] ?? NULL,
'%B' => $fullMonths[$month] ?? NULL,
'%d' => $day > 9 ? $day : '0' . $day,
Expand All @@ -430,7 +454,6 @@ public static function customFormat($dateString, $format = NULL, $dateParts = NU
'%i' => $minute > 9 ? $minute : '0' . $minute,
'%p' => strtolower($type),
'%P' => $type,
'%A' => $type,
'%Y' => $year,
'%s' => str_pad($second, 2, 0, STR_PAD_LEFT),
'%S' => str_pad($second, 2, 0, STR_PAD_LEFT),
Expand Down
17 changes: 17 additions & 0 deletions tests/phpunit/CRM/Utils/DateTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,8 @@ public function testCustomFormat() {
$this->assertEquals(CRM_Utils_Date::customFormat($dateTime, "%P"), "PM");
$this->assertEquals(CRM_Utils_Date::customFormat($dateTime, "%Y"), "2018");
$this->assertEquals(CRM_Utils_Date::customFormat($dateTime, "%s"), "44");
$this->assertEquals(CRM_Utils_Date::customFormat($dateTime, "%A"), "Thursday");
$this->assertEquals(CRM_Utils_Date::customFormat($dateTime, "%a"), "Thu");
}

/**
Expand All @@ -293,6 +295,8 @@ public function testCustomFormatTs() {
$this->assertEquals(CRM_Utils_Date::customFormatTs($ts, "%p"), "pm");
$this->assertEquals(CRM_Utils_Date::customFormatTs($ts, "%P"), "PM");
$this->assertEquals(CRM_Utils_Date::customFormatTs($ts, "%Y"), "2018");
$this->assertEquals(CRM_Utils_Date::customFormatTs($ts, "%A"), "Thursday");
$this->assertEquals(CRM_Utils_Date::customFormatTs($ts, "%a"), "Thu");
}

/**
Expand Down Expand Up @@ -325,6 +329,19 @@ public function testLocalizeConsts() {
}
}

public function testWeekDayArrayOrder() {
$this->callAPISuccess('Setting', 'create', ['weekBegins' => 1]);
$this->assertEquals([
1 => 'Monday',
2 => 'Tuesday',
3 => 'Wednesday',
4 => 'Thursday',
5 => 'Friday',
6 => 'Saturday',
0 => 'Sunday',
], CRM_Utils_Date::getFullWeekdayNames());
}

/**
* Test formatDate function.
*
Expand Down