FEATURE: Introduce routingArguments and queryParameters to Fusion link prototypes to replace arguments and additionalParams

[mficzel]

The Fusion prototype Neos.Fusion:ActionUri has two additional properties:

• :routingArguments: (array) That are handled by the router
• :queryParameters: (array) Query parameters that are appended after routing

Those will eventually replace the properties arguments and additionalParams which are deprecated and will be removed with Neos 9.

The Fusion prototypes Neos.Fusion:NodeUri and Neos.Fusion:NodeLink have one additional property:

• :queryParameters: (array) Query parameters that are appended after routing

This will eventually replace the property additionalParams which is deprecated and will be removed with Neos 9.

Also this pr deprecates the fusion properties addQueryString and argumentsToBeExcludedFromQueryString from the Neos.Fusion:ActionUri, Neos.Neos:NodeUri, and Neos.Neos:NodeLink.

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]

Not entirely sure wether the appending of queryParameters via

$uri = new Uri($uriString);
parse_str($uri->getQuery(), $queryParametersFromRouting);
return (string)$uri->withQuery(http_build_query([...$queryParametersFromRouting, ...$queryParameters])); 

is already the best way.

[mhsdesign]

thx for that clean up !!!

yummi pass by reference 😄
that confused me for a sec as i didnt remember that beautifull php api

but there seems no better way - other than have some flow uri helper.

like https://github.com/neos/flow-development-collection/blob/72ec464b47af1925b8488f13e8c13038218cf5df/Neos.Utility.Unicode/Classes/Functions.php#L180

[mficzel]

Played with Guzzle/Psr7/Query::parse and build but those do not work well with nested parameters

[mhsdesign]

hmm okay but in that case this is even more complex behavior. Sorry to suggest that (as nobody likes the current fusion tests) but we need to test a bit...

[mficzel]

@mhsdesign added tests for the actionUriImplementation ... i would like to discuss the following questions

1. How to merge queryArgumentsAfterRouting with queryParameters or arguments with routingArguments should happen. Is [...array1, ..$array2] correct or should we better use Arrays:arrayMergeRecursiveOverrule instead?
   For now queryParameters are merged with arrayMergeRecursiveOverrule like in https://github.com/neos/flow-development-collection/blob/67b96019761e203ee143561a5495c4fc0ba9e8e5/Neos.Flow/Classes/Mvc/Routing/Dto/UriConstraints.php#L219-L228. RoutingArguments will simply override arguments as those shall replace them eventually anyways

2. How should nested params be encoded ?foo%5Bbar%5D=baz or ?foo[bar]=baz

[mhsdesign]

1. How to merge queryArgumentsAfterRouting with queryParameters or arguments with routingArguments should happen. Is [...array1, ..$array2] correct or should we better use Arrays:arrayMergeRecursiveOverrule instead?

i think one should not be allowed to use the new option side by side: [... $this->getArguments(), ...$this->getRoutingArguments()]

this should throw an exception to be on the safe side ;)

eg - either new or old would be my guess

[mficzel]

@bwaidelich as sidenote: Since adding parameters to an existing url is not that comfortable i now think that the future solution should still have a way to define queryParameters directly.

[markusguenther]

I push this back to in progress... we need to rebase to 8.3 and resolve the conflict. But guess we will be back soonish in in review ;)

[mhsdesign]

I think its a bit hard to process in my head what should happen in certain cases when both arguments of the deprecated and new api are used.

How about before evaluation we check if the combination is valid? Basically what we already did with: throw new RuntimeException('Neos.Fusion:ActionUri does not allow to combine "arguments" and "routingArguments"', 1665431866) but for all params. What do you say?

This is based on the assumption, that you dont need to use the legacy api anymore but can rewrite the behavior always with the new api ...

[mficzel]

@mhsdesign the weirdness is in the handling of the "additionalParams" that are passed to "$uriBuilder->setArguments" here https://github.com/neos/neos-development-collection/pull/3914/files#diff-d87939b0060650e9ffd72fdde486f702f8b668a35673f74b5eaa49243c5804caL172-L175

We could throw an exception when "additionalParams" are used together with "queryParameters" but since this is not needed for this to work i would only deprecate this now.

The other deprecations addQueryString and argumentsToBeExcludedFromQueryString are just bad features that shall be removed in future without any replacement.

[mhsdesign]

That would look great in a flow uri utility or service, but I know the hassle since it’s in another repo… it might be enough time to pull it up (and you have my +1)

Advantage beeign that we can super easily tests this ugly code snippet isolated against all scenarios.

