edamov / pushok

PHP client for Apple Push Notification Service (APNs) - Send push notifications to iOS using the new APNs HTTP/2 protocol with token-based (JWT with p8 private key)
MIT License
368 stars 119 forks source link

JSON_FORCE_OBJECT breaks arrays #82

Closed hnicolas-w closed 4 years ago

hnicolas-w commented 4 years ago

Hello,

I noticed that the library use the json_encode JSON_FORCE_OBJECT option in the Payload toJson method. This causes non-associative array to be outputed as objects.

I'm wondering why this choice ? It makes a few of my apps crash as they were expected non-associative arrays.

Output :

  "path_list" = {
                    0 = {
                        sign = "xxxx";
                        url = "xxx";
                    }
                };

I did the fix on my end, and now have this output :

  "path_list" = [
                    {
                        sign = "xxxx";
                        url = "xxx";
                    }
               ];

To me, the library should only set this option if it was asked by the user. I can make a PR after we talk about it.

edamov commented 4 years ago

I think you are right. I added JSON_FORCE_OBJECT for passing tests. It is useful when the recipient of the output is expecting an object and the array is empty. It will not throw an exception. So you can make a PR if you wish.

edamov commented 4 years ago

I just updated the test (setCustomValue('array', [1,2,3])) and it passed (without your PR):

    public function testConvertToJSon()
    {
        $alert = Alert::create()->setTitle('title');

        $payload = Payload::create()
            ->setAlert($alert)
            ->setBadge(1)
            ->setSound('sound')
            ->setCategory('category')
            ->setThreadId('tread-id')
            ->setContentAvailability(true)
            ->setMutableContent(true)
            ->setCustomValue('key', 'value')
            ->setCustomValue('array', [1,2,3]);

        $this->assertJsonStringEqualsJsonString(
            '{"aps": {"alert": {"title": "title"}, "badge": 1, "sound": "sound", "category": "category", ' .
            ' "thread-id": "tread-id", "mutable-content": 1, "content-available": 1}, "key": "value", "array": [1,2,3]}',
            $payload->toJson()
        );
    }

Can you please give an example of Payload data which doesn't work as expected?

hnicolas-w commented 4 years ago

It's because assertJsonStringEqualsJsonString doesn't compare properly the JSON string is this case. (I might create an issue on PHPUnit but each thing in time...).

Just var_dump($payload->toJson()) and you will see what i'm talking about. The only way to properly test this case it to json_decode($payload->toJson()) and test the type of the array value like i did in my PR.

Add this and you will see your test breaking:

$decoded_json = json_decode($payload->toJson()); $this->assertEquals(gettype($decoded_json->custom_array), 'array');

edamov commented 4 years ago

If you want json_decode returns an array you should pass second parameter true. This will pass:

$decoded_json = json_decode($payload->toJson(), true);
$this->assertEquals(gettype($decoded_json['custom_array']), 'array');
edamov commented 4 years ago

Seems now I understand. Array non-associative keys shouldn't exist in json, right?

hnicolas-w commented 4 years ago

Yeah but why passing this ? It's not good. This 2nd parameter does : When TRUE, returned objects will be converted into associative arrays. (aka the exact opposite of JSON_FORCE_OBJECT). So ok it works for the test but the lib is still outputing {0: 1, 1:2, 2:3} when i want [1,2,3].

Also look at my PR request, i feel you used JSON_FORCE_OBJECT just to make the aps key always an object even when empty. To me this is bad pratice as the aps key should have been a PHP Object from the start. Not an array.

edamov commented 4 years ago

Thanks, now it is clear