[11.x] Trim trailing ? from generated URL without query params

[onlime]

This is a follow-up for @stevebauman's PR #51075 👏. I would expect a generated URL without query params never ending with a sole ?. The url()->query() helper also supports removing existing query params by setting them to null. If the final query param is removed, the URL should not end with a ?.

This can be considered as a breaking change. But as this feature was just introduced in the newly released Laravel v11.5.0 and the behavior of this PR is probably the one you would expect, I would rather consider it as a tiny little improvement/fix over #51075. I've also added more assertions to test for query param removal.

Cheers, Pipo

[vlakoff]

Only the first ? has special meaning in an URL, and the query string itself may contain literal ?'s. See this question on Stack Overflow.

In particular, the query string may end with a ?, that would be removed by the current code because of the rtrim(..., '?')

I would suggest the following code:

$newQueryString = Arr::query(array_merge($existingQueryArray, $query));

if ($newQueryString !== '') {
    $newQueryString = '?'.$newQueryString;
}

return $this->to($path.$newQueryString, $extra, $secure);

And tests for cases like:

• $url->query('foo/bar?baz=boom?')
• $url->query('foo/bar', ['baz' => 'boom?'])

[onlime]

@vlakoff

Only the first ? has special meaning in an URL, and the query string itself may contain literal ?'s. See this question on Stack Overflow.

In particular, the query string may end with a ?, that would be removed by the current code because of the rtrim(..., '?')

you're right about a query string may end with a ?, but forgot about Arr::query() internally using http_build_query with PHP_QUERY_RFC3986 encoding type, which results in any ? of the query string getting encoded as %3F.

You can test with this modified version of UrlGenerator::query():

    public function query($path, $query = [], $extra = [], $secure = null)
    {
        [$path, $existingQueryString] = $this->extractQueryString($path);

        parse_str(Str::after($existingQueryString, '?'), $existingQueryArray);

        $newQuery = Arr::query(array_merge($existingQueryArray, $query));

        return $this->to($path.($newQuery ? '?'.$newQuery : ''), $extra, $secure);
    }

RoutingUrlGeneratorTest::testQueryGeneration():

        $this->assertSame('http://www.foo.com/foo/bar?baz=boom?', $url->query('foo/bar', ['baz' => 'boom?']));

that test would fail with:

1) Illuminate\Tests\Routing\RoutingUrlGeneratorTest::testQueryGeneration
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'http://www.foo.com/foo/bar?baz=boom?'
+'http://www.foo.com/foo/bar?baz=boom%3F'

so, because of the applied encoding, we can safely ignore any ending ? from the query string and the rtrim of this PR works as expected.

[vlakoff]

Actually, the code I suggested was initially because I felt it was wrong to use rtrim() as you did, because its purpose is not really an end trim, but rather it is used in a "diverted" way, to delete the "?" when nothing has been appended after it.

Then, I noticed it was actually fixing a potential issue… at least I thought, as you demonstrated the issue cannot occur.

Though, we are relying on the behavior that http_build_query() encodes the ?'s (of course it certainly will always do, so it's future-proof). It's just that by doing differently, we can avoid relying on this behavior, so it seems to be a better approach.

Therefore, I still prefer the code I suggested, but yours is also fine, as there is no issue with it.