[#36469] - [4.0 - PHP8.1] Remove call to deprecated strftime - with unit test
- Closed
- 7 Mar 2022
- Medium
- Build: 4.0-dev
- # 36469
- Diff
- PhilETaylor:strftime
User tests: Successful: Unsuccessful:
Summary of Changes
strftime is deprecated, Joomla Core only uses it (AFAIK) for the frontend calendars for the Smart Search Filters (not enabled by default either).
This PR provides a test case that can be run before the code change, and you can see it passes.
This PR provides a test case that can be run after the code change (The removal of strftime calls), and you can see it passes.
This PR is a light touch approach, simply converting strftime format to DateTimeInterface format using a simple mapping table thanks to @relipse on StackOverflow. Its not perfect, as the comments point out, but its the best that can be expected - especially as its only used in Core Joomla a few places, and might only effect 3PD more than Joomla Core. The formats that are not mapped, and thus dont work, are not the kind of formats you would use in a calendar field anyway (Like, why would you select a Date and then only show the month name?! you wouldn't)...
Testing Instructions
You can test in any PHP version.
In Joomla 4 admin -> Smart Search -> Options -> Enable Date Filters
Create Menu item to the Smart Search
Visit frontend menu item for Smart Search, Click Advanced Search
and/or:
Any 3PD that uses the calendar field is invited to test this at a PHP code level implementing the calendar field with
HTMLHelper::calendar('1978-03-08 06:12:12', 'testName', 'testId', '%Y-%m-%d', []);Actual result BEFORE applying this Pull Request
Calendar works fine, and after selecting a date, the formatted date is shown in the text box (for Joomla Core this is %Y-%m-%d like 1978-03-08 by default)
Expected result AFTER applying this Pull Request
Calendar works fine, and after selecting a date, the formatted date is shown in the text box (for Joomla Core this is %Y-%m-%d like 1978-03-08 by default)
Note
Im aware there is a CalendarField.php call with strftime in it, that will be done next.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries Unit Tests |
| Labels |
Added:
?
?
|
||
We had a proposal to do an automatic conversion of the strftime to Date format in the past. It didn't work well.
Ive not seen it. Just because it failed in the past doesn't mean this attempt will fail too.
I'm not a fan of doing such automatic conversion, but it will be better than nothing.
There is no other way. strftime is going to be removed. If you want backward compatibility then there is no other way.
Main thing to do is imho to rewrite the calendar so it doesn't use the strftime format anymore (if it still does).
Out of scope of this PR. I will not be rewriting the calendar. It still does use strftime format. Rewriting the calendar will not be backward compatible.
This PR is backward compatible and provides a useable Unit Test that you can add as many unit test cases to throw at its implementation change to prove that it works.
@dgrammatiko looking forward to your rewrite that for Joomla 5 that allows us to break b/c and use PHP dates ;-)
@PhilETaylor only when php dates support all non gregorian calendars
Famous last words...
The only strftime formats NOT mapped are
| %U | Week number of the given year, starting with the first Sunday as the first week | 13 (for the 13th full week of the year) |
|---|---|---|
| %V | ISO-8601:1988 week number of the given year, starting with the first week of the year with at least 4 weekdays, with Monday being the start of the week | 01 through 53 (where 53 accounts for an overlapping week) |
| %C | Two digit representation of the century (year divided by 100, truncated to an integer) | 19 for the 20th Century |
|---|
| %g | Two digit representation of the year going by ISO-8601:1988 standards (see %V) | Example: 09 for the week of January 6, 2009 |
|---|---|---|
| %G | The full four-digit version of %g | Example: 2008 for the week of January 3, 2009 |
And if you are really scared that 3PD might need those 5 representations, then we can add them... just seems a lot of work for not a lot of people...
looking forward to your rewrite that for Joomla 5 that allows us to break b/c and use PHP dates ;-)
As I wrote in the other issue 2022 will be the year that all browsers will roll temporal: https://github.com/tc39/proposal-temporal (implemented behind a flag already in a few). I don't know if I will be the one that will rewrite the calendar but once that piece of code is available (actually it is right now with a polyfill...) the calendars will be way easier than the current thing...
But - and Im sorry to bang on about it again - The rewrite of the calendar is a BACKWARD INCOMPATIBLE CHANGE and therefore "for the future" and not a solution to the current issue that strftime is now deprecated.
Therefore we need a plan for Joomla 4 (And Joomla 3 if the plan is to support > PHP 8.1 in Joomla 3).
I propose this. Along with a Unit test that can be abused to prove its worth.
Drone failures unrelated to the content of this PR.
Current PR is okay as temporary solution.
only when php dates support all non gregorian calendars
@brianteeman it can, try this:
$formatter = new \IntlDateFormatter('fa_IR', \IntlDateFormatter::LONG, \IntlDateFormatter::NONE);
echo $formatter->format(time());It just not easy to integrate to the api we have. It should be something new (kind of calendar2 field).
For proper date format we need to think about PHP Intl date formater for server side and "temporal js" for client side.
They both based on ICU (if I right understood).
They will have common pattern format, but different from date(), strftime(). https://unicode-org.github.io/icu/userguide/format_parse/datetime/
For general use should be enough default patterns SHORT, MEDIUM, LONG.
And with it we should drop the translations for patterns in the inputs, it a horror.
For proper date format we need to think about PHP Intl date formater for server side and "temporal js" for client side.
YES, I totally agree here.
A (probably radical) idea: PHP should only (by default) deal ONLY with the SQL format (also the JS part should also return SQL formatted datetime string). That would simplify immensely the code. A quick explanation: the PHP should NOT be used to directly handle input from the UI (that's the JS's part), thus handling only one default format is totally fine.
Current PR is okay as temporary solution.
Alexa, define "temporary" -> for the whole of the Joomla 3 and 4 series and until such time as the calendar can be refactored. Possibly in 2-10 years time.
So we are all in agreement that this needs to be done for PHP 8.1+ compatibility, and doing it the way this PR does is acceptable, despite its small limitations, should not impact the real world, therefore if we can get some actual Human Tests marked we can get this one merged. More talk about "the future" of Calendar will just derail this PR further and is out of scope of this PR.
I'll try to back port this to the CalendarField.php in the libraries sometime today.
| Labels |
Added:
?
|
||
It seems works, on quick test,
You also have to update it here:
You also have to update it here
Like I have said at least twice. But I had to ensure there was buy in to the solution before wasting my time.
| Labels |
Added:
?
Removed: ? |
||
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-03-07 18:34:16 |
| Closed_By | ⇒ | PhilETaylor |
We had a proposal to do an automatic conversion of the strftime to Date format in the past. It didn't work well.
I'm not a fan of doing such automatic conversion, but it will be better than nothing.
Main thing to do is imho to rewrite the calendar so it doesn't use the strftime format anymore (if it still does). And then the calendar formfield can be rewritten to only use the Date formatstring (that one already exists in the language pack).