Discussion: Removal of concepts `addQueryString` and `argumentsToBeExcludedFromQueryString` in Neos

mhsdesign commented 1 month ago

As part of improving our flow routing and uri building we discussed to also adjust our fusion prototypes and the Linking service accordingly.

It was originally planned to deprecate addQueryString and argumentsToBeExcludedFromQueryString for Neos 9.0:

Also this pr [#3914] deprecates the fusion properties addQueryString and argumentsToBeExcludedFromQueryString from the [prototypes ...] Neos.Neos:NodeUri, and Neos.Neos:NodeLink

The docs state:

:addQueryString: (boolean) Whether to keep the query parameters of the current URI :argumentsToBeExcludedFromQueryString: (array) Query parameters to exclude for addQueryString

public discussion so far

@mficzel said:

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

@kitsunet noted (Dresden sprint 23):

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.

Which was answered by @mficzel

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.

how to move forward?

We rediscussed this topic briefly again in the HH Sprint March and we came to the conclusion that as the features are artefacts of the past (typo3) they can be safely removed with Neos 9. They should not be part of a new uri builder api and thus are not fundamentally supported.

We could think of an anti corruption layer in front but i dont know if that is possible - it looks like some effort - and for that i have to understand how the conceptional outdated and obsolete features should work. At least we thought it would not be worth the effort as there are more dedicated replacement and patterns to solve the once thought usecase.

So We need to make a final decision in order to move forward with the overhaul of the new uri builder.

When we agree to remove the features we should also write a little upgrade note.

links

related pr to improve flow's uri building (shows the direction we want to head to): https://github.com/neos/flow-development-collection/pull/2744
pr to overhaul fusion Neos.Fusion:ActionUri and deprecate said properties: https://github.com/neos/neos-development-collection/pull/3914
pr to introduce new node uri builder as replacement for linking service and remove said properties: https://github.com/neos/neos-development-collection/pull/4892 (overhaul, but the current implementation is not released yet)

kitsunet commented 1 month ago

I would drop argumentsToBeExcludedFromQueryString for now. We could make the queryString manipulation for addQuerystring be a part of the fusion implementation instead of passing this down? ala fusion lets the router create an Uri and then just appends a given query string (DataStructure?) to this Uri object? that would be trivial to implement and removes the feature from routing.

mhsdesign commented 1 month ago

I think were talking slightly past each other. addQueryString is a boolean flag:

:addQueryString: (boolean) Whether to keep the query parameters of the current URI :argumentsToBeExcludedFromQueryString: (array) Query parameters to exclude for addQueryString

That means its yes is separate from the routing - currently handled by Flows uri builder - but might still not be sensible to reimplement.

The flow uribuilder code is obscured due to the nature of subrequests but its current implementation looks like this: https://github.com/neos/flow-development-collection/blob/32055e5c966579509a3ec06a3520b246696bcdc4/Neos.Flow/Classes/Mvc/Routing/UriBuilder.php#L452:

protected function mergeArgumentsWithRequestArguments(array $arguments)
{
    // ... (sub-request stuff) ...
    if ($this->request->isMainRequest() && $this->addQueryString === true) {
        $requestArguments = $this->request->getArguments();
        foreach ($this->argumentsToBeExcludedFromQueryString as $argumentToBeExcluded) {
            unset($requestArguments[$argumentToBeExcluded]);
        }
        if ($requestArguments !== []) {
            $arguments = Arrays::arrayMergeRecursiveOverrule($requestArguments, $arguments);
        }
    }
    return $arguments;
}

That means to archive the same feature without these little helpers, one could write this in Fusion:

- root = Neos.Fusion:NodeUri {
-    addQueryString = true
-    argumentsToBeExcludedFromQueryString = ${['bar']}
- }
+ root = Neos.Fusion:NodeUri {
+    additionalParams = ${request.arguments.filter((value, key) => key != 'bar')}
+ }

Alternatively with a proper introduction of queryParameters as option for fusion that will use the new UriHelper::uriWithAdditionalQueryParameters (https://github.com/neos/flow-development-collection/pull/3316) one could even use queryParameters directly instead of the to be deprecated additionalParams that leverage append exceeding arguments internally. Basically we definitely want #3914 for 9.0. (Or maybe for even back-ported for 8.x)

As it seems to be easier than assumed to offer a b/c layer in Fusion, where we still have the action request at hand, i could reimplement it for 9.0

kitsunet commented 1 month ago

Right yeah the bool is meh it can go I guess

mhsdesign commented 1 month ago

Actually i could imagine implementing it like:

$nodeUriBuilder->uriFor($nodeAddress, Options::create()->withAddedQueryString($actionRequest, excludedArguments: ['bar']);

its just an odd api, but would work and re enable the usecase for the LinkingService, NodeUri in Fusion & Fluid for Neos 9.0

But with the expense of carrying this thing around another time - till Neos 10. I think the additionalParams = ${request.arguments} "hack" is more than enough for people really relying on this dated feature.

mficzel commented 1 month ago

This should also be removed from the flow UriBuilder

neos / neos-development-collection