Error when passing an array as sort direction parameter in ORM pagination

[MalteWunsch]

Affected version: 3.6.0 (possibly 4.1.0 as well)

When using ORM pagination and calling something like http://localhost/galery/page/2?direction[test]=1&sort=columnname, the direction parameter becomes an array. This will result in an array to string conversion error in the 3.6.0 QuerySubscriber. The 4.1.0 QuerySubscriber looks like it could be affected, too.

First, I thought about a quick is_string() check. But then I noticed that other places like the SlidingPagination in the KnpPaginatorBundle rely on the direction parameter to be a string, too. It seems that more than one class gets the value directly from the request. So my current idea is fixing the request with a kernel event subscriber like this draft:

final class SanitizeKnpPaginationParameters implements EventSubscriberInterface
{
    public static function getSubscribedEvents(): array
    {
        return [
            KernelEvents::REQUEST => ['onKernelRequest', 10],
        ];
    }

    public function onKernelRequest(RequestEvent $event): void
    {
        $direction = $event->getRequest()->query->get('direction');

        if (null === $direction || \is_string($direction)) {
            return;
        }

        $event->getRequest()->query->set('direction', 'desc');
    }
}

What do you think? I'm not convinced this steamroller approach is an elegant solution.

I'd be happy to open a PR when we have a good notion on how to do it. (If so, could I target a 3.x branch or are PRs only accepted on the current major?)

[mpdude]

#306 might be related, but is in a very early discussion stage

[mpdude]

IMHO, using an EventSubscriber approach like the one above is a bit like "I don't know who and where you're going to use it, so I'll fix this upfront in a central place".

That's surprising for developers working on this library, since you wonder (no obvious hints) where the type conversion happens.

Also, from looking at the code in the event subscribers, you might also ask whether or not this type-checking takes place at all. Includes static analysis tools that may not see the actual types.

What about using \Symfony\Component\HttpFoundation\ParameterBag::filter() to access these parameters, passing FILTER_REQUIRE_SCALAR for $options?

[garak]

Unfortunately the RFC in #306 didn't gain momentum, but anyway the implementation already started, and in the 4.x branch you can access arguments using your own implementation.

[garak]

The latest major version implemented what proposed in #306
Did you try it?

[mpdude]

Not sure whether #306 – an abstraction on top of the request parameters – is what was asked for in the first place.

Anyways, since Symfony 6 (symfony/symfony#37265) the InputBag will throw a BadRequestException when the request data contains an array in a place where a scalar value is expected. Turning e. g. the sort direction parameter into an array (like ?direction[]=asc) will run into that exception, not into downstream issues in the paginator code.

[garak]

@mpdude this is currently a problem, because ArgumentAccessInterface doesn't allow for array values in get and set.
We need to fix it in a major release.