iconv: deprecate iconv_set_encoding() and iconv_get_encoding()

[jorgsowa]

According to RFC https://wiki.php.net/rfc/default_encoding the functions iconv_set_encoding() and iconv_get_encoding() should be deprecated long time ago.

It's preparation for seamless merge of #12445 (currently there is no deprecation for functions changing encoding in iconv). The same must be done for mbstring extension.

[Girgias]

Seems like you need to regenerate the optimized files

[Girgias]

The generated Zend/Optimizer/zend_func_infos.h file is still out of date. Please use genstub to fix this.

[jorgsowa]

I'm sorry. There was wrong order of Attribute and PHPDoc and parser didn't get it correctly. Now it should be fine.

[DanielEScherzer]

I don't see this discussed in the RFC - the strings set_ or get_ are not mentioned in the RFC page at all, and the only mention of deprecation is

Use of iconv./mbstring. php.ini parameters raise E_DEPRECATED for 5.6 and up.

Given that

• it is unclear if deprecating these functions was a part of the RFC that was voted on
• its been >10 years since the RFC without marking the deprecations

I'm not sure this should be included in 8.5 (or even at all without some discussion, but that is a question for later). I'm not vetoing this for 8.5 at the moment, because maybe I'm missing something in that RFC?

CC @edorian
Marking as "request changes" so this doesn't get merged without addressing

[edorian]

I agree with Daniel.

Additionally, stating "deprecated since 5.6" in the error message strikes me as odd as the deprecation wasn't in these versions. Or I'm very much misunderstanding what's happening here.

[jorgsowa]

Without proper INI settings those functions are pointless, so it's safer to deprecate those functions and users would switch to global encoding, than suddenly changing the behaviour of those.

Additionally, stating "deprecated since 5.6" in the error message strikes me as odd as the deprecation wasn't in these versions.

Formally it was, but it was never marked in the code properly.

I have written mail to internals list to address this matter.

[DanielEScherzer]

The mailing list email is great, but this should be discussed and probably also needs a full RFC (maybe just an entry in the PHP 8.6 deprecations mass RFC)
For now, upgrading my

Marking as "request changes" so this doesn't get merged without addressing

to "this should not be merged for 8.5"

[jorgsowa]

to "this should not be merged for 8.5"

What's the reason? Deprecations are being accepted in this phase.

[Girgias]

The mailing list email is great, but this should be discussed and probably also needs a full RFC (maybe just an entry in the PHP 8.6 deprecations mass RFC) For now, upgrading my

Marking as "request changes" so this doesn't get merged without addressing

to "this should not be merged for 8.5"

I disagree, this feature was slated for deprecation way back in PHP 5.6 and removal in PHP 7 (it says 6, but this is due to confusion of the name of the release), and didn't get removed in PHP 7 due to a technical TODO which wasn't done in time. See following discussion: https://externals.io/message/103087.
This was fixed in 7.4 but when I submitted #5334 to get them removed in 8.0 the short time span, confusion around those INI settings and behaviour and time constraints meant that it got punted to PHP 9. (Some of the R11 discussion at the time: https://chat.stackoverflow.com/transcript/11?m=49250309#49250309 and https://chat.stackoverflow.com/transcript/11?m=50205227#50205227).

Confirming the status of the deprecation are the manual entries for the iconv and mbstring INI settings clearly state the followings:

This feature has been DEPRECATED as of PHP 5.6.0. Relying on this feature is highly discouraged.

and

This deprecated feature will certainly be removed in the future.

respectively.

Moreover, multiple discussion clearly indicate that removing the INI settings, which as established are formally slated for deprecation, while keeping the functions is nonsensical. Because what would those functions do, nothing?

Delaying the deprecations of these functions to a latter point serves absolutely no point from my point of view. Especially as we don't know if a version 8.6 will even exist.

I am going to be harsh, but forcing an RFC for something that is a natural follow-up and arguably was just forgotten in the original RFC, and has been multiple time argued that it should be done, is a waste of everyone's time and makes PHP look like a joke to people not familiar with the project.

[Girgias]

Just put 8.5 as this is usually what we do when formally deprecating something.

Suggested change

	#[\Deprecated(since: '5.6', message: 'use internal_encoding, input_encoding, and output_encoding INI settings instead')]
	#[\Deprecated(since: '8.5', message: 'use internal_encoding, input_encoding, and output_encoding INI settings instead')]

[DanielEScherzer]

@Girgias you raise some good points
@edorian and I have discussed this some more - if there are no objections on the mailing list thread after a week, then we are okay with this being included in 8.5. Can you add a message to the https://news-web.php.net/php.internals/128624 thread (or start a new one)?