knp-components

knp-components copied to clipboard

There is a problem in v2.2.0 vs v1.3.10.

https://github.com/KnpLabs/knp-components/blob/master/src/Knp/Component/Pager/Paginator.php#L108

When running a test suite for example, $requestStack->getCurrentRequest() returns null, which leads to an error on the above line when trying to access $this->request->query - Trying to get property 'query' of non-object.

You can instantiate a RequestStack object and pass it to constructor. Anyway, some context could be useful to understand your problem

I am getting knp_paginator from the container. You could see more context here, where I get the error: https://github.com/sineflow/ElasticsearchBundle/blob/2019-10-22-knp-fix/Tests/Functional/Finder/Adapter/KnpPaginatorAdapterTest.php#L142

But the point is, that here https://github.com/KnpLabs/knp-components/blob/master/src/Knp/Component/Pager/Paginator.php#L64 you basically rely to always get a Request object, which is not always the case - you could get null (If you have a RequestStack, but not a Request object in it). And this situation is not handled below at line 108

Do you know a practical case where RequestStack has no Request inside? It looks like your test is a functional test, so you should use Symfony client and make a request (then, you should get a non-empty RequestStack).

I do not, but I imagine if I tried to use the paginator from a Console Command, there wouldn't be a Request either. Plus, I wouldn't dare to assume I know all possible practical cases, if the method signature itself specifies that null is a possible return value:

    /**
     * @return Request|null
     */
    public function getCurrentRequest()

The case of console command was object of an old issue and it's already covered. Your case should be covered too (let me know if it's not). For other cases, I guess that will be handled as soon as they will emerge

I suppose you are referring to https://github.com/KnpLabs/KnpPaginatorBundle/issues/563 ?

Yes, if I use the $kernel->getContainer()->get('request_stack')->push(new Request()); hack, I can get my test working, but I don't think that's the point.

But it looks like your official stance is that the paginator is just not intended to be used in a non-web context, so I guess that resolves it.

Actually, main purpose is web, but non-web context are not excluded. See for example issue #221

Anyway, I guess that the whole thing should work without a Request inside. Likely it would require a bit of work in many classes, but I'm confident that is doable. Feel free to open a PR

This is solved in v4