Dots in parameters get converted to underscores because of parse_str

[jhard]

I've banged my head against the wall over this for the past hour until I looked at curl's debug output and really paid attention and noticed that https://api.cloudflare.com/client/v4/zones?account.id=... becomes https://api.cloudflare.com/client/v4/zones?account_id=..., so Cloudflare is correct in ignoring my filtering and I'm lucky I haven't provided some very, very constructive feedback on their API documentation yet.

In the trait ManagesPsrRequests there's

        parse_str($uri->getQuery(), $existingQuery);
        return $uri->withQuery(
            http_build_query(array_merge($existingQuery, $this->query()->all()))
        );

and the docs confirm:

Because variables in PHP can't have dots and spaces in their names, those are converted to underscores. Same applies to naming of respective key names in case of using this function with result parameter.

I've found that I can do

    public function query() : ArrayStoreContract
    {
        $query = new ArrayStore();
        $query->set([ "account.id" => $this->accountId ]);

        return $query;
    }

in the Request as a workaround, but that feels a lot more clunky than just having

    public function resolveEndpoint() : string
    {
        return "/zones?account.id={$this->accountId}";
    }

Guzzle does some explode-based parsing (GuzzleHttp\Psr7\Uri::getFilteredQueryString) which looks a little sketchy, but it should work just as well. parse_str doesn't support ; as a delimiter either, so I assume it should be fine to switch to something like that unless there's a conscious decision behind using parse_str that I'm not aware of.

[Sammyjo20]

Hey @jhard

Thank you for making me aware of this issue and I'm sorry for your frustration that this caused you! Does this still happen when you use the defaultQuery method on the connector/request?

This is the recommended way to add query parameters:

class YourRequest extends Request
{
    // ...

    protected function defaultQuery(): array
    {
        return [
             'account.id' => $this->accountId,
        ];
    }
}

However, I do support putting query parameters in the URL - so I'm going to look at a much better query string parser! Should have a PR up soon.

[Sammyjo20]

Do you mind having a quick look at this PR please @jhard? https://github.com/saloonphp/saloon/pull/370 Just want someone to double-check the new implementation. I think it's a bit better than the explode-based passing as if someone wrote a query parameter like foo==bar it will still be parsed as foo=bar.

[jhard]

Hey @Sammyjo20 thank you! No worries, it hasn't been the first time I've bashed my head against the wall yesterday, and I'm sure it won't be the last this week. I have a special wall-finding talent and I suck at finding openings in them.

I can confirm that it works with defaultQuery.

370 looks good in general, but it needs to decode the names and values, i.e.

            $name  = urldecode((string) strtok($parameter, '='));
            $value = urldecode((string) strtok('='));

so url-encoded characters in won't be double-escaped (i.e. %40 => %2540) when http_build_query reassembles the query string. I've added string casting in case strtok returns false (e.g. on ?value=).

I'd also add

            if ( str_starts_with($parameter, "=") || ! $name ) {
                continue;
            }

to handle cases like ?=abc and ?a=d& like parse_str would, i.e. no empty names allowed. Currently, it will turn =abc into ["abc" => ""] and a=b& into ["a" => "b", "" => ""]. The latter shouldn't really cause a problem unless someone on the other receiving end parses it similarly and trips over it, but it's probably best to not send it in the first place.

[Sammyjo20]

Ah thank you for spotting those parts I missed! I will go back and add that to the PR. (Thank you also for looking).

My only other worry was with nameless query parameters e.g (?checked&foo=bar) (where checked is the parameter) in my code, ?checked is always being converted back into ?checked= - not sure if this is valid still or not - I think it's http_build_query doing it. Do you think that's something I should consider?

[jhard]

No, I understand that's equivalent. They are meant to be key-value pairs, and having only a key is shorthand for having a key with an empty value, and they are always string values, there's no way to differentiate between "" and null, except not sending the key at all for null.

Someone might do some custom parsing, but I believe pretty much all frameworks / languages these day are aligned on how to understand them. RFC6570 on the URI Template suggests that it's how you assign an empty value, but mentions ;empty for the semicolon-separated version (which are deprecated and not supported by http_build_query) in Level 3 templates:

{;x,y} ;x=1024;y=768 {;x,y,empty} ;x=1024;y=768;empty

{?x,y} ?x=1024&y=768 {?x,y,empty} ?x=1024&y=768&empty=

[Sammyjo20]

Awesome, I have updated the PR 👍

[Sammyjo20]

The issue should be fixed in the next release - let me know if not! Thank you again for the help!