… or we can always later adjust it too (which is more likely a never thought ^^)

[mficzel]

Eventually this code will be replaced by an all new action-uri-builder on the flow side. See neos/flow-development-collection#2744 (not finished and still in discussion). Once this is in place ActionUri should be adjusted to use this internally.

In the meantime this pr allows to handle the negative effects of additionalParams ending up in the routingCache and introduces the distinction between routingArguments and queryParameters.

[mhsdesign]

Okay i see then its totally fine with inlining this temporarily ;)

[mhsdesign]

FYI now with neos/flow-development-collection#3316 in place we can actually use a proper helper instead ;)

[mficzel]

I think this is ready to review again.

[kitsunet]

I have a couple of questions, born out of the discussion in the sprint huddle right now, I guess conceptual things that might have been discussed before?

argumentsToBeExcludedFromQueryString
Rare case but I have had use for this with external trackign/seo whatever parameters. I guess a way to make sure certain query parameter are kept (if they are there) or to remove them (if they are there) could be helpful for those situations, as there is no other way to do it then (apart from string replacing after building the url?)?

route encoded strings that should not be part of the route according to the new concept
Eg. the (awful) seo wish of .../search//page/1 or something as URL, we would need a way to add those to the path without adding them to the route then? I guess in such cases you could say we restrict it to a basic route ala ..../search/ and an uncached appended section that is essentially query parameters? IDK I had these kind of use cases quite often over the years and just saying "it's not possible" seems not great.

appendExceedingArguments
Yes I agree this should die, but then having some argument that is not encoded in the URL for reasons BUT becomes part of the cache is again one of those edge use cases that proof to be useful at times. I guess this is the least problematic though, I don't have an amazingly great example in mind for using it this way.

[mhsdesign]

@mficzel what do you think about @kitsunet questions?

[mficzel]

@kitsunet @mhsdesign

Regarding argumentsToBeExcludedFromQueryString and addQueryString i think this should not be a core feature in UriBuilding. It can be added to a generated uri with a custom helper. This would make weird things slightly more complicated which is a good thing.

"route encoded strings that should not be part of the route according to the new concept" i do not get how this is affected. Even today one would create a special route with that parameter. Nothing will stop anyone of doing that. Maybe i did not get that point.

Regarding appendExceedingArguments i see no way to get rid of this now or in the near future. As long as routing is handled like a black box for the code generating the uri. That is why we can discourage it and document the problems but i think we cannot deprecate it.

[mhsdesign]

I think we discussed at the sprint to remove the appendExceeding arguments stuff and co (patterns from typo 3 times) as this will be no longer supported after the overhauled node uri building: #4892

[mhsdesign]

Fyi extracted the discussion regarding argumentsToBeExcludedFromQueryString and addQueryString into #5076 to be better visible and linkable.

[mhsdesign]

Btw what is the way we would like to promote to access query parameters in fusion?

currently documented: ${request.arguments.myQuery} (requires appendExceedingArguments as far as i know??)

vs

${request.httpRequest.queryParams.myQuery}

the latter, more correct? way is definitely more cumbersome ...

[bwaidelich]

${request.arguments.myQuery}
vs.
${request.httpRequest.queryParams.myQuery}

Those are two different things and IMO we should really make that distinction clear.
I think you should use the latter if you are interested in query parameters.

[kitsunet]

We are mixing two things here, actual (MVC) route parameters and other (http) queryParameters, IMHO the two removed concepts were mostly about other query parameters. So I would putt any query parameter handling outside of the router and thus appendExceedingArguments doesn't affect this. As in if you want an argument to be used in a route you pass it as argument and the router will decide (based on route config and appendExceedingArguments) what happens to this. But if you just want to add query parameters you should just get the Uri object resulting from the router and manipulate that.

[mficzel]

@kitsunet that is why the original suggestion added routingArguments and queryParameters ... as the distinction between them is important. The queryParameters should not be handled by the router but appended to the generated uri.

AppendExceedingArguments would then only be relevant if someone passes stuff to as routingArgument that should be a queryParameter.

[kitsunet]

Ype very good! :)

[mficzel]

The only problem i see here is that it kindof implements the Neos side of a change in the Flow behavior that is not yet implemented. Such changes caused trouble in the past so i would rather add routingArguments and queryParameters to the Flow UriBuilder beforehand.

[mhsdesign]

If and only if everything would be as (i) wish this would be the flow change: neos/flow-development-collection#2744

At its current state it has the flaw of not working for subrequests, but its basically the new uribuilder for flow because its better to start on a blank sheet in this case. (Same what were doing with the new node uri builder #4892)