Reset page size before load

[DanieliMi]

Summary

The page size is already reset in _renderFiltersBefore but might be changed between rendering the filters and loading the collection. If that happens no items will be found.

Related Issues

#249

Description

When moving the navigation (catalog.leftnav) it will change the load order of the blocks. In our case we moved it in catalog_category_view_type_layered.xml:

<move element="catalog.leftnav" destination="content" before="-"/>

When rendering a category view \Magento\Catalog\Block\Product\ProductList\Toolbar::setCollection is called multiple times. Within setCollection $this->_collection->setCurPage($this->getCurrentPage()); is called and will overwrite resetting the page size in _renderFiltersBefore if _renderFiltersBefore is called outside of \Magento\Framework\Data\Collection\AbstractDb::load since filters will be only rendered once (see \Magento\Framework\Data\Collection\AbstractDb::_renderFilters).

So we need to make sure _pageSize is false when loading the collection and setting it in _renderFiltersBefore is good but it might be overwritten if the method is called outside of \Magento\Framework\Data\Collection\AbstractDb::load.

Setting _pageSize to false in _beforeLoad() makes sure the page size is handled by the engine and the collection does not depend on block load order anymore.

The commit should be applied to 2.9.x as well since it might have the same issue (not verified by myself tho).

[amenk]

Tests are basically running through.
but phpmd shows errors

/home/travis/build/Smile-SA/elasticsuite/src/module-elasticsuite-
catalog/Model/ResourceModel/Product/Fulltext/Collection.php:31 The class Collection has an overall complexity of 50 which > is very high. The configured complexity threshold is 50.

I am wondering if that really changed because of the patch?

/home/travis/build/Smile-SA/elasticsuite/src/module-elasticsuite-
catalog/Model/ResourceModel/Product/Fulltext/Collection.php:453 The method _beforeLoad is not named in camelCase.

That does not make sense :-D The Method of course has to be called like that because we are overwriting the parent.

[amenk]

@DanieliMi can you quickly add a

 * @SuppressWarnings(PHPMD.CamelCaseMethodName)

to the _beforeLoad, like it is with the other methods.

@ maintainers: I am wondering if we should just ignore the class complexity as well? Or do we need major refactoring because of this small change? That would be a pity

[amenk]

@DanieliMi and this one:

   452 | ERROR | Missing @return tag in function comment

[androshchuk]

Hello everybody.
Thank you for your contribution.

You can add * @SuppressWarnings(PHPMD.ExcessiveClassComplexity) into class annotations
to suppress "The class Collection has an overall complexity of 50 which > is very high. The configured complexity threshold is 50."

and, please, use annotation * {@inheritDoc} instead * @inheritDoc
in this case, you don't have to add @return tag

Regards

[amenk]

all checks passed :-D does this have chances to be merged?

[Quazz]

This appears to break the solution for Infortis Ultimo theme mentioned in https://github.com/Smile-SA/elasticsuite/wiki/FAQ

It relies on sending the pageSize to the collection, because otherwise it tries to load everything and that just doesn't work of course.

Problematic code:
($collection being created from categoryLayerFactory)
$collection->setPage(1, $productCount) ->load();

EDIT: Even worse; this seems to break pagination on layered navigation (at least on elasticsuite 2.9.2), causing it to try to load the entire product collection of a category on page load.

@romainruaud @androshchuk @amenk @DanieliMi

Can you check this?

[gaiterjones]

Hi Guys, I've been struggling all day with what I thought was a custom theme / pagination issue with M2.3.3 and elasticsuite 2.8 but turns out to be the changes made by this commit. Even when I revert to the Luma theme with no custom code loading the _beforeLoad() function is setting pagesize to false causing all products in the category /. search / filter to be loaded. When I revert this commit everything works as normal. I guess this might be causing problems for a lot of users...

[amenk]

@romainruaud How can we proceed here, this also seems to cause #1903 ... but just reverting will also cause the old problems to appear.
It would be cool to have some tests in place here, not clue how to write them.

[romainruaud]

@amenk I don't know if all the tests would be easy to write :

• ensure that pagination is not broken : this could be done
• ensure that the pageSize is correct even if the catalog.leftnav is moved elsewhere in the layout : I'm not sure how to test this...

I'll try to have a look but actually we'll have to deliver a release containing the fix asap.

And by the way, I missed @Quazz message that was pointing the problem 19 days ago :/ As always, you did a good catch guy !

[amenk]

@romainruaud

ensure that the pageSize is correct even if the catalog.leftnav is moved elsewhere in the layout : I'm not sure how to test this...

good question, maybe some example layout XML files have to be created and tested against.
But I think if we make that possible, we could avoid a lot of headaches in the future.

[amenk]

It could work by having certain my_handle_1.xml files and write integration tests which inject the according handles and check the pagination on the according pages.