Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHP 8.1: strftime() is deprecated - Support CalendarField.php for PHP 8.1 #41266

Closed
wants to merge 12 commits into from

Conversation

korenevskiy
Copy link
Contributor

@korenevskiy korenevskiy commented Jul 27, 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.

PHP 8.1: strftime() is deprecated
@korenevskiy korenevskiy marked this pull request as ready for review July 27, 2023 13:48
@richard67
Copy link
Member

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

@Fedik
Copy link
Member

Fedik commented Jul 27, 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.

@brianteeman
Copy link
Contributor

Doesn't work - sorry

@wilsonge
Copy link
Contributor

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

@joomdonation
Copy link
Contributor

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

@korenevskiy
Copy link
Contributor Author

korenevskiy commented Jul 27, 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.

@korenevskiy
Copy link
Contributor Author

korenevskiy commented Jul 27, 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.

@joomdonation
Copy link
Contributor

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

@korenevskiy
Copy link
Contributor Author

korenevskiy commented Jul 27, 2023

Then it will work well. No warnings anymore.

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

@joomdonation
Copy link
Contributor

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

@korenevskiy
Copy link
Contributor Author

korenevskiy commented Jul 27, 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

@korenevskiy
Copy link
Contributor Author

korenevskiy commented Jul 27, 2023

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

This method works

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

@korenevskiy
Copy link
Contributor Author

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.

@Fedik
Copy link
Member

Fedik commented Jul 28, 2023

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

@korenevskiy
Copy link
Contributor Author

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

@korenevskiy
Copy link
Contributor Author

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.

@korenevskiy
Copy link
Contributor Author

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.

@Fedik
Copy link
Member

Fedik commented Jul 28, 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.

@Fedik
Copy link
Member

Fedik commented Jul 28, 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.

@Quy Quy added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Aug 21, 2023
@joomdonation
Copy link
Contributor

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 ?

@Fedik
Copy link
Member

Fedik commented Aug 26, 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 😉

@HLeithner HLeithner changed the base branch from 4.3-dev to 4.4-dev September 30, 2023 22:43
@HLeithner
Copy link
Member

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

korenevskiy

This comment was marked as spam.

korenevskiy

This comment was marked as duplicate.

korenevskiy

This comment was marked as duplicate.

@HLeithner
Copy link
Member

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.

@bembelimen
Copy link
Contributor

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

@bembelimen bembelimen closed this Mar 27, 2024
@korenevskiy
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug PR-4.4-dev Updates Requested Indicates that this pull request needs an update from the author and should not be tested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.