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

seamuslee001 · 2022-07-21T06:17:29Z

…s to generate day full names and abbreviations and month abbreviations

Overview

This aims to reduce dependency on the strftime function which is deprecated in php8.1 by replacing usage of it in CRM_Utils_Date and a couple of other places with ts() similar to #21157

Before

strftime used to generate translated strings

After

ts used to generate same translated strings

ping @demeritcowboy @mlutfy @eileenmcnaughton @totten @colemanw

civibot · 2022-07-21T06:17:31Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

eileenmcnaughton · 2022-07-21T07:41:25Z

The recommended date function is https://www.php.net/manual/en/intldateformatter.format.php - which I assume we could use with the right polyfills?

demeritcowboy · 2022-07-21T11:31:49Z

On the intl polyfill - it would be incompatible with the existing require: ext-intl in composer.json. If you don't have intl you get this when trying to composer require civicrm/civicrm-core (e.g. when installing on drupal 9), and it happens whether composer.json has the polyfill or not.

In PackageDiscoveryTrait.php line 312:

  Package civicrm/civicrm-core has requirements incompatible with your PHP version, PHP extensions and Composer version:
    - civicrm/civicrm-core dev-master requires ext-intl * but it is not present.

Also that particular polyfill is english-only.

But for weekday names etc I think this PR makes sense. ts('Sun') might make for some amusing translations.

demeritcowboy · 2022-07-21T11:32:52Z

CRM/Utils/Date.php

-      for ($i = $firstDay; count($days) < 7; $i = $i > 5 ? 0 : $i + 1) {
-        $days[$i] = strftime('%a', mktime(0, 0, 0, 6, $i, 1970));
-      }
+    if (empty(\Civi::$statics[__CLASS__][$key])) {


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.

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

demeritcowboy · 2022-07-21T12:04:18Z

This is probably because those translations might not exist yet.

CRM_Utils_DateTest::testLocalizeConsts
Check temporal names in fr_FR
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 'janv.'
-    1 => 'mar.'
+    0 => 'Jan'
+    1 => 'Tue'
     2 => 'mars'
-    3 => 'jeudi'
+    3 => 'Thursday'
 )

demeritcowboy · 2022-07-23T15:28:59Z

CRM/Utils/Date.php

+        4 => ts('Thu'),
+        5 => ts('Fri'),
+        6 => ts('Sat'),
+      ];


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.

@mlutfy thoughts?

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?

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.

…s to generate day full names and abbreviations and month abbreviations Fix array order and add unit test to lock in fix

seamuslee001 · 2022-07-28T00:31:56Z

given @mlutfy's comment I have updated the code as per suggestion from @demeritcowboy for intl formatter

demeritcowboy · 2022-07-28T00:43:55Z

CRM/Utils/Date.php

+      $intl_formatter = IntlDateFormatter::create(CRM_Core_I18n::getLocale(), IntlDateFormatter::MEDIUM, IntlDateFormatter::MEDIUM, null, IntlDateFormatter::GREGORIAN, 'MMM');
+      \Civi::$statics[__CLASS__][$key] = [
+        1 => $intl_formatter->format(strtotime('January')),
+        2 => $intl_formatter->format(strtotime('Feburary')),


I believe it's spelled "Liberry"

demeritcowboy · 2022-07-28T02:48:49Z

CRM/Utils/Date.php

@@ -414,6 +439,8 @@ public static function customFormat($dateString, $format = NULL, $dateParts = NU
      }

      $date = [
+        '%A' => $fullWeekdayNames[$dayInt] ?? NULL,


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.

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

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.

@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

seamuslee001 added the has-test label Jul 21, 2022

civibot bot added the master label Jul 21, 2022

seamuslee001 force-pushed the remove_dependency_strftime branch from e205520 to 4ffe9b9 Compare July 21, 2022 06:19

seamuslee001 mentioned this pull request Jul 21, 2022

[REF][PHP8.1] Apply patches to pear/log and zetacomponents/mail packa… #24019

Merged

seamuslee001 force-pushed the remove_dependency_strftime branch from 4ffe9b9 to 1aa1339 Compare July 21, 2022 09:50

demeritcowboy reviewed Jul 21, 2022

View reviewed changes

seamuslee001 force-pushed the remove_dependency_strftime branch from 1aa1339 to 8f47f39 Compare July 22, 2022 23:40

demeritcowboy reviewed Jul 23, 2022

View reviewed changes

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

50b5edc

…s to generate day full names and abbreviations and month abbreviations Fix array order and add unit test to lock in fix

seamuslee001 force-pushed the remove_dependency_strftime branch from 8f47f39 to 1d8b16e Compare July 28, 2022 00:31

demeritcowboy reviewed Jul 28, 2022

View reviewed changes

seamuslee001 force-pushed the remove_dependency_strftime branch from 1d8b16e to 36f41c5 Compare July 28, 2022 00:44

Update to use INTL Date formatter instead

81c58aa

seamuslee001 force-pushed the remove_dependency_strftime branch from 36f41c5 to 81c58aa Compare July 28, 2022 00:45

seamuslee001 mentioned this pull request Jul 28, 2022

[REF][PHP8.1] Replace usage of the date_format modifer with crmDate t… #24008

Merged

demeritcowboy reviewed Jul 28, 2022

View reviewed changes

demeritcowboy merged commit 4b4b8a0 into civicrm:master Jul 28, 2022

seamuslee001 deleted the remove_dependency_strftime branch July 28, 2022 04:40

seamuslee001 mentioned this pull request Jul 28, 2022

[REF] Add in Post Upgrade message about %A format change #24070

Closed

demeritcowboy mentioned this pull request Jul 28, 2022

Upgrade task to handle %A date format specifier change #24077

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

seamuslee001 commented Jul 21, 2022

civibot bot commented Jul 21, 2022

eileenmcnaughton commented Jul 21, 2022

demeritcowboy commented Jul 21, 2022

demeritcowboy Jul 21, 2022

seamuslee001 Jul 22, 2022

demeritcowboy commented Jul 21, 2022

demeritcowboy Jul 23, 2022

seamuslee001 Jul 24, 2022

eileenmcnaughton Jul 25, 2022

mlutfy Jul 25, 2022

seamuslee001 commented Jul 28, 2022

demeritcowboy Jul 28, 2022

seamuslee001 Jul 28, 2022

demeritcowboy Jul 28, 2022

seamuslee001 Jul 28, 2022

demeritcowboy Jul 28, 2022

seamuslee001 Jul 28, 2022

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

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

Conversation

seamuslee001 commented Jul 21, 2022

Overview

Before

After

civibot bot commented Jul 21, 2022

eileenmcnaughton commented Jul 21, 2022

demeritcowboy commented Jul 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demeritcowboy commented Jul 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seamuslee001 commented Jul 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment