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

Is the plugin ready for new PHP 8.1? #1085

Closed
SeoDeveloper01 opened this issue Nov 28, 2021 · 16 comments
Closed

Is the plugin ready for new PHP 8.1? #1085

SeoDeveloper01 opened this issue Nov 28, 2021 · 16 comments
Labels
core Core functionalities, including the admin section severity: major Major functionality

Comments

@SeoDeveloper01
Copy link

I'm going to upgrade PHP on my server to 8.1, so I want to make sure everything goes smoothly.

@herrvigg
Copy link
Collaborator

herrvigg commented Nov 28, 2021

Usually it's fine with "minor" upgrades - though they call 8.1 a major upgrade, many new features.
If you do experiments in production it's at your own risk. Better try on dev sites first,

First look at the depreciations, should be fine:
https://www.php.net/releases/8.1/en.php#deprecations_and_bc_breaks

But here I found something with strftime they didn't report on the official page:
https://php.watch/versions/8.1/strftime-gmstrftime-deprecated

There is an advanced option to use strftime in QTX but it's not the default. So for those using this you'll be forced to switch. We'll have to remove that option.

@herrvigg herrvigg added the core Core functionalities, including the admin section label Nov 28, 2021
@alphp
Copy link
Contributor

alphp commented Jan 29, 2022

Deprecated: preg_split(): Passing null to parameter #3 ($limit) of type int is deprecated in wp-content\plugins\qtranslate-xt-3.11.4\qtranslate_core.php on line 832:
Fixed:

$vals = preg_split( '/[\s,]+/', strtolower( $ignore_file_types ), -1, PREG_SPLIT_NO_EMPTY );

For strftime you can use this gist: https://gist.github.com/bohwaz/42fc223031e2b2dd2585aab159a20f30

@herrvigg
Copy link
Collaborator

herrvigg added a commit that referenced this issue Jan 30, 2022
Passing null to parameter #3 ($limit) of type int is deprecated.
@herrvigg
Copy link
Collaborator

I don't recommend migrating to PHP 8.1 yet.

The fix for strftime might take more time. It's used in more places than expected. But I don't think we want to rewrite that function, we should make it more specific to every usage.

Also, there are so many deprecated notices triggered in WP core, even in 5.9. They say it's "just notices in debug mode, no impact on production, no need to panic". But in practice it's very annoying when for developers. It seems they had a huge list of PHP 8.1 deprecations such as Requests to be fixed but only a few of them made their way in WP 5.9.
For sure there are also many plugins and themes that need to be upgraded.

@alphp
Copy link
Contributor

alphp commented Jan 30, 2022

I'm doing the first impact assessments in test environments.
These tests allow me to detect incompatibilities with applications from other servers, such as databases.
It is in this scenario where I have tested the compatibility of WP with different plugins and themes that I use regularly.

@andreysneg
Copy link

Only 1 bug for php 8.1:

Deprecated: preg_split(): Passing null to parameter #3 ($limit) of type int is deprecated in D:_web\mix\wp-content\plugins\qtranslate-xt-master\qtranslate_core.php on line 832

$vals = preg_split( '/[\s,]+/', strtolower( $ignore_file_types ), null, PREG_SPLIT_NO_EMPTY );

Simple change NULL to "0" (zero)

@herrvigg
Copy link
Collaborator

@andreysneg All the preg_split have been fixed in this commit: 6db0f3a

But strftime is still used and that should generate some deprecated warnings in PHP 8.1.

@herrvigg
Copy link
Collaborator

herrvigg commented Sep 11, 2022

So back to the PHP 8.1 compatibility.
The remaining problem is strftime. There are options in the settings to use strftime or date format, but the code is always falling back to strftime, even if a format is given in the date fashion.
All the conversions must be done the other way around, using IntlDateFormatter as done here.
This is a blocker for PHP 8.1 and it becomes the main priority.

@herrvigg
Copy link
Collaborator

It's been several days I'm looking at this code in qtranslate_date_time.php and still doesn't understand it fully... Such an incredible mess. It's a real nightmare to refactor.

@herrvigg
Copy link
Collaborator

herrvigg commented Sep 18, 2022

These functions are so old that some functionalities don't make any sense to me, like allowing a format containing ML values... I'd like to simplify this but it's hard to understand the impact in the user code.

I start to have a plan in mind, at least to allow PHP 8.1 to work without warnings on the short run. This will also serve as an experimental API before rolling it out for all. After that there will surely be breaking changes in the legacy API because it's really a mess to maintain.

@herrvigg
Copy link
Collaborator

I have now merged a first workaround with #1228 specifically for PHP 8.1 (and above).
This keeps all the existing formats in place, I may have missed something because the legacy code is extremely hard to read but this is supposed to keep the current behavior.

However, it's clear we should abandon the strftime formats completely in qTranslate and only use a standard date format using IntlDateFormatter. We should store the language date and time configuration only in this date format. I will open a new topic because this has several implications.

From now I ask everyone interested in PHP8.1 to test the last version with the master branch directly.

@herrvigg herrvigg added the help wanted Extra attention is needed label Sep 25, 2022
@rein2000
Copy link

rein2000 commented Sep 27, 2022

I received an error notice from Wordpress today, possibly related?

Fehler-Details

Ein Fehler vom Typ E_ERROR wurde in der Zeile 167 der Datei
EDITED/wordpress/wp-content/plugins/qtranslate-xt/qtranslate_date_time.php
verursacht. Fehlermeldung: Uncaught InvalidArgumentException: Format
"%L" is unknown in time format in
EDITED/wordpress/wp-content/plugins/qtranslate-xt/qtranslate_date_time.php:167

