ghassani / surveymonkey-v3-api-php

SurveyMonkey v3 API Wrapper
GNU General Public License v3.0
21 stars 23 forks source link

`createWebhook()` with `object_ids` is serialised incorrectly #20

Closed NoelLH closed 5 years ago

NoelLH commented 5 years ago

Thanks for this lib, it's looking promising for my use case so far!

I would create a PR for this but I'm not yet 100% clear on the best way to resolve it.

The behaviour I expected was that I could pass object_ids as a key of the array passed to createWebhook() and have it passed on in the request object like

{
...
object_ids: ["1234"]
...
}

But because Client::createRequest() does the encoding from the input array and adds the JSON_FORCE_OBJECT flag, the current behaviour seems to be this:

{
...
object_ids: {"0": "1234"}
...
}

This is rejected by the API with an invalid schema error.

One workaround is to do my own json_encode() without the flag and pass in the string, but this generates a warning since the public method declares its argument as an array. (I'm still on PHP 5.6 for this project unfortunately - haven't checked what happens on 7.x yet.)

As things stand, I don't think it's possible to pass in the correct param type while setting this filter for the new webhook successfully.

If you don't see obvious BC breaks for other methods, I'm thinking the cleanest solution might be:

What do you think?

ghassani commented 5 years ago

Looks like the JSON_FORCE_OBJECT was added in commit 1113f133430eaf4de6f6f202d9c6a865383183f2 and references this stack overflow question https://stackoverflow.com/questions/41150119/bad-request-error-on-surveymonkey-v3-send. I think we can maybe ditch the JSON_FORCE_OBJECT parameter and if the data is an array and empty, just pass in an empty json object string "{}" as needed for requests with no/optional parameters. Maybe @shoaibali can chime in since he committed that fix.

shoaibali commented 5 years ago

Yes I agree, adding JSON_FORCE_OBJECT is less than ideal. Ideally, should be dealt with at SurveyMonkey API end but it is what it is.

Different PHP versions have no bearing on the json_encodingoutput.

Would something like this work for us @NoelLH ?

    private function createRequest($method, $uri, array $options = [], $body = '')
    {
            if (is_array($body) && empty($body)) 
            {
                $body= '{}';
            }
           $ret = new Request($method, $uri, $options, is_array($body) ? json_encode($body) : $body);

The other option (not recommended) is we change the createRequest paramters to also accept $json_options. So the method looks something like this


    private function createRequest($method, $uri, array $options = [], $body = '', $json_options = 0)
    {
        $ret = new Request($method, $uri, $options, is_array($body) ? json_encode($body, $json_options) : $body);

      }

The limitation with above is that you can only do one option at a time or perhaps just pass in | seperated list of options such as JSON_HEX_TAG | JSON_HEX_APOS | JSON_HEX_QUOT | JSON_HEX_AMP | JSON_UNESCAPED_UNICODE

Feel free to do a pull request, happy to weigh in. Thanks for bringing this to attention and apologies for the trouble this may have caused.

NoelLH commented 5 years ago

@shoaibali I just mentioned different PHP versions w.r.t. the workaround & incorrect argument type - I was thinking newer versions with strict_types would make this an even worse idea - but academic for an actual fix.

I hadn't fully thought this through when I suggested an outer cast to (object). Since associative arrays already get JSON-encoded as objects I think your suggestion of a special case workaround, while removing the flag, is a good approach. I'll make a PR now.

ghassani commented 5 years ago

Fixed in 1.0.1