Skip to content

Remove deprecated iconv INI settings #5491

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove deprecated iconv INI settings #5491

wants to merge 1 commit into from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Apr 29, 2020

Analogous to #5334 for iconv

cmb69
cmb69 reviewed Apr 29, 2020
View reviewed changes
@Girgias Girgias force-pushed the iconv-remove-deprecated-ini branch 2 times, most recently from b5789e7 to b760ae4 Compare April 29, 2020 15:21
cmb69
cmb69 reviewed Apr 29, 2020
View reviewed changes
@Girgias Girgias force-pushed the iconv-remove-deprecated-ini branch 2 times, most recently from b215af0 to b3ba3f0 Compare April 29, 2020 15:57
@Girgias Girgias force-pushed the iconv-remove-deprecated-ini branch 2 times, most recently from 40343fe to 30ea7f5 Compare May 9, 2020 17:37
@Girgias Girgias force-pushed the iconv-remove-deprecated-ini branch 2 times, most recently from 724d599 to 084bbaa Compare June 20, 2020 22:53
@Girgias Girgias force-pushed the iconv-remove-deprecated-ini branch from 084bbaa to 8f94869 Compare July 3, 2020 10:06
@Girgias
Copy link
Member Author

Girgias commented Jul 4, 2020

@nikic @cmb69 can one of you have a look at this again and tell me if I can merge this into master?

@cmb69
Copy link
Member

cmb69 commented Jul 4, 2020

Thanks for working on this! I'm not really sure what to do with iconv_set_encoding() and iconv_get_encoding(); since they don't have been already deprecated, it might be best to deprecate them now, but to keep the functionality. Or maybe not, since it'd be somehow confusing.

@Girgias Girgias force-pushed the iconv-remove-deprecated-ini branch from 8f94869 to 95af3c5 Compare July 10, 2020 14:11
@Girgias
Copy link
Member Author

Girgias commented Jul 10, 2020

Thanks for working on this! I'm not really sure what to do with iconv_set_encoding() and iconv_get_encoding(); since they don't have been already deprecated, it might be best to deprecate them now, but to keep the functionality. Or maybe not, since it'd be somehow confusing.

I've deprecated both of them not sure how to go about preserving functionality tho as it would seem weirdly convoluted.

@Girgias
Remove deprecated iconv INI settings
5d2142e 
Also deprecated relevant function to set/get iconv encodings.
@Girgias Girgias force-pushed the iconv-remove-deprecated-ini branch from 95af3c5 to 5d2142e Compare July 20, 2020 09:23
@Girgias
Copy link
Member Author

Girgias commented Jul 20, 2020

If there are no objections I'll merge this at the end of the week.

@nikic
Copy link
Member

nikic commented Jul 23, 2020

For both this one and #5334:

  • What is the state regarding different encoding names in iconv and mbstring? Did you check that they support the same set of encodings?
  • This PR deprecates iconv_set_encoding() and converts it into a no-op, while the mbstring PR retrains the mb_internal_encoding() etc functionality. At the least this is inconsistent, but I don't know what the right solution is.

@Girgias Girgias requested a review from kocsismate as a code owner October 7, 2023 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmb69 cmb69 cmb69 left review comments

@kocsismate kocsismate Awaiting requested review from kocsismate kocsismate is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Girgias @cmb69 @nikic