Add PHP 8.1 compatibility #678

atomiix · 2021-10-07T17:45:16Z

The goal of this PR is to make smarty compatible with PHP 8.1.

The main issue was the use of the function strftime() which is deprecated in PHP 8.1. This problem is solve by the use of IntlDateFormatter after being sure that the intl extension is loaded.

Other deprecations have been removed too.

wisskid · 2021-10-13T09:53:10Z

Hi @atomiix this looks great! However, I've been working on feature/php8-support to support PHP8.0 already.
In this branch, I switched from travis to gitlab-ci, added PHPUnit 8.5. I already rewrote all the expectException stuff too.

Could you rebase your PR on this branch (or wait until it is in master)?

Reviewing your PR, I see a couple of changes that do not seem to be explained by support for PHP8.1. E.g. the strtlower in libs/sysplugins/smarty_internal_method_loadplugin.php:87. Could you clarify those?

Thanks for your work!

atomiix · 2021-10-20T14:12:55Z

hey @wisskid, I rebased my PR, it should be good now!

I also edited my description to explain why I added the strtolower in libs/sysplugins/smarty_internal_method_loadplugin.php

atomiix · 2021-11-23T11:09:52Z

@wisskid just a friendly reminder as PHP 8.1 due date is in 2 days 🙃

Progi1984 · 2021-11-25T15:37:01Z

Good news. PHP 8.1 is available ;)

wisskid

Great PR by the way, 2 comments made

libs/sysplugins/smarty_internal_method_loadplugin.php

wisskid · 2021-11-25T21:57:28Z

libs/plugins/function.html_select_date.php

+
+        $formatter = null;
+        $format_compare = '%m';
+        if (class_exists('IntlDateFormatter')) {


Since IntlDateFormatter isn't included by default and the strftime deprecation notice is only a notice, wouldn't it be better to keep it as it was? We're adding complexity here, only fixing the deprecation notices on certain environments. I'm hoping PHP9 will include a way to handle this smoothly that is included by default?

Unfortunately, I don't think that PHP 9.0 will handle it smoothly, from https://wiki.php.net/rfc/deprecations_php_8_1:

The strftime() and gmstrftime() functions exhibit similar issues as strptime(), in that the formats they support, as well as their behavior, is platform-dependent. Unlike strptime(), these functions are available on Windows, though with a different feature set than on Linux. Musl-based distributions like Alpine do not support timezone-related format specifiers correctly. These functions are also locale-based, and as such may exhibit thread-safety issues.

Once again date() or DateTime::format() provide portable alternatives, and IntlDateFormatter::format() provides a more sophisticated, localization-aware alternative.

The proposal is to deprecate strftime() and gmstrftime() in favor of these alternatives.

So I guess this is the way we'll have to do it in PHP 9.0 and in the meantime, we can have strftime as a fallback. WDYT?

Let me put it in another way: your change will still fail in PHP9, because strftime will be deleted by then (unless they reverse the decision). So what is the point of making the change in the first place?
Also: I know strftime is used a LOT in PHP libs, so the deprecation will trigger the need for a suitable alternative to be included in PHP9 by default. If that turns out to be IntlDateFormatter, we'll switch then.

I agree with @atomiix , having strftime as fallback is a good idea. I'm not in favor of having deprecation notices only because it's acceptable. When you're working you don't want to be spamed by useless messages you can avoid.
Adding IntlDateFormatter as alternative is a good idea, but the real check should be if (extension_loaded('intl')) { for a better understanding.

I also think avoiding any strftime would be beneficial. The intl extension is quite common nowadays, and Symfony has a polyfill (symfony/polyfill-intl-icu) which is an easy alternative if the intl extension is missing and provides a way of avoiding deprecation notices even without the extension.

pineapps · 2021-12-07T03:06:57Z

So are we going to merge this branch or.....?

PierreRambaud · 2021-12-07T07:49:01Z

So are we going to merge this branch or.....?

Maybe, maybe not, that's a hard question 🤔
Let the maintainer take time to estimate if the change is legit and can be accepted or not 😺

j-applese3d · 2021-12-21T19:20:40Z

I noticed that modifier.date_format also uses strftime.
Would it make sense to also update that plugin at the same time?

PierreRambaud · 2021-12-22T08:26:36Z

I noticed that modifier.date_format also uses strftime. Would it make sense to also update that plugin at the same time?

I think yes, but the whole part needs to be refactored:

smarty/libs/plugins/modifier.date_format.php

Line 51 in cd96228

    
           if ($formatter === 'strftime' || ($formatter === 'auto' && strpos($format, '%') !== false)) {

atomiix · 2021-12-22T13:53:12Z

@j-applese3d you're right! I just added a new modifier date_format_intl. I guess that's a good compromise to avoid a BC break and still allow us to use IntlDateFormatter when needed.

PierreRambaud · 2021-12-22T15:27:28Z

@j-applese3d you're right! I just added a new modifier date_format_intl. I guess that's a good compromise to avoid a BC break and still allow us to use IntlDateFormatter when needed.

Not sure it's the right solution as the date_format will still be used.
Maybe you can do the same condition inside the current implementation as you did before, like "Is Intl available, yes, let's use it". And use it by default.

atomiix · 2021-12-22T15:52:16Z

Maybe you can do the same condition inside the current implementation as you did before, like "Is Intl available, yes, let's use it". And use it by default.

The issue is that IntlDateFormatter cannot be 100% compatible with strftime. You can see in this gist that it's not 100% correct.

IMHO, for date_format, as you can fully customize the format, it's better to have a dedicated method

wisskid · 2021-12-22T22:05:40Z

I like the idea of combining IntlDateFormatter with symfony/polyfill-intl-icu and replace all the strftime in the entire codebase.

atomiix · 2021-12-22T22:32:51Z

@wisskid We'll have to keep in mind that symfony/polyfill-intl-icu is limited to english though. That being said, that's ok for me. Also, that means BC breaks as strftime format string is different than IntlDateFormatter's ones and not completely convertisable.

iwiznia · 2021-12-30T22:01:58Z

I am working on a migration of a project to PHP 8.1 and can confirm that this PR solves all the problems I've been seeing.

genyslt · 2022-01-05T11:42:23Z

Any news?

wisskid · 2022-01-10T12:01:39Z

I'm thinking we should separate the fixes for the fatal error on PHP8.1(#671) from the removal of deprecation notices strftime. The former is much more pressing now.

genyslt · 2022-01-13T06:23:22Z

Can you tell us estimated time, when you can fix it? Is there a lot of problems to run Smarty on php8.1?

thirsch · 2022-01-17T12:56:50Z

Looks like there is not much missing to get 8.1 support. Is there anything we can do to speed up the merge/release? Maybe creating another PR with all changes except the strftime-issue?

wisskid · 2022-01-17T22:51:09Z

Maybe creating another PR with all changes except the strftime-issue?

Yes that would be super. I'd merge that.

thirsch · 2022-01-18T10:36:30Z

Maybe creating another PR with all changes except the strftime-issue?

Yes that would be super. I'd merge that.

Okay, I've created another pr for only the non-intldate related stuff: #713

scortellino · 2022-01-19T15:22:53Z

There are 3 PR on PHP 8.1...Do we expect a merge soon like in coming days?

wisskid · 2022-01-21T07:25:15Z

Closing this one in favor of #713
We'll handle the strftime issue later

atomiix force-pushed the php81 branch from 8900e51 to 8a566ae Compare October 8, 2021 08:08

wisskid added the waiting Waiting for answer label Oct 13, 2021

This was referenced Oct 13, 2021

strftime deprecated in PHP 8.1 #672

Closed

Smarty is not compatible with PHP 8.1 #671

Closed

atomiix force-pushed the php81 branch from 8a566ae to 10ded86 Compare October 20, 2021 14:03

atomiix mentioned this pull request Oct 28, 2021

PrestaShop must be compatible with PHP 8.0 PrestaShop/PrestaShop#18547

Closed

5 tasks

wisskid requested changes Nov 25, 2021

View reviewed changes

atomiix added 3 commits November 26, 2021 11:43

PHP 8.1 compatibility

da76d92

Fix phpunit warning

e55da19

Add PHP 8.1 tests

c6055f6

atomiix force-pushed the php81 branch from 10ded86 to c6055f6 Compare November 26, 2021 10:43

marclaporte mentioned this pull request Dec 1, 2021

Support policy of Smarty 3.x vs 4.x #686

Closed

azett mentioned this pull request Dec 18, 2021

strftime() is deprecated as of PHP 8.1 flatpressblog/flatpress#92

Closed

Add new date_format_intl modifier

6c1e061

This was referenced Jan 12, 2022

PHP 8.1 deprecation notices in demo/plugins/cacheresource.pdo.php #706

Closed

With PHP 8.1.0 : Deprecated: Implicit conversion from float 14.5 to int loses precision in Smarty/libs/plugins/modifier.truncate.php on line 45 and 46 and 59 #699

Closed

thirsch mentioned this pull request Jan 18, 2022

php 8.1 compatibility #711

Closed

wisskid closed this Jan 21, 2022

thirsch mentioned this pull request Jan 21, 2022

PHP 8.1 compatibility (without IntlDateFormatter) #713

Merged

eternoendless mentioned this pull request Mar 11, 2022

Make PrestaShop compatible with PHP 8.1 PrestaShop/PrestaShop#27888

Closed

9 tasks

mfettig mentioned this pull request May 3, 2022

address PHP 8.1 'explode', 'number_format', and 'replace' deprecations #755

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PHP 8.1 compatibility #678

Add PHP 8.1 compatibility #678

atomiix commented Oct 7, 2021 •

edited

wisskid commented Oct 13, 2021

atomiix commented Oct 20, 2021

atomiix commented Nov 23, 2021

Progi1984 commented Nov 25, 2021

wisskid left a comment

wisskid Nov 25, 2021

atomiix Nov 26, 2021

wisskid Nov 26, 2021

PierreRambaud Nov 29, 2021

iquito Nov 30, 2021

pineapps commented Dec 7, 2021 •

edited

PierreRambaud commented Dec 7, 2021

j-applese3d commented Dec 21, 2021

PierreRambaud commented Dec 22, 2021

atomiix commented Dec 22, 2021

PierreRambaud commented Dec 22, 2021

atomiix commented Dec 22, 2021

wisskid commented Dec 22, 2021

atomiix commented Dec 22, 2021

iwiznia commented Dec 30, 2021

genyslt commented Jan 5, 2022

wisskid commented Jan 10, 2022

genyslt commented Jan 13, 2022

thirsch commented Jan 17, 2022

wisskid commented Jan 17, 2022

thirsch commented Jan 18, 2022 •

edited

scortellino commented Jan 19, 2022

wisskid commented Jan 21, 2022

Add PHP 8.1 compatibility #678

Add PHP 8.1 compatibility #678

Conversation

atomiix commented Oct 7, 2021 • edited

wisskid commented Oct 13, 2021

atomiix commented Oct 20, 2021

atomiix commented Nov 23, 2021

Progi1984 commented Nov 25, 2021

wisskid left a comment

Choose a reason for hiding this comment

wisskid Nov 25, 2021

Choose a reason for hiding this comment

atomiix Nov 26, 2021

Choose a reason for hiding this comment

wisskid Nov 26, 2021

Choose a reason for hiding this comment

PierreRambaud Nov 29, 2021

Choose a reason for hiding this comment

iquito Nov 30, 2021

Choose a reason for hiding this comment

pineapps commented Dec 7, 2021 • edited

PierreRambaud commented Dec 7, 2021

j-applese3d commented Dec 21, 2021

PierreRambaud commented Dec 22, 2021

atomiix commented Dec 22, 2021

PierreRambaud commented Dec 22, 2021

atomiix commented Dec 22, 2021

wisskid commented Dec 22, 2021

atomiix commented Dec 22, 2021

iwiznia commented Dec 30, 2021

genyslt commented Jan 5, 2022

wisskid commented Jan 10, 2022

genyslt commented Jan 13, 2022

thirsch commented Jan 17, 2022

wisskid commented Jan 17, 2022

thirsch commented Jan 18, 2022 • edited

scortellino commented Jan 19, 2022

wisskid commented Jan 21, 2022

atomiix commented Oct 7, 2021 •

edited

pineapps commented Dec 7, 2021 •

edited

thirsch commented Jan 18, 2022 •

edited