Discussion: retire the use of strftime from code base
Opened this issue · 3 comments
Problem
Some of the code is still using the deprecated function strftime. Includes:
- modules/Formal Assessment/moduleFunctions.php
- modules/Timetable/moduleFunctions.php
- modules/Finance/src/Forms/FinanceFormFactory.php
- src/Services/Format.php
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:
- If the date format does not need to be localized, it can be replaced with DateTimeInterface::format().
- If the date format needs localization, it can be replaced with IntlDateFormatter::format(). (see this PHP Watch article for details). The class is a part of the Intl bundle available since PHP 5.3.0. It's installed by default or available as packages like "php-intl" for many Linux distros.
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
- We can leave this be and re-visit later. Thou it is not my recommendation.
- We can write or find a drop-in replacement library for strftime date format convert. While it would ease the rewrite we need, we could introduce new issues in this translation layer. It would be preferable to rewrite to the recommended PHP's core function / class format.
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():
- modules/Staff/src/Messages/CoveragePartial.php
- modules/Staff/src/Tables/CoverageCalendar.php
- modules/Staff/src/Tables/AbsenceCalendar.php
- modules/Staff/report_subs_availability.php
- modules/Staff/report_coverage_summary.php
- modules/Staff/report_absences_summary.php
- modules/Staff/report_subs_availabilityWeekly.php
- modules/Staff/report_absences_weekly.php
- modules/Planner/planner.php
- modules/Planner/units_edit_deploy.php
- modules/Planner/units_edit_working.php
- modules/Students/student_view_details.php
- modules/Attendance/report_courseClassesNotRegistered_byDate_print.php
- modules/Attendance/src/AttendanceView.php
- modules/Attendance/attendance_future_byPerson.php
- modules/Attendance/report_formGroupsNotRegistered_byDate_print.php
- modules/Attendance/attendance.php
- modules/Attendance/report_graph_byType.php
- modules/Attendance/report_courseClassesNotRegistered_byDate.php
- modules/Attendance/report_formGroupsNotRegistered_byDate.php
- modules/Timetable/report_viewAvailableTeachers_view.php
- modules/Timetable/report_viewAvailableSpace_view.php
- modules/System Admin/import_manage.php
- modules/Admissions/src/Forms/ApplicationMilestonesForm.php
- modules/Reports/reporting_access_manage.php
- modules/Reports/archive_byStudent_view.php
- modules/Reports/reporting_my.php
- modules/Activities/activities_attendance.php
- modules/Activities/report_attendance.php
Files that are using Gibbon\Service\Format::dateTimeReadable():
- src/Forms/Builder/View/ApplicationCheckView.php
- modules/Staff/src/Messages/AbsenceApproval.php
- modules/Students/firstAidRecord.php
- modules/Students/firstAidRecord_edit.php
- modules/Attendance/attendance_take_byPerson.php
- modules/Admissions/applicationForm_payFee.php
- modules/User Admin/user_manage_password.php
- modules/Reports/reports_generate_ajax.php
- modules/Reports/reports_generate_single.php
- modules/Reports/reports_generate_batch.php
- modules/Reports/reports_send_batch.php
- modules/Reports/reports_send.php
- modules/Reports/reports_generate.php
- modules/Reports/archive_byReport_view.php
The difference between the 2 replacements:
- DateTimeInterface::format()
uses the old date() function formatting string. This method does not use locales. All output is in English. - IntlDateFormatter::format()
uses the ICU date time format:
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! 😃
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:
- create 2 new methods using IntlDateFormatter::format() internally and expose the format string parameter.
(Sigh ... method naming ...) - mark the 2 old methods deprecated.
- slowly rewrite everything that uses the old methods to the new ones.
Format conversion
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 |