{
"PHP_VERSION": "8.1.10",
"WP_VERSION": "6.0.2",
"QTX_VERSION": "3.12.1",
"Plugins": [
"Advanced Custom Fields PRO 6.0.0",
"Age Gate 3.0.7",
"Billbee - Auftragsabwicklung, Warenwirtschaft, Automatisierung 1.0.0",
"Check & Log Email 1.0.6",
"Classic Editor 1.6.2",
"Contact Form 7 5.6.3",
"Disable Comments 2.4.2",
"Yoast Duplicate Post 4.5",
"Git Updater 11.1.1.3",
"Health Check & Troubleshooting 1.5.0",
"Mailchimp for WooCommerce 2.7.5",
"One Stop Shop for WooCommerce 1.3.0",
"qTranslate-XT 3.12.1",
"Simple Custom Post Order 2.5.6",
"User Role Editor 4.63.1",
"vendidero Helper 2.1.6",
"WooCommerce Estimated Shipping Date 4.2.0",
"WooCommerce Blocks 8.5.1",
"WooCommerce Stripe Gateway 6.7.0",
"Germanized for WooCommerce Pro 3.5.7",
"Germanized for WooCommerce 3.10.6",
"WooCommerce Menu Cart 2.12.0",
"WooCommerce PayPal Payments 1.9.3",
"WooCommerce Quantity Increment 1.1.0",
"WooCommerce 6.9.4",
"WP Mail SMTP 3.5.2",
"WP Migrate 2.4.0",
"WPForms Lite 1.7.6"
]
}

@herrvigg
Copy link
Collaborator

Uncaught InvalidArgumentException: Format "%L" is unknown in time format

@rein2000 ah nice, this is useful.
It turns out that %L is not a standard strftime format. So the error makes sense.
But... if you go back to PHP 8.0 it was converted in the legacy qTranslate code to date format G -> "24-hour format of an hour without leading zeros (0 through 23)".

Solutions:
To avoid adding new hacks, it's better to replace %L with %k which is an official format in strftime -> "Hour in 24-hour format, with a space preceding single digits" (0-23). There might be small variations with the spaces.
If you prefer the version with leading zeros you can use %H (00-23).

References

With the last patch I pushed, the strformat formats are still accepted in the settings but they are converted to date at runtime using IntlDateFormatter with PHP 8.1, but it's just a temporary solution.
Note the next step will be to convert everything to date as a native format in qtranslate. We should not store or use strftime formats but for now you can keep it like this.
Yes - it's quite complicated.

@herrvigg
Copy link
Collaborator

herrvigg commented Oct 2, 2022

@rein2000 did you try replacing %L with %k?

herrvigg added a commit that referenced this issue Oct 16, 2022
Some formats like '%E' are not standard in strftime, but they have
been added in qTranslate to mimic some date formats.
Complete the conversion tables in the new `qxtranxf_intl_strftime`
to support those, for retro-compatibility. Anyway this function is
temporary, as strftime formats should not be used anymore.

Reorder the mappings in the original converter to better separate
non-standard pairs. Note that three of them are not supported in the
new function.
@herrvigg
Copy link
Collaborator

herrvigg commented Oct 16, 2022

I have uploaded a new patch that completes the support of non-standard formats like %L, there were many others...
The first value is for date, the second for strftime. The purpose was to mimic the date formats that don't exist in strftime.

'j' => '%E', // Day number no zero
'S' => '%q', // Day english ordinal
'w' => '%f', // Week number
'T' => '%v', // Timezone abbreviation, if known; otherwise the GMT offset.
'n' => '%i', // Numeric representation of a month, without leading zeros
't' => '%J', // Number of days in the given month
'B' => '%K', // Swatch internet time 000-999
'G' => '%L', // 24-hour format of an hour without leading zeros --> %L should be %k!
'u' => '%N', // Microseconds
'e' => '%Q', // Timezone identifier
'I' => '%o', // 1 if Daylight Saving Time, 0 otherwise.
'O' => '%O', // Difference to Greenwich time (GMT) without colon between hours and minutes
'Z' => '%1', // Timezone offset in seconds
'c' => '%2', // ISO 8601 date
'r' => '%3', // RFC 2822/» RFC 5322 formatted date
'U' => '%4'  // Seconds since the Unix Epoch

So these are supported, but ideally we should not use them because it's not standard.

Even worse, there are three pairs that override the strftime format with something specific to qTranslate.

'z' => '%F', // z: The day of the year (starting from 0) -- %F: Same as "%Y-%m-%d
'P' => '%s', // P: Difference to Greenwich time (GMT) with colon between hours and minutes -- %s: unix timestamp
'L' => '%k', // L: leap year -- %k: Hour in 24-hour format, single digit

So the conversions are inconsistent 😕 and we should not override them in the new qxtranxf_intl_strftime. Therefore these last three won't be supported anymore. Hopefully they are so specific that it should not be real problem.

It should now be possible to use PHP 8.1 in a (almost) transparent manner, thanks to this workaround for strftime that is connected to the current code. However I consider this only as a temporary solution. We should abandon the strftime formats completely and therefore migrate to date but this requires other changes. I will open a new post.

@herrvigg
Copy link
Collaborator

I have now generalized the usage of qtranxf_intl_strftime to any PHP version, not just PHP 8.1.
Merged in master, please test this so we can release it.

@herrvigg herrvigg added severity: major Major functionality and removed help wanted Extra attention is needed labels Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core functionalities, including the admin section severity: major Major functionality
Projects
None yet
Development

No branches or pull requests

5 participants