Remove long-deprecated mbstring INI settings #12445

alexdowad · 2023-10-15T18:58:11Z

@Girgias very kindly opened a PR for the same issue a few years ago: #5334. Apparently, the INI settings mbstring.http_input, mbstring.http_output, and mbstring.internal_encoding were deprecated in PHP 5.6 but have never been completely removed yet. It seems that it is high time to remove them.

I just reviewed the PR from @Girgias, and discovered that it makes some changes which are not needed and which will definitely cause BC breaks. This is the reason why the changes in #5334 are not OK:

The internal mbstring functionality which could previously be accessed by (for example) setting mbstring.http_input was not accessible solely in that way. Rather than using mbstring.http_input, you could also set the INI setting input_encoding, and it would do the same thing. Likewise for mbstring.http_output; the same functionality could also be activated using the function mb_http_output. Or for mbstring.internal_encoding, you could call the function mb_internal_encoding.

Since none of these features (such as the function mb_internal_encoding) are deprecated, we cannot remove any of the internal mbstring code which helps to implement them. All we can remove is the code which directly handles reads/writes to the obsolete INI settings.

Some thorough review would really be appreciated here. Also, I would like to ask what section of UPGRADING it would be best to note this removal in. Or do we normally put a note in UPGRADING when deprecated features are removed?

@nikic @Girgias @cmb69 @bukka @kamil-tekiela @youkidearitai

alexdowad · 2023-10-15T19:16:00Z

Sorry, just to give credit where credit is due... I am looking through the code from @Girgias again and it doesn't have as many problems as I first thought. 👍🏻

I do think this PR is a little bit better though.

kamil-tekiela · 2023-10-15T19:24:34Z

In my opinion, all of it should be removed in PHP 9.0. But first all connected functions should be properly deprecated. Removing it in 8.4 is not recommended. If someone is still using it, it could prove to be a serious BC. Such removals should only be done in major releases. But otherwise I am in favor of nuking this whole feature.

…

On Sun, Oct 15, 2023, 21:16 Alex Dowad ***@***.***> wrote: Sorry, just to give credit where credit is due... I am looking through the code from @Girgias <https://github.com/Girgias> again and it doesn't have as many problems as I first thought. 👍🏻 I do think this PR is a little bit better though. — Reply to this email directly, view it on GitHub <#12445 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABSHGGB2ANLAF32NUDXLLS3X7QY73AVCNFSM6AAAAAA6BGGKVGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRTGQ3TSNJVG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Girgias

We probably should also formally deprecate mb_http_input(), mb_http_output(), mb_internal_encoding (), and other functions which tweak the INI settings.

IIRC the reason we didn't remove them in 8.0, is that until 7.4 MBString and Iconv didn't actually use the global INI settings.

Girgias · 2023-10-15T20:13:50Z

ext/mbstring/tests/bug48697.phpt

@@ -4,7 +4,6 @@ Bug #48697 (mb_internal_encoding() value gets reset by parse_str() or mb_parse_s
 mbstring
 --FILE--
 <?php
-ini_set('mbstring.internal_encoding', 'ISO-8859-15');
 ini_set('mbstring.encoding_translation', true);


Unrelated: This could use the INI PHPT section to set the INI config

Girgias · 2023-10-15T20:17:04Z

ext/standard/tests/strings/htmlentities25.phpt

@@ -3,7 +3,6 @@ htmlentities() should not be influenced by mb_internal_encoding()
 --INI--
 default_charset=


This test can also be deleted.

Girgias · 2023-10-15T20:21:10Z

ext/mbstring/mbstring.h

 	bool internal_encoding_set;
 	bool http_output_set;
-	bool http_input_set;


Why are the two other fields not removed?

Because while http_input_set was only written from the handler for writes to mbstring.http_input, http_output_set (for example) is also written by the function mb_http_output. Same for internal_encoding_set.

ACK, we probably should deprecate the functions that affect those fields.

ext/mbstring/mbstring.c

Girgias · 2023-10-15T20:27:13Z

Also, what's the point of mb_language() and the corresponding INI setting?

alexdowad · 2023-10-15T20:30:01Z

Also, what's the point of mb_language() and the corresponding INI setting?

I'd have to check the code to refresh my memory on everything which is affected by mb_language(), but just from memory, one of the things which is affected is the default behavior of mb_detect_encoding if you don't specify what the candidate encodings which you want to "detect" are.

Girgias · 2023-10-15T20:35:13Z

Also, what's the point of mb_language() and the corresponding INI setting?

I'd have to check the code to refresh my memory on everything which is affected by mb_language(), but just from memory, one of the things which is affected is the default behavior of mb_detect_encoding if you don't specify what the candidate encodings which you want to "detect" are.

Right, from a quick glance at the docs it also seems to affect mb_send_mail() ? But that seems strange?
I think getting rid of this INI setting is probably a good thing.

derickr · 2023-10-15T21:40:13Z

Guessing ("detecting") a text's encoding is an inexact science and potentially unsafe. It should never have been a "feature".

youkidearitai · 2023-10-15T23:29:21Z

It should never have been a "feature".

I agree.

By the way, mb_internal_encoding() is affected mbstring.internal_encoding. My understanding, this PR not modify it is. mb_internal_encoding() is use it often. Should I change mbstring.internal_encoding in mb_internal_encoding to default_charset?

Girgias · 2023-10-16T12:31:37Z

It should never have been a "feature".

I agree.

By the way, mb_internal_encoding() is affected mbstring.internal_encoding. My understanding, this PR not modify it is. mb_internal_encoding() is use it often. Should I change mbstring.internal_encoding in mb_internal_encoding to default_charset?

Yes, either the default_charset or internal_encoding INI settings should be used instead.
Similarly for the input/output encodings one can use input_encoding or output_encoding

youkidearitai · 2023-10-16T14:42:27Z

@Girgias Thank you for your addition.

I was worried about mb_internal_encoding why is because I often used this function to set the desired character encoding, At least old (legacy) PHP code.
This is confused that searched google "mb_internal_encoding" in Japanese.

Anyway, using input_encoding and output_encoding is correct.

alexdowad · 2023-10-17T16:54:48Z

Dear @Girgias, @derickr, @youkidearitai, @kamil-tekiela... all of your comments are very helpful.

Do you think we want to keep mb_output_handler or not? (https://www.php.net/manual/en/function.mb-output-handler.php)

kamil-tekiela · 2023-10-17T18:42:58Z

I don't want to keep it. But just because I can't see any reasonable use for it, doesn't mean there isn't one. It would be good to discuss in internals.

…

On Tue, Oct 17, 2023, 18:55 Alex Dowad ***@***.***> wrote: Dear @Girgias <https://github.com/Girgias>, @derickr <https://github.com/derickr>, @youkidearitai <https://github.com/youkidearitai>, @kamil-tekiela <https://github.com/kamil-tekiela>... all of your comments are very helpful. Do you think we want to keep mb_output_handler or not? ( https://www.php.net/manual/en/function.mb-output-handler.php) — Reply to this email directly, view it on GitHub <#12445 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABSHGGEY3LZG5OCDFF4FYOTX72Z6LAVCNFSM6AAAAAA6BGGKVGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRWHAYTCNJXG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Girgias · 2023-10-18T00:14:00Z

mb_output_handler

Let me ask @dseguy to see if this is actually used.

Girgias · 2023-10-23T14:00:29Z

Dear @Girgias, @derickr, @youkidearitai, @kamil-tekiela... all of your comments are very helpful.

Do you think we want to keep mb_output_handler or not? (https://www.php.net/manual/en/function.mb-output-handler.php)

Forgot to update the PR, but @dseguy looked into it and there are 0 occurrences of this in close to 3000 projects, ranging from composer packages, large open source projects and private code bases. So I think this is safe to put on the chopping board.

cmb69 · 2024-07-11T22:00:37Z

Removing it in 8.4 is not recommended. If someone is still using it, it could prove to be a serious BC. Such removals should only be done in major releases.

I fully agree, and would like to put a PHP 9 milestone onto this PR. That might help a bit to not forget about this removal again. Or would it be better to track that in the Wiki (or elsewhere)?

Girgias · 2024-07-12T14:33:15Z

I created the milestone and added the PR to it.

alexdowad added 2 commits October 15, 2023 20:46

Remove deprecated INI setting mbstring.http_input

57d2042

Remove deprecated INI setting mbstring.http_output

c4b7064

alexdowad requested a review from bukka as a code owner October 15, 2023 18:58

github-actions bot added Extension: mbstring Extension: standard labels Oct 15, 2023

Remove deprecated INI setting mbstring.internal_encoding

e144123

alexdowad force-pushed the depr branch from 6317f3c to e144123 Compare October 15, 2023 19:10

Girgias reviewed Oct 15, 2023

View reviewed changes

Girgias mentioned this pull request Oct 15, 2023

Remove deprecated mbstring INI settings #5334

Closed

Girgias added this to the PHP 9 milestone Jul 12, 2024

quicksketch mentioned this pull request Oct 28, 2024

Remove deprecated mbstring.http_input and mbstring.http_output from .htaccess backdrop/backdrop-issues#6734

Closed

		@@ -3,7 +3,6 @@ htmlentities() should not be influenced by mb_internal_encoding()
		--INI--
		default_charset=

Remove long-deprecated mbstring INI settings #12445

Are you sure you want to change the base?

Remove long-deprecated mbstring INI settings #12445

Uh oh!

Conversation

alexdowad commented Oct 15, 2023

Uh oh!

alexdowad commented Oct 15, 2023

Uh oh!

kamil-tekiela commented Oct 15, 2023 via email

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Girgias Oct 15, 2023

Choose a reason for hiding this comment

Uh oh!

Girgias Oct 15, 2023

Choose a reason for hiding this comment

Uh oh!

Girgias Oct 15, 2023

Choose a reason for hiding this comment

Uh oh!

alexdowad Oct 15, 2023

Choose a reason for hiding this comment

Uh oh!

Girgias Oct 15, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Girgias commented Oct 15, 2023

Uh oh!

alexdowad commented Oct 15, 2023

Uh oh!

Girgias commented Oct 15, 2023

Uh oh!

derickr commented Oct 15, 2023 via email

Uh oh!

youkidearitai commented Oct 15, 2023

Uh oh!

Girgias commented Oct 16, 2023

Uh oh!

youkidearitai commented Oct 16, 2023

Uh oh!

alexdowad commented Oct 17, 2023

Uh oh!

kamil-tekiela commented Oct 17, 2023 via email

Uh oh!

Girgias commented Oct 18, 2023

Uh oh!

Girgias commented Oct 23, 2023

Uh oh!

cmb69 commented Jul 11, 2024

Uh oh!

Girgias commented Jul 12, 2024

Uh oh!

Uh oh!