Updates Requested bug PR-4.4-dev Pending

User tests: Successful: Unsuccessful:

avatar korenevskiy
korenevskiy
27 Jul 2023

Pull Request for ISSUE: #41265

Summary of Changes

Before:

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

After

$this->value =  (new DateTime($this->value))->format($this->format);

Testing Instructions

<field name="date_start" type="calendar" default="NOW"
label="MOD_MULTIMODULE_FIELD_FILTERDATESTART_LABEL"     showon="date_is:on"  
showtime="false"  singleheader="true" />   

This XML code add to XML file module.
Enabled all Errors and enable debug mode.
Change PHP to 8.2 in for this site.
And open configuration this module.

Actual result BEFORE applying this Pull Request

You see error on depricated for function strftime().

Expected result AFTER applying this Pull Request

After change you not see this message.
But this field will be work.

avatar joomla-cms-bot joomla-cms-bot - change - 27 Jul 2023
Category Libraries
avatar korenevskiy korenevskiy - open - 27 Jul 2023
avatar korenevskiy korenevskiy - change - 27 Jul 2023
Status New Pending
avatar korenevskiy korenevskiy - change - 27 Jul 2023
Labels Added: PR-4.3-dev
avatar korenevskiy korenevskiy - change - 27 Jul 2023
The description was changed
avatar korenevskiy korenevskiy - edited - 27 Jul 2023
avatar richard67
richard67 - comment - 27 Jul 2023

@korenevskiy Will this solve issue #41265 ? If so, please link to the issue at the top of the description of this PR (Pull Request for Issue #xxxxx).

avatar korenevskiy korenevskiy - change - 27 Jul 2023
The description was changed
avatar korenevskiy korenevskiy - edited - 27 Jul 2023
avatar korenevskiy korenevskiy - change - 27 Jul 2023
The description was changed
avatar korenevskiy korenevskiy - edited - 27 Jul 2023
avatar korenevskiy korenevskiy - change - 27 Jul 2023
The description was changed
avatar korenevskiy korenevskiy - edited - 27 Jul 2023
avatar korenevskiy korenevskiy - change - 27 Jul 2023
The description was changed
avatar korenevskiy korenevskiy - edited - 27 Jul 2023
avatar Fedik
Fedik - comment - 27 Jul 2023

This will not work, there were other PR somewhere around.

strftime format https://www.php.net/manual/en/function.strftime.php and DateTime format https://www.php.net/manual/en/datetime.format.php is incompatible.

avatar brianteeman
brianteeman - comment - 27 Jul 2023

Doesn't work - sorry

avatar wilsonge
wilsonge - comment - 27 Jul 2023

public static function strftimeFormatToDateFormat(string $strftimeformat)
It was the PR that introduced this very dodgy helper function (for better or worse)

avatar joomdonation
joomdonation - comment - 27 Jul 2023

It was the PR that introduced this very dodgy helper function (for better or worse)

@wilsonge That helper method is not used by Calendar Form Field, so it is unrelated. It was created to address the deprecated warnings HTMLHelper calendar in method with with the most common DateTime format parameters (until we a better solution).

avatar korenevskiy
korenevskiy - comment - 27 Jul 2023

in site https://www.php.net/manual/en/function.strftime.php
Below is a method to support the old code.
https://github.com/alphp/strftime/blob/master/src/php-8.1-strftime.php
This function is very large.


$format = HTMLHelper::strftimeFormatToDateFormat($this->format);
$this->value =  (new DateTime($this->value))->format($format);

In the corrected format, we use DateTime object. The function also converts the format to a new one for DateTime.

avatar korenevskiy
korenevskiy - comment - 27 Jul 2023

Let's create a static method strftime for the JDate class.

From https://github.com/alphp/strftime/blob/master/src/php-8.1-strftime.php
And let's call it obsolete - deprecated before Joomla 5
It's funny, but it's the right decision.

avatar joomdonation
joomdonation - comment - 27 Jul 2023

@korenevskiy The right solution is adding filterformat to your field

<field name="date_start" type="calendar" default="NOW" filterformat="Y-m-d"
label="MOD_MULTIMODULE_FIELD_FILTERDATESTART_LABEL"     showon="date_is:on"  
showtime="false"  singleheader="true" />   

Then it will work well. No warnings anymore.

avatar korenevskiy
korenevskiy - comment - 27 Jul 2023

Then it will work well. No warnings anymore.

But where is backward compatibility? The CALENDAR field should work without the filters attribute.

avatar joomdonation
joomdonation - comment - 27 Jul 2023

But where is backward compatibility? The CALENDAR field should work without the filters attribute.

It was PHP, not Joomla! introduced that backward incompatible change and Joomla! already provided you the solution with the undocumented filterformat attribute, you will just have to adapt your code to work with the changes introduced by PHP.

There is no reliable way to to convert all the format parameters supported by strftime to format parameters uses by DateTime class, so you cannot do the magic conversion (there are some format parameters in strftime has no equivalent format in DateTime class).

avatar korenevskiy
korenevskiy - comment - 27 Jul 2023

We need to ensure backward compatibility with older codes. At the same time, support PHP 8.2. At the same time, the format field should fully support backward compatibility, with full support strftime()
@joomdonation According to the PHP.net website. The function laid out with full support for the old function.
There's a link at the bottom of the page.

Let's create a static method strftime for the JDate class.

Everyone vote

avatar korenevskiy korenevskiy - change - 27 Jul 2023
Labels Added: bug
avatar korenevskiy
korenevskiy - comment - 27 Jul 2023

Colleagues, I added a static strftime() method to the Date class

This method works

$this->value = JDate::strftime($this->format, strtotime($this->value));
5f6e0d8 27 Jul 2023 avatar korenevskiy Fix
f1550a8 27 Jul 2023 avatar korenevskiy fix 2
1f07be3 27 Jul 2023 avatar korenevskiy Fix3
49ea15d 27 Jul 2023 avatar korenevskiy Fix 4
4544cd3 27 Jul 2023 avatar korenevskiy Fix 5
b95cee4 27 Jul 2023 avatar korenevskiy Fix 6
34b7d26 27 Jul 2023 avatar korenevskiy Fix 7
avatar korenevskiy
korenevskiy - comment - 27 Jul 2023

No matter how stupid this way of solving the problem would not be. This is the only way that meets all the requirements. There are no other ways.

avatar Fedik
Fedik - comment - 28 Jul 2023

@korenevskiy do not waste your time, re-implementing of strftime is not a correct way to go.

avatar korenevskiy
korenevskiy - comment - 28 Jul 2023

@Fedik

Of course that is wrong. For example, Joomla 5 should support PHP 8. Which means We can't release a release with an error popping up. There is another option to abandon the old format support. But it's also a backward compatibility violation. I wouldn't waste time if I didn't weigh the options. My option is very bad, BUT it is the only one that will allow you to maintain backward compatibility without errors. And already with the release of Joomla 6, this method can be removed.

There is another option, to pretend that nothing is happening. Meanwhile, 100% of Joomla 4 users on PHP 8.1 will display an error.

avatar korenevskiy
korenevskiy - comment - 28 Jul 2023

There is another option. We simply write the contents of this function into the CalendarField class. This will allow, as you say, not to create new functions. But even such a swollen Calendar FIeld is even worse than my version.

avatar korenevskiy
korenevskiy - comment - 28 Jul 2023

Each participant will believe that there will be a magic way of 3 lines that will solve all the problems, and will think that someone else will guess for sure. I read the contents of this function. And I will say that it will not be possible to replace it with 3 lines. There is absolutely no way to replace it.

avatar Fedik
Fedik - comment - 28 Jul 2023

Joomla 5 should support PHP 8. Which means We can't release a release with an error popping up

It works. It not an error, it a warning for developers.

avatar Fedik
Fedik - comment - 28 Jul 2023

A proper fix:

Introduce a new property datetime-format=""
The calendar class checks for this property, and when it exists then use it for value display and for value filter,
Othervise it continue to use strftime.

However, this is require changes in calendar.js, new lang constant, and review all our forms.
If you feels brave enough, you are welcome to do it.

Otherwise, any hacking around strftime is a waste of time.

avatar joomdonation
joomdonation - comment - 26 Aug 2023

Introduce a new property datetime-format="" The calendar class checks for this property, and when it exists then use it for value display and for value filter, Othervise it continue to use strftime.

@Fedik I think we have filterformat attribute for that purpose already https://github.com/joomla/joomla-cms/blob/4.3-dev/libraries/src/Form/Field/CalendarField.php#L238 . Shouldn't this PR be closed ?

avatar Fedik
Fedik - comment - 26 Aug 2023

I think we have filterformat attribute for that purpose

That for filter, I have suggest to use new attribute datetime-format="" for display, this should use DateTime style format instead of strftime.

But in general the Calendar need a bigger fix, see #41444
A deprecated strftime is a last thing I would worry ?

avatar HLeithner
HLeithner - comment - 30 Sep 2023

This pull request has been automatically rebased to 4.4-dev.

avatar HLeithner
HLeithner - comment - 6 Mar 2024

We discussed this PR in the maintainers meeting and would suggest the following.
include the function with composer based on https://github.com/alphp/strftime
and detect if the function exists in the bootstrap.php, if it's missing load the polyfill and include the function.

avatar bembelimen
bembelimen - comment - 27 Mar 2024

As Harald said,
please use the composer dependency and make a PR against 4.4

avatar bembelimen bembelimen - close - 27 Mar 2024
avatar bembelimen bembelimen - change - 27 Mar 2024
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2024-03-27 19:20:55
Closed_By bembelimen
Labels Added: Updates Requested PR-4.4-dev
Removed: PR-4.3-dev
avatar korenevskiy
korenevskiy - comment - 28 Mar 2024

As Harald said, please use the composer dependency and make a PR against 4.4

Explain what it means to make a dependency on the compositor in our case.

Add a Comment

Login with GitHub to post a comment