Corrected length for unicode chars #8

Closed
wants to merge 2 commits into
from

Projects

None yet

6 participants

@hairyhum

mb_strlen($str, 'UTF-8') returns wrong length for unicode payload, unicode messaged does not send. Fixed to strlen($str)

@Maks3w
Zend Framework member

@hairyhum Please add a unit test case.

@maciekish

Any ETA on this please? I have implemented the fix locally but it'd be nice if it made it's way into the original repo.

@Maks3w
Zend Framework member

Without unit test I can't see why this should be merged.

@maciekish If you can write the unit test you may send a PR with the code of @hairyhum

@maciekish

@Maks3w I think i could but Im not sure about the philosophy behind the test. What should it test for? Input a unicode message and check if the calculated length is correct?

@Maks3w
Zend Framework member

Exactly. One test for an errored scenario and another one for a sucessful scenario.

@maciekish

But the errored scenario only occurs in the old code, not in the new code. Don't really understand how to do that in the test.

@maciekish maciekish added a commit to maciekish/ZendService_Apple_Apns that referenced this pull request Dec 4, 2013
@maciekish maciekish Update Message.php
Fix string length issue for unicode payloads. Ref zendframework#8 (comment)
b4bfe38
@weierophinney
Zend Framework member

@maciekish The idea with a unit test is to do the following:

  • Given certain input
  • Assert that expected output occurs

In your case, you've indicated that given a certain value, the code currently does not perform as expected -- so you would test using that value, and provide your expectation in the form of an assertion.

Personally, I have my doubts about whether or not this is correct. If UTF-8 is not provided, your patch would incorrectly calculate the length (essentially doubling it). This is why we want tests.

@Diwann Diwann referenced this pull request Aug 19, 2014
Closed

payload encoding problem #10

@Diwann

I don't know how to write the unit test you want but I can confirm that this patch correct the error of sending UTF-8 push message (like accents in french).

@mwillbanks

This looks like it was previously fixed.

@mwillbanks mwillbanks closed this May 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment