Skip to content

Skip test if libxml does not support GBK encoding #15072

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

Merged
merged 2 commits into from
Jul 23, 2024

Conversation

arnaud-lb
Copy link
Member

When libxml uses musl's iconv implementation, conversion to GBK is not supported. As a result this test fails like this:

001- <?xml version="1.0" encoding="GBK"?>
002- <!--������-->
001+ Warning: XMLWriter::startDocument(): xmlTextWriterStartDocument : unsupported encoding in /__w/php-src/php-src/ext/xmlwriter/tests/xmlwriter_toStream_encoding_gbk.php on line 6
002+ <!--ééé-->

Here I try to skip the test in this case.

The skipif section is quite large, unfortunately. Checking that iconv supports GBK via the iconv extension wouldn't be reliable as it may be linked against gnu iconv even on musl builds.

@nielsdos
Copy link
Member

It doesn't necessarily have to be GBK, another CJK encoding would do too, but I guess musl's encoding support is limited in general.

When libxml uses musl iconv, GBK encoding is not supported as a target encoding
@arnaud-lb
Copy link
Member Author

arnaud-lb commented Jul 22, 2024

Thank you! Yes, musl's iconv does not support most multibyte encodings as target encoding. I've updated the test to use SHIFT-JIS instead as it is supported as both source and target.

Edit: FreeBSD doesn't support SHIFT-JS apparently. Will try to find one that is supported everywhere.

@arnaud-lb arnaud-lb mentioned this pull request Jul 23, 2024
@arnaud-lb
Copy link
Member Author

Using SHIFT_JS instead of SHIFT-JS fixed FreeBSD

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

I haven't tested this on Alpine, as I don't have a test setup for that with me, but this patch is fine. Thanks.

@arnaud-lb arnaud-lb marked this pull request as ready for review July 23, 2024 14:06
@arnaud-lb arnaud-lb merged commit e63f822 into php:master Jul 23, 2024
11 checks passed
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.

2 participants