It would require a more complex block of code than that in order to execute the replacement only on the query component of the URL — str_replace would also catch %20 in the path and fragment components, which is not desired.

So it could be something like this:

$url = parent::doGenerate(...);

$parts = parse_url($url);

if (isset($parts['query'])) {
    $search = $parts['query'];
    $replace = str_replace('%20', '+', $search);
    $url = str_replace($search, $replace, $url);
}

For reliability I would be more comfortable with something like

if (isset($parts['query'])) {
    $parts['query'] = str_replace('%20', '+', $parts['query']);

    $url = '';
    
    if (isset($parts['scheme'])) {
        $url .= $parts['scheme'];
    }
    
    if (isset($parts['host'])) {
        ...
    }
    
    if (isset($parts['port'])) {
        ...
    }
    
    ...
}

But to me that starts to be excessively redundant with the heavy lifting doGenerate has already done, and it seems much preferable to simply have a way to toggle the parameter passed to http_build_query.

I don't think a global config is appropriate for this. Using RFC 1738 depends on the mime type of the page and you could even mix links with encoding RFC 1738 and RFC 3986 in the same page if you need to. So a global config would not fit there.
For this reason, it seems better to just write a helper function that replaces %20 with + as you pointed out and does not require a change in route generation.

While we could simply replace %20 with + after generating facet URLs, it becomes much more complicated to accomplish the same effect in pagination links if you are using a bundle like KnpPaginatorBundle.

You can provide a custom template for the pagination in the KnpPaginatorBundle that uses + for spaces: https://github.com/KnpLabs/KnpPaginatorBundle/blob/f4ece5b347121b9fe13166264f197f90252d4bd2/Twig/Extension/PaginationExtension.php#L46
Or you could write a response filter that just replaces all links in the pagination div.

Btw, the PR with the implementation and description is excellent. Good job! I just don't think this is the right solution to the problem.

Thanks, @Tobion.

I was trying to make a good argument by anticipating counterarguments — but ultimately, I would not have created the pull request if I did not feel that the framework should let the developer have control over which query encoding type is used and avoid complex workarounds. I understand that RFC 1738 may be considered outdated, and RFC 3986 is the default for good reason. But I want to copy what Google does! 😄

you could even mix links with encoding RFC 1738 and RFC 3986 in the same page
if you need to.

This is a point @iltar also raised, and it’s a good one. But I don’t understand how it is a relevant criticism of this PR, because you can’t do this currently.

As I see it, there are four options:

1. as of [Routing] build query string with PHP_QUERY_RFC3986 #19625 — the generator always uses RFC 1738
2. as of [Routing] Generate URLs in compliance with PHP_QUERY_RFC3986 #19639 — the generator always uses RFC 3986
3. this PR — the generator always uses the developer’s preferred encoding type
4. TBD — the generator lets you specify the encoding type at runtime (as a method arg perhaps?)

Are you suggesting we skip 3 and proceed straight to 4? Or do you feel that the framework should continue to use RFC 3986 exclusively and leave it to developers to find alternate ways of achieving RFC 1738 links?

Honestly, I'm fine with this feature. I don't see why we should force RFC 3986 over RFC 1738. Changing globally is fine to me. At least there are no practical downsides I know about. @Tobion I feel like you're suggesting there are differences related to the mime content type, can you elaborate?

Can't we make RFC1738 the hardcoded behavior instead of RFC 3986 actually, for the query part only?

That was the original behavior before #19639 was merged, since the enc_type in http_build_query defaults to PHP_QUERY_RFC1738.

Undoing that change would nullify the discussion and decisions in #19625 and #19639. It would also create a BC break for anyone who made adjustments because of the previous BC break (#20856).

Global configs are a dead end IMO. Let's say I write a reusable bundle and generate links to a controller. I cannot even assume anymore that I can read links properly that I generated myself.

I agree with @Tobion here

I'm closing as won't fix: the currently generated URLs work fine so the matter is purely cosmetic, and we didn't reach an agreement to find the best way this should be configurable if it should.
Thanks for opening.