FEATURE: introduce UriHelper to work with query parameters

[mhsdesign]

FYI: This pr was refactored again via #3336

While working on #2744 and also neos/neos-development-collection#4552 we always came to the conclusion that the $queryParameters merging of the psr uri is limited.

This introduces a utility to do this:

UriHelper::uriWithAdditionalQueryParameters($this->someUriBuilder->uriFor(...), ['q' => 'search term']);

and allows us to remove any $queryParam logic from the uribuilder(s)

Upgrade instructions

Review instructions

Checklist

• Code follows the PSR-2 coding style
• Tests have been created, run and adjusted as needed
• The PR is created against the lowest maintained branch
• Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
• Reviewer - The first section explains the change briefly for change-logs
• Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

[mficzel]

Very nice idea ... how about using named variadic arguments instead of an array to

$this->someUriBuilder->uriFor(...)->withQueryParameters(q: 'search term');

The only downside of extending the psr uri imho would be that a future psr-version might define the same method with a different signature. Bit theoretical and we would find a way (migration) around that.

[mficzel]

Also:

1. The \Neos\Http\Factories\UriFactory should return the Neos Uri in that case and it might make more sense to
2. Why not extend \GuzzleHttp\Psr7\Uri as those guzzle classes are used in the UriFactory curently anyways

[bwaidelich]

Thanks for the mini adjustments, +1 by reading

how about using named variadic arguments instead

IMO that could be weird with nested arrays. And it won't work if the param name contains invalid characters (like -)

The \Neos\Http\Factories\UriFactory should return the Neos Uri in that case and it might make more sense to

+1

Why not extend \GuzzleHttp\Psr7\Uri as those guzzle classes are used in the UriFactory curently anyways

I would vote against that because it adds an explicit dependency and makes us more vulnerable to future changes of the upstream implementation.
Forwarding all method calls to the wrapped implementation is awkward, so I had the same reflex. But I think it is not an actual issue here and still better than inheritance

[mficzel]

Bastian is right ... even though i really like variadics

[kitsunet]

Yeeeeeah, but then how do we deal with interface compatibility, because suddenly every place we use this we shouldn't / can't rightfully annotate UriInterface anymore. I would rather prefer to go the guzzle route and implement static methods for this. It's a simple enough operation.

[bwaidelich]

I would rather prefer to go the guzzle route and implement static methods for this

@kitsunet Is your suggestion to not re-add the Uri implementation but instead provide some static method like

SomeClass {
  public static function addQueryParameters(UriInterface $uri, array $queryParameters): UriInterface;
}

?

I don't think that's a bad idea.. For the regular use cases (i.e. Fusion, ...) we can add corresponding helpers

[kitsunet]

Yes! Exctly, guzzle psr7 has a couple of helpers that do similar things (nothing for GET params though) but I find that super fine for such syntactic sugar.

[bwaidelich]

guzzle psr7 has a couple of helpers that do similar things (nothing for GET params though)

btw: It does https://github.com/guzzle/psr7/blob/2.6/src/Query.php#L71 but I prefer the suggested signature for brevity (SomeHelper::addQueryParameters($uri, ['foo' => 'bar']) instead of $uri->withQuery(Query::build(array_merge(Query::parse($uri->getQuery(), ['foo' => 'bar'])))

[mhsdesign]

Id be find also with a utility, even though its not that easily usable, but less pain on our end. Also we can simply document in the uribuilders that this utility exists and how to use it ;)

UriHelper::withAdditionalQueryParameters($this->someUriBuilder->uriFor(...), ['q' => 'search term']);

will implement this accordingly ;) And thanks for your extensive feedback!

[mhsdesign]

Okay added a UriHelper and removed also the old Uri tests, as we dont have an own implementation, and can assume that guzzle tests its stuff :D

[mficzel]

totally fine by 👀 ... also no risk in here

[mhsdesign]

Lol i just found out we already have a \Neos\Flow\Http\Helper\UriHelper :D Have to reevaluate where to put this then :D
And also \Neos\Flow\Mvc\Routing\Dto\UriConstraints::withAddedQueryValues seems to do something similar :D

[bwaidelich]

Nice one, +1 by reading.
I'm wondering whether this could be used as an Eel helper, too?
(implementing ProtectedContextAwareInterface and allowing string as input)

That way we could use it something like:

uri = Neos.Fusion:UriBuilder {
    // ...

    @process.addQuery = ${Neos.Flow.uri.withAdditionalQueryParameters(value, {foo: 'bar'})}
}

or was the plan to extend the built-in prototype implementations to support this?

in any case, this can be tackled in separate PRs ofc

[mhsdesign]

Wait a second i think i wanted to add the method to

flow-development-collection/Neos.Flow/Classes/Http/Helper/UriHelper.php

Line 19 in d8bed81

abstract class UriHelper

as otherwise we would have two UriHelper's 😂 lol (see #3316 (comment))

thanks for the plenty +1 but i prefer to merge my own prs :D

[mhsdesign]

The followup which was due since some time but i was to scared to jump into another rabbit hole: #3336