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

[4.2] PHP8.2 Replace deprecated utf8 encode/decode to mb string #39583

Merged
merged 7 commits into from Jan 14, 2023

Conversation

laoneo
Copy link
Member

@laoneo laoneo commented Jan 9, 2023

Pull Request for Issue #38258 and partially for #38922.

Summary of Changes

This pr replaces the few calls to utf8_encode/utf8_decode to mbstring. It uses hardcoded the latin 1 character set (ISO-8859-1).

[Update]
It also makes the mbstring polyfill an explicit dependency, so we ensure that it still works on hosts, which do not have the mbstring module loaded.

If we should support all, then 'ISO-8859-1' can be replaced with mb_list_encodings(). See comment below why not!

Testing Instructions

Login in the frontend, and go to the profile user page.

Actual result BEFORE applying this Pull Request

A deprecated warning is shown.

Expected result AFTER applying this Pull Request

No deprecated warning is shown.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@nikosdion
Copy link
Contributor

If we should support all, then 'ISO-8859-1' can be replaced with mb_list_encodings().

No, we absolutely shouldn't!

The reason utf8_decode was deprecated is that it only ever supported ISO-8859-1 as per https://wiki.php.net/rfc/remove_utf8_decode_and_utf8_encode in the Introduction at the very top of the page (emphasis mine):

The built-in functions utf8_encode and utf8_decode convert strings encoded in ISO-8859-1 (“Latin 1”) to and from UTF-8, respectively. While this is sometimes a useful feature, they are commonly misunderstood

I am unwilling to consider this PR a good solution because mbstring is actually not a hard requirement for Joomla 4 per the published technical requirements. I know it's nonsensical and probably a mistake on that page.

Unless that page is amended to list mbstring as a hard requirement it makes far more sense to follow the approach in #38922 with a function which uses whatever is actually available — including a fallback to a pure PHP implementation. Do note that the code in #38922 is in fact following the “Throwing the kitchen sink at it” section of the PHP RFC for utf8_decode deprecation, i.e. the “official” way PHP core developers want us to polyfill this deprecated feature. It also uses utf8_decode on older PHP versions to remove surprises, so there's that.

As a second stage we should look at every instance of utf8_decode and decide if it actually makes sense to be doing that (most of that code came from early PHP 5 versions which didn't support UTF8 natively) and whether we actually meant to use ISO-8859-1 (e.g. when handling URLs that's deliberate) instead of blindly trying to use whatever encoding. I believe that in most cases we'll just end up removing the decoding altogether.

@laoneo
Copy link
Member Author

laoneo commented Jan 9, 2023

I really like the idea to recheck if the code still makes sense and then remove what is not really needed. For now to improve compatibility I would prefer to replace the deprecated functions with alternatives.

From a maintenance perspective I would not add two helper functions which do basically replace 5 native function calls, even when it requires to change the compatibility requirements.

@nikosdion
Copy link
Contributor

@laoneo I was thinking of a one-two approach. First add a drop-in replacement marked as @internal and @deprecated 5.0, then go through the (few, to be fair) instances where we use that function, thus removing the need for the drop-in replacement and the drop-in replacement itself.

This would be perfectly viable with regards to semantic versioning as @internal clearly communicates “thou shalt not use” to 3PDs and @deprecated sets the expectation for dropping the darned thing on a very short horizon.

Why do that instead of just doing the second step? To make sure that real world use of the replacement doesn't cause any issues in use cases we have never thought of or could have expected just by looking at the code. I am a bit conservative here but I have seen… THINGS 🤣

@laoneo
Copy link
Member Author

laoneo commented Jan 11, 2023

We have been discussing this in maintainers and in #38922. Making the mbstring polyfill an explicit dependency and converting the core code to mbstring is the least disruptive change to improve PHP 8.2 compatibility in the 4.x series.

@laoneo laoneo added the PHP 8.x PHP 8.x deprecated issues label Jan 12, 2023
@laoneo laoneo changed the title Replace deprecated utf8 encode/decode to mb string [4.2] PHP8.2 Replace deprecated utf8 encode/decode to mb string Jan 12, 2023
@carlitorweb
Copy link
Member

I have tested this item ✅ successfully on 68ee206

For test it. Login in the frontend, and go to the profile user page


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39583.

@joomdonation
Copy link
Contributor

I have tested this item ✅ successfully on f3e4c20


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39583.

@joomdonation
Copy link
Contributor

I restored test result from @carlitorweb because it was lost for some reasons. RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39583.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 13, 2023
@roland-d roland-d merged commit 72af961 into joomla:4.2-dev Jan 14, 2023
@roland-d roland-d deleted the php82/utf8 branch January 14, 2023 11:01
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 14, 2023
@roland-d
Copy link
Contributor

Thank you

@roland-d roland-d added this to the Joomla! 4.2.7 milestone Jan 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants