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

Fixes C14N, PDO.PgSQL, chr() #1066

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

kripper
Copy link
Contributor

@kripper kripper commented Sep 7, 2022

@jakubmisek here is a patch for the C14N bug.
Can you also please merge my helper function "peachpie_set_string_encoding()" which is required to change encoding and be able to work with ISO-8859-1 sources.
It won't harm the project, but will be usefull/required to work with ISO-8859-1 sources.

@jakubmisek
Copy link
Member

thanks @kripper

@@ -37,6 +37,11 @@ internal static CharMap InitializeCharMap()

#endregion

// Helper function to change Console.OutputEncoding from PHP
public static void peachpie_set_string_encoding(string str) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't just add a public function to the API like that. You can always define this method in privately your project, or we could add a configuration option for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't just add a public function to the API like that. You can always define this method in privately your project, or we could add a configuration option for this.

The configuration options sounds good to me.

@jakubmisek
Copy link
Member

Can we have the PR without the change to Strings.cs, it's totally unrelated to the issue.

thank you

@kripper
Copy link
Contributor Author

kripper commented Apr 2, 2023

Please note that the helper method needed for working with iso-8859-1 and utf-8 at the same time, was reverted here: 2808001

We have been using this method in production for years and we really need it, since there is no other option.

Having this method in our code, is also an obstacle for us to release pull requests of all other fixes that are required upstream.

We don't know how to deal with this method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants