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

Fix GH-17469: UConverter::transcode() not hardcoding error handling. #17488

Closed
wants to merge 1 commit into from

Conversation

devnexen
Copy link
Member

Respecting instead intl.use_exceptions/intl.error_level.

@devnexen devnexen marked this pull request as ready for review January 16, 2025 20:20
@devnexen devnexen requested a review from Girgias January 16, 2025 20:20
php_error_docref(NULL, E_WARNING, "Ambiguous encoding specified, using %s", actual_encoding);
char *msg;
spprintf(&msg, 0, "Ambigious encoding specified, using %s", actual_encoding);
intl_error_set(NULL, error, msg, 1);
Copy link
Member

Choose a reason for hiding this comment

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

I think this one should remain a warning, as ucnv_open() was successful and the conversion is actually performed:

$str = "\x0a";
var_dump(bin2hex($str));

$str = UConverter::transcode("\x0a", 'UTF-16', 'UTF-8');
var_dump(bin2hex($str));

$str = UConverter::transcode($str, 'windows-1251', 'UTF-16');
var_dump(bin2hex($str));

Result:

string(2) "0a"
string(8) "fffe0a00"

Warning: UConverter::transcode(): Ambiguous encoding specified, using ibm-5347_P100-1998 in test.php on line 8
string(2) "0a"

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

php_error_docref(NULL, E_WARNING, "Error setting encoding: %d - %s", (int)error, u_errorName(error));
char *msg;
spprintf(&msg, 0, "Error setting encoding: %d - %s", (int)error, u_errorName(error));
intl_error_set(NULL, error, msg, 1);
Copy link
Member

Choose a reason for hiding this comment

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

Should we pass error to the code parameter here? This is exposed via intl_get_error_code() so it can be useful.

In the Ambigious encoding specified case above, if we convert it back to a warning, we could still expose the error code by calling intl_errors_set_code().

Copy link
Member Author

Choose a reason for hiding this comment

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

well objval is null in that case, what do you suggest ?

Copy link
Member

Choose a reason for hiding this comment

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

I was suggesting to pass error to the code parameter, but I'm noticing now that you are already doing exactly that, so please dismiss this comment :D

Copy link
Member

@arnaud-lb arnaud-lb left a comment

Choose a reason for hiding this comment

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

Looks good to me apart from the typo

@@ -376,25 +376,28 @@ static bool php_converter_set_encoding(php_converter_object *objval,
/* Should never happen */
actual_encoding = "(unknown)";
}
php_error_docref(NULL, E_WARNING, "Ambiguous encoding specified, using %s", actual_encoding);
php_error_docref(NULL, E_WARNING, "Ambigious encoding specified, using %s", actual_encoding);
Copy link
Member

Choose a reason for hiding this comment

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

Typo on Ambigious :)

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

MSTM, thank you!

Respecting instead intl.use_exceptions/intl.error_level.

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

Successfully merging this pull request may close these issues.

3 participants