Skip to content

[4.0 - PHP8.1] Remove call to deprecated strftime - with unit test#36469

Closed
PhilETaylor wants to merge 10 commits intojoomla:4.1-devfrom
PhilETaylor:strftime
Closed

[4.0 - PHP8.1] Remove call to deprecated strftime - with unit test#36469
PhilETaylor wants to merge 10 commits intojoomla:4.1-devfrom
PhilETaylor:strftime

Conversation

@PhilETaylor
Copy link
Copy Markdown
Contributor

@PhilETaylor PhilETaylor commented Dec 28, 2021

Summary of Changes

@Bakual @dgrammatiko

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.

@Bakual
Copy link
Copy Markdown
Contributor

Bakual commented Dec 28, 2021

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).

@PhilETaylor

This comment was marked as abuse.

@dgrammatiko
Copy link
Copy Markdown
Contributor

Like, why would you select a Date and then only show the month name?! you wouldn't

Famous last words...

FWIW if @Bakual is ok I'm more than happy with the changes

@PhilETaylor

This comment was marked as abuse.

@brianteeman
Copy link
Copy Markdown
Contributor

@PhilETaylor only when php dates support all non gregorian calendars

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@dgrammatiko
Copy link
Copy Markdown
Contributor

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...

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@Fedik
Copy link
Copy Markdown
Member

Fedik commented Dec 29, 2021

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.

@dgrammatiko
Copy link
Copy Markdown
Contributor

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.

@PhilETaylor

This comment was marked as abuse.

@alikon alikon added the PHP 8.x PHP 8.x deprecated issues label Dec 29, 2021
@Fedik
Copy link
Copy Markdown
Member

Fedik commented Jan 2, 2022

It seems works, on quick test,
You also have to update it here:

$this->value = strftime($this->format, strtotime($this->value));

@PhilETaylor

This comment was marked as abuse.

@chmst chmst changed the base branch from 4.0-dev to 4.1-dev January 31, 2022 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PHP 8.x PHP 8.x deprecated issues Unit/System Tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants