| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
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. |
Sorry, something went wrong.
|
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:
***@***.***>
|
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| --FILE-- | ||
| <?php | ||
| ini_set('mbstring.internal_encoding', 'ISO-8859-15'); | ||
| ini_set('mbstring.encoding_translation', true); |
There was a problem hiding this comment.
Unrelated: This could use the INI PHPT section to set the INI config
Sorry, something went wrong.
| --TEST-- | ||
| htmlentities() should not be influenced by mb_internal_encoding() | ||
| --INI-- | ||
| default_charset= |
There was a problem hiding this comment.
This test can also be deleted.
Sorry, something went wrong.
| bool internal_encoding_set; | ||
| bool http_output_set; | ||
| bool http_input_set; |
There was a problem hiding this comment.
Why are the two other fields not removed?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
ACK, we probably should deprecate the functions that affect those fields.
Sorry, something went wrong.
|
Also, what's the point of mb_language() and the corresponding INI setting? |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
Right, from a quick glance at the docs it also seems to affect mb_send_mail() ? But that seems strange? |
Sorry, something went wrong.
|
Guessing ("detecting") a text's encoding is an inexact science and potentially unsafe.
It should never have been a "feature".
|
Sorry, something went wrong.
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? |
Sorry, something went wrong.
Yes, either the default_charset or internal_encoding INI settings should be used instead. |
Sorry, something went wrong.
|
@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. Anyway, using input_encoding and output_encoding is correct. |
Sorry, something went wrong.
|
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) |
Sorry, something went wrong.
|
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:
***@***.***>
|
Sorry, something went wrong.
Let me ask @dseguy to see if this is actually used. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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)? |
Sorry, something went wrong.
|
I created the milestone and added the PR to it. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
@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