Incorrect encoding of (pagination) query parameters

[Ilyes512]

I believe this is is incorrect:

    /**
     * @param mixed $cursor
     */
    public static function getPaginationQueryString($cursor, int $size): string
    {
        return urlencode("page[cursor]=$cursor") . "&" . urlencode("page[size]=$size");
    }

The above method called with foobar and 10 results in: page%5Bcursor%5D%3Dfoobar&page%5Bsize%5D%3D10.

As you can see it also encodes the =. I think using http_build_query (doc) might be the solution instead? Do you agree?

Source: https://github.com/woohoolabs/yin/blob/master/src/JsonApi/Request/Pagination/CursorBasedPagination.php#L61

[kocsismate]

Thanks for the report! I added the referenced commit to fix the issue. Please confirm that it works for you, and I'll release it in v 4.2.0.

[Ilyes512]

I finally took the time to go through all of the unreleased changes :) (sorry for taking so long):

• #98: Support for validating top-level members in requests
  • I added this to my middleware and noticed my GET requests breaking... fixed by also checking the request method is either one of POST, PUT, PATCH or DELETE.
• #96: Support for providing a custom schema path to ResponseValidator
  • Nice! I got to delete my custom extension of the class... I was doing exactly the same. Should have made a PR instead.
• #100: Error in createResourceIdInvalidException
  • Yup, this works.
• #97: Incorrect encoding of (pagination) query parameters
  • Yup, works as well.

So I can't wait for the 4.2.0 release :)

[kocsismate]

I added this to my middleware and noticed my GET requests breaking... fixed by also checking the request method is either one of POST, PUT, PATCH or DELETE.

Thanks for the idea, I certainly forgot about checking the method. However, I went with checking for the methods that shouldn't have a body (get, head, delete) so that validation isn't performed in such cases.

[Ilyes512]

The problem is, that there are more (less popular?) methods that exist... for example Symfony uses the following list:

    public const METHOD_HEAD = 'HEAD';
    public const METHOD_GET = 'GET';
    public const METHOD_POST = 'POST';
    public const METHOD_PUT = 'PUT';
    public const METHOD_PATCH = 'PATCH';
    public const METHOD_DELETE = 'DELETE';
    public const METHOD_PURGE = 'PURGE';
    public const METHOD_OPTIONS = 'OPTIONS';
    public const METHOD_TRACE = 'TRACE';
    public const METHOD_CONNECT = 'CONNECT';

Source: https://github.com/symfony/http-foundation/blob/5.x/Request.php#L55-L64

That's why I chose to whitelist instead of blacklist because I know I want to do the check for those methods.

[kocsismate]

Ah, in the meanwhile I posted another comment, but in the linked issue (deleted since then), so I re-post it here:

Hmm, perhaps a more conformant implementation would be to check if there is a body in the request at all. This would ensure that GET et al. requests still work, while it wouldn't make it impossible to receive post requests without a body (which are not necessarily JSON:API documents).

[Ilyes512]

At first that was what I wanted to do, but there was no hasBody() and was not really sure if that was the way to go anyways.