Bacon / BaconQrCode

QR Code Generator for PHP
BSD 2-Clause "Simplified" License
1.86k stars 212 forks source link

Something wrong with appendKanjiBytes function? #123

Closed askdkc closed 7 months ago

askdkc commented 1 year ago

After I fix Shift-JIS handling in inOnlyDoubleByteKanji function with this PR, appendKanjiBytes function starts throwing an error every time I pass Double Byte Kanji Character string.

For example: "日本", "こんにちは" -> error // Only Double Bytes Shift-JIS "日本123", "こんにちはhellow" -> OK // Double Bytes Shift-JIS and ASCII works fine

Details in https://github.com/Bacon/BaconQrCode/pull/122#issuecomment-1397920451

213619290-b544f094-b19e-4cf7-a77c-068847a6364b
DASPRiD commented 1 year ago

Looking through the code and also your PR, I think I might know what the problem is. Please correct me if I'm wrong:

Looking at the test in your PR, you pass in the Kanji string as UTF-8 encoded. I'm not perfectly certain on what the Zxing library did there, but I think that you have to pass the string in as SHIFT-JIS encoded, not UTF-8 encoded. You can do that via iconv().

askdkc commented 1 year ago

@DASPRiD

Thank you for checking the code.

I should have explained the situation first. My apologies.

I use this package via simple-qrcode using Laravel so that user inputs are always encoded in UTF-8.

But I need to output Shift-JIS encoded QRcode because using QR scanner in Japan usually doesn’t recognize the QRcode that is not encoded in Shift-JIS.

And even if I change user input in Laravel from UTF-8 to Shift-JIS, it still throws an error below:

image
DASPRiD commented 1 year ago

Okay, I think I spotted the issue. I should actually be converting the string to Shift-Jis in that function and kinda missed that.

Can you please verify if this branch fixes your issue? If yes, I'll also merge your pull request and release a new version.

https://github.com/Bacon/BaconQrCode/tree/fix/issue-123

askdkc commented 1 year ago

@DASPRiD

Thank you for your fix. I tried your code but it throws an Unsupported operand types: string & int Error.

And find out the code below hasn't fixed.

https://github.com/Bacon/BaconQrCode/blob/f5a7a84a30e4ff17fb248a6b82ef015de3ad472c/src/Encoder/Encoder.php#L220

I created PR: https://github.com/Bacon/BaconQrCode/pull/124 for this. (Now QRcode is generatited fine😃)

BTW, although Shift-JIS encoding is fixed, the test starts throwing some errors:

% ./vendor/bin/phpunit test/Encoder/EncoderTest.php
PHPUnit 9.5.28 by Sebastian Bergmann and contributors.

......E......E.                                                   15 / 15 (100%)

Time: 00:00.034, Memory: 6.00 MB

There were 2 errors:

1) BaconQrCodeTest\Encoder\EncoderTest::testAppendBytes
BaconQrCode\Exception\WriterException: Content could not be converted to SHIFT-JIS

/Users/dkc/Sites/qrcoder/vendor/bacon/bacon-qr-code/src/Encoder/Encoder.php:635
/Users/dkc/Sites/qrcoder/vendor/bacon/bacon-qr-code/src/Encoder/Encoder.php:531
/Users/dkc/Sites/qrcoder/vendor/bacon/bacon-qr-code/test/Encoder/EncoderTest.php:258

2) BaconQrCodeTest\Encoder\EncoderTest::testAppendKanjiBytes
BaconQrCode\Exception\WriterException: Content could not be converted to SHIFT-JIS

/Users/dkc/Sites/qrcoder/vendor/bacon/bacon-qr-code/src/Encoder/Encoder.php:635
/Users/dkc/Sites/qrcoder/vendor/bacon/bacon-qr-code/test/Encoder/EncoderTest.php:448

ERRORS!
Tests: 15, Assertions: 107, Errors: 2.

So I also fixed your test and it is included this PR: https://github.com/Bacon/BaconQrCode/pull/124

askdkc commented 1 year ago

@DASPRiD

Hi. Sorry to bother you. But may I ask when this fix gets merged? If there are some problems, let me know.