Pagination returning wrong results

[nicktan147]

Preconditions

1. XAMPP, Windows 10
2. Magento 2.1

Steps to reproduce

1. Installed Magento 2.1 with Sample Data, no other extension installed. Except this custom module.
2. The code, assuming the setCurPage increases each time

$categoryProducts = $categoryFactory->create()
							      ->load($categoryId)
			    				      ->getProductCollection()
							      ->setPageSize(5)
							      ->setCurPage(1); 

3. This category should have 10 products,
   Page 1 -> 5 products shown
   Page 2 -> 5 products shown
   Page 3 -> Same products from Page 2.

Expected result

1. Page 3 shouldn't have shown any products at all since there is only 10 products.

Actual result

1. This category should have 10 products,
   Page 1 -> 5 products shown
   Page 2 -> 5 products shown
   Page 3 -> Same products from Page 2.

[korostii]

If you are looking for some help or an advice regarding development please refer to the Community Forums or the Magento Stack Exchange.
The GitHub issue tracker is intended for tracking technical issues (read: "bugs") only.
The issues which cannot be consistently reproduced on a clean installation of Magento 2 CE aren't likely to get resolved in here.
Thus if you are, in fact, reporting a technical issue, please try add as much information as possible, including the environment you use, 3rd party themes and modules installed, preconditions, exact steps to follow, and even the custom code you're using. These raise the chances of the bug getting reproduced and acknowledged.
If you don't provide enough information, it is likely that the issue will receive a "needs update" label and will get closed in a couple weeks.

[nicktan147]

@korostii

Updated as requested.

[korostii]

I confirm this issue, I happened to see this effect myself recently, thus I shall provide some additional background information about the issue:
The limit seems to get applied to the collection here and the getCurPage() method has some extra logic which always provides a valid (wrong, if the original page number you've sent in is negative or too large).

\Magento\Framework\Data\Collection gives out first or last page's result when the requested page number is invalid. It shouldn't be the default behavior because it is completely unreasonable. This is clearly a bug!

As a workaround for your custom code, I'd advise to add an extra check just like the Magento's own Pager block does. In your example, on the page 2 (last page) it will prevent the "next page" button from appearing.

Back to the collection's implementation, I see the following possible solutions:
a) move those checks from getCurPage() to setCurPage() forcing it to throw an \InvalidArgumentException if the requested page number is not available
b) catch it when loading the collection and return an empty array instead.

[orlangur]

Page 3 -> Same products from Page 2.

Page 3 should not be accessed in the first place. Please provide a scenario reproducible on clean Magento installation when you saw a third page in such situation.

the getCurPage() method has some extra logic which always provides a valid (wrong, if the original page number you've sent in is negative or too large).

That is the whole point. When currentPage provided is too small, it is treated as first page, when it is too big - as last page.

It is clearly not a responsibility of collection class to check if currentPage provided is valid. Just compute pages properly and do not set invalid page into collection.

[korostii]

It is clearly not a responsibility of collection class to check if currentPage provided is valid. Just compute pages properly and do not set invalid page into collection.

Well the collection tries to check that anyway, and it sucks at it:

magento2/lib/internal/Magento/Framework/Data/Collection.php

Lines 243 to 247 in 9224419

	if ($this->_curPage + $displacement < 1) {
	return 1;
	} elseif ($this->_curPage + $displacement > $this->getLastPageNumber()) {
	return $this->getLastPageNumber();
	} else {

Imagine you wanna go 100 miles on a bus, request a a ticket from the driver (which driver provides you, no questions asked), enter the bus, fall asleep and several hours later you find yourself staring at a small village that does seem very familiar because the driver has been driving you around the block the whole time.
They just don't go outta town, apparently. Well, why didn't the driver just say so?

Page 3 should not be accessed in the first place. Please provide a scenario reproducible on clean Magento installation when you saw a third page in such situation.

For once, you can type it directly, this can be reproduced on stock magento.
This a very strange behavior. I would understand if the controller would redirect such request to a different page of if the block would show a "no products found" or "bad request" error, but it seem to be a "valid" page URL from Magento's point of view (which it's not).

Not just that, it's also implemented on the Collection level. Thus potentially it might also lead to obscure bugs producing invalid data (as opposed to fatal errors) elsewhere, impossible to be detected.

And there's no easy way to validate the page number short of accessing the Request object, because both the collection and the pager block always gives you a valid page number (see above).

I mean, the single place which has knowledge of "how many pages there are available total" doesn't produce an error or provide any other way to find out whether the requested page number has hit the limit after the fact. How messed up is that?

[orlangur]

For once, you can type it directly, this can be reproduced on stock magento:
http://magento2-demo.nexcess.net/women/tops-women/tanks-women.html?p=-10
http://magento2-demo.nexcess.net/women/tops-women/tanks-women.html?p=100500
Or even: http://magento2-demo.nexcess.net/women/tops-women/tanks-women.html?p=asdasdasd

This is clearly a responsibility of controller, not collection, to validate such cases.

And there's no easy way to validate the page number short of accessing the Request object, because both the collection and the pager block always gives you a valid page number (see above).

I mean, the single place which has knowledge of "how many pages there are available total" doesn't produce an error or provide any other way to find out whether the requested page number has hit the limit after the fact. How messed up is that?

That's make sense, it would be nice to have a class where all pagination logic is consolidated (maybe there is some symfony/pager ready to use?).

Just that currently Collection works as intended (it is behavior derived from Magento 1, isn't it?) and if it will start to throw exceptions neither developers nor merchants will like it :)

[korostii]

This is clearly a responsibility of controller, not collection, to validate such cases.

So we agree it's a bug?

Just that currently Collection works as intended (it is behavior derived from Magento 1, isn't it?) and if it will start to throw exceptions neither developers nor merchants will like it :)

That hasn't stop Magento core developers from "improving" URL key functionality, has it?

Anyway, there's plenty of ways around that.
You can always just create a new collection class with sane behavior, deprecate the old one, and slowly move all the core code to use the newer one instead.

[orlangur]

So we agree it's a bug?

It's obviously a bug of controller, not collection.

That hasn't stop Magento core developers from "improving" URL key functionality, has it?

URL key functionality was IMPROVED in fact, not "improved". URL Rewrites in M1 where causing troubles while I don't remember complaints about pagination. As far I see for now, naive customization may lead to unexpected page numbers, no more than that.

You can always just create a new collection class with sane behavior, deprecate the old one, and slowly move all the core code to use the newer one instead.

I find collection behavior perfectly valid. What I was saying is that we can have some class, similar to Pager block, which will consolidate page numbers calculation logic.

[korostii]

It's obviously a bug of controller, not collection.

A bug is a bug.
Would you please care to remove the needs update label now that we agree on that?

[korostii]

I find collection behavior perfectly valid. What I was saying is that we can have some class, similar to Pager block, which will consolidate page numbers calculation logic.

First of all, that's nonsense (if we're leaving it at Block level) since you might require this logic somewhere on a different layer (REST API \ or whatever else). It should be part of the framework, just like the \Data\Collection is. Why not have an improved Magento\Framework\Paginated\Collection (or whatever) for future use?

And secondly, with current Collection implementation, that's just impossible. You just won't be able to integrate the page validation we're discussing into the \Pager without giving it higher-level information about the _pageVarName which is in itself an ugly hack. Currently, Pager block relies on Collection::getCurPage which always returns an incorrect (it's always valid, but it still ight be wrong i.e. different from the requested) page number.

Just think about it: you won't be able to rely on this block's validation unless you explicitly give it both the collection and the page number yourselves. That messes up CQRS in a major way: you cannot rely on the page number you get from this block because you need to wait until some 3rd class (Toolbar block in this case, I believe) calls setCollection on it.
Seriously, it will be invalid otherwise:

magento2/app/code/Magento/Theme/Block/Html/Pager.php

Lines 126 to 129 in 3419c71

	if (is_object($this->_collection)) {
	return $this->_collection->getCurPage();
	}
	return (int)$this->getRequest()->getParam($this->getPageVarName(), 1);

So, to reiterate (here in regard to product collection on catalog and catalogsearch pages):

1. Controller is the only place that has knowledge of the original request. But it needs to wait for the page to start rendering (i.e. until after _prepareLayout() calls complete, IIRC) because otherwise Pager doesn't have the Collection (with all those fancy filters applied) yet.
2. Pager is the middleman which knows a fair bit of everything but it is pushed around by everyone, thus it's not sure of anything and is unable to redirect the request with a mere Exception because it's just a block. It'll just get ignored anyway, so why even bother trying?
3. Collection has all the knowledge it needs but prefers to ignore the invalid page numbers as a nightmare, replacing them with better ones at will. Oh and also, it doesn't know where to get the page size from - only Pager seems to know that one.

P.S. I think this little tangled family of crazies got somewhat refactored in 2.2 but it's still as fragile, as a Rube Goldberg machine.

[orlangur]

As explained above,

For once, you can type it directly, this can be reproduced on stock magento:
http://magento2-demo.nexcess.net/women/tops-women/tanks-women.html?p=-10
http://magento2-demo.nexcess.net/women/tops-women/tanks-women.html?p=100500
Or even: http://magento2-demo.nexcess.net/women/tops-women/tanks-women.html?p=asdasdasd

is the only thing which may be considered as a bug since it may lead to DoS attack. Collection in it's current implementation works as intended.

Closing as original scenario is not an issue but rather a bug in described custom implementation. Feel free to report a new issue describing a probable DoS attack (note https://github.com/magento/magento2#reporting-security-issues) or propose a pull request containing suggested changes.