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 Windows encoding convertion #492

Merged
merged 2 commits into from Apr 13, 2016

Conversation

mizunashi-mana
Copy link

I got an error on run phing.

> phing prepare
Notice: iconv(): Wrong charset, conversion from `UTF-8' to `Windows-932//IGNORE' is not allowed in vendor\phing\phing\classes\phing\system\io\Win32FileSystem.php on line 635
Buildfile:  does not exist!

Now, phing use iconv or mb_convert_encoding for fixing windows encoding.
However, Windows-${codepage} is not usual codepage encoding format.

My cmd.exe use codepage 932.
CP932 exists in supported encoding for iconv and mbstring, but Windows-932 is not.
So, fixEncoding function failed and returned not string...

I request to use CP for encoding prefix.
Thanks!

Refs

@mrook
Copy link
Member

mrook commented Apr 6, 2016

Changing "Windows-" to "CP-" will probably cause a regression in 84e4050 / https://www.phing.info/trac/ticket/1198

Is there some other way to determine available codepages? I don't do much Windows development, and I don't have any special codepages set on my Windows machine.

@mizunashi-mana
Copy link
Author

@mrook Thank you for feedback!

I try on PHP 7.4 (on psysh v0.7.2):

iconv('UTF-8', 'Windows-1251//IGNORE', 'something'); // success
iconv('UTF-8', 'CP1251//IGNORE', 'something'); // success

If iconv receive not support encoding, return false by ignore.

mb_convert_encoding('something', 'Windows-1251', 'UTF-8'); // success
mb_convert_encoding('something', 'CP1251', 'UTF-8'); // success

If mb_convert_encoding receive not support encoding, throw error.

It seems to be no problem.
On https://gist.github.com/hakre/4188459 (iconv encodings) and http://php.net/manual/ja/mbstring.supported-encodings.php (mbstring encodings), CP- is more supported than Windows- and there are CP- aliases for all of Windows- encoding.
If we trust documents on the net, this request will be fine on Windows ;)

My Japanese Windows is using CP932. This is usual code page in Japan. However, CP932 doesn't have Windows- alias unfortunately...

@@ -630,7 +630,7 @@ public function lister($f)
*/
private function fixEncoding($strPath)
{
$codepage = 'Windows-' . trim(strstr(setlocale(LC_CTYPE, 0), '.'), '.');
$codepage = 'CP' . trim(strstr(setlocale(LC_CTYPE, 0), '.'), '.');
Copy link
Member

Choose a reason for hiding this comment

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

As shown in the iconv list of encodings https://gist.github.com/hakre/4188459 Windows- is the alias for CP as mentioned by @mizunashi-mana, there will be no regression to this. But i would suggest changeing setlocale(LC_CTYPE, 0) to setlocale(LC_CTYPE, '') to get allways the default locale under windows and not an C if not defined in settings properly.

@mrook
Copy link
Member

mrook commented Apr 12, 2016

Thanks for the updates! @mizunashi-mana, can you update your pull request with @siad007 's comments? I'll then merge it.

@mizunashi-mana
Copy link
Author

@mrook @siad007 I updated this request!

@mrook mrook merged commit d1c18df into phingofficial:master Apr 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants