Skip to content

25038 Only set collection to toolbar once in ListProduct block #26934

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

25038 Only set collection to toolbar once in ListProduct block #26934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dd2e1a2
25038 Only set collection to toolbar once in ListProduct block
wigman Feb 19, 2020
31ddbf5
static test fix, cover changes with unit test
Nazar65 Feb 20, 2020
fb6f282
add missing tests
Nazar65 Feb 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion app/code/Magento/Catalog/Block/Product/ListProduct.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,7 @@ public function getIdentities()
}

foreach ($this->_getProductCollection() as $item) {
// phpcs:ignore Magento2.Performance.ForeachArrayMerge
$identities = array_merge($identities, $item->getIdentities());
}

Expand Down Expand Up @@ -516,7 +517,9 @@ private function configureToolbar(Toolbar $toolbar, Collection $collection)
$toolbar->setModes($modes);
}
// set collection to toolbar and apply sort
$toolbar->setCollection($collection);
if (!$toolbar->getCollection()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not really logical to solve issues with initializing toolbar object in the client code. In case if someone use it in custom module - issue will appear again. I think it should be moved inside Toolbar class as internal logic.

I'm not totally sure what is the best solution would be there, but not setting collection not looks logically here.

Could you describe with more details how issue actually appears?

$toolbar->setCollection($collection);
}
$this->setChild('toolbar', $toolbar);
}
}
100 changes: 81 additions & 19 deletions app/code/Magento/Catalog/Test/Unit/Block/Product/ListProductTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,89 +9,92 @@
use Magento\Framework\Event\ManagerInterface;
use Magento\Framework\Pricing\Render;
use Magento\Framework\Url\Helper\Data;
use Magento\Framework\View\LayoutInterface;
use PHPUnit\Framework\TestCase;

/**
* @SuppressWarnings(PHPMD.CouplingBetweenObjects)
*/
class ListProductTest extends \PHPUnit\Framework\TestCase
class ListProductTest extends TestCase
{
/**
* @var \Magento\Catalog\Block\Product\ListProduct
*/
protected $block;

/**
* @var \Magento\Framework\Registry|\PHPUnit_Framework_MockObject_MockObject
* @var \Magento\Framework\Registry|MockObject
*/
protected $registryMock;

/**
* @var \PHPUnit_Framework_MockObject_MockObject
* @var MockObject
*/
protected $layerMock;

/**
* @var \Magento\Framework\Data\Helper\PostHelper|\PHPUnit_Framework_MockObject_MockObject
* @var \Magento\Framework\Data\Helper\PostHelper|MockObject
*/
protected $postDataHelperMock;

/**
* @var \Magento\Catalog\Model\Product|\PHPUnit_Framework_MockObject_MockObject
* @var \Magento\Catalog\Model\Product|MockObject
*/
protected $productMock;

/**
* @var \Magento\Checkout\Helper\Cart|\PHPUnit_Framework_MockObject_MockObject
* @var \Magento\Checkout\Helper\Cart|MockObject
*/
protected $cartHelperMock;

/**
* @var \Magento\Catalog\Model\Product\Type\Simple|\PHPUnit_Framework_MockObject_MockObject
* @var \Magento\Catalog\Model\Product\Type\Simple|MockObject
*/
protected $typeInstanceMock;

/**
* @var Data | \PHPUnit_Framework_MockObject_MockObject
* @var Data|MockObject
*/
protected $urlHelperMock;

/**
* @var \Magento\Catalog\Model\ResourceModel\Category | \PHPUnit_Framework_MockObject_MockObject
* @var \Magento\Catalog\Model\ResourceModel\Category|MockObject
*/
protected $catCollectionMock;

/**
* @var \Magento\Catalog\Model\ResourceModel\Product | \PHPUnit_Framework_MockObject_MockObject
* @var \Magento\Catalog\Model\ResourceModel\Product|MockObject
*/
protected $prodCollectionMock;

/**
* @var \Magento\Framework\View\LayoutInterface | \PHPUnit_Framework_MockObject_MockObject
* @var \Magento\Framework\View\LayoutInterface | MockObject
*/
protected $layoutMock;

/**
* @var \Magento\Catalog\Block\Product\ProductList\Toolbar | \PHPUnit_Framework_MockObject_MockObject
* @var \Magento\Catalog\Block\Product\ProductList\Toolbar|MockObject
*/
protected $toolbarMock;

/**
* @var Context|\PHPUnit_Framework_MockObject_MockObject
* @var Context|MockObject
*/
private $context;

/**
* @var Render|\PHPUnit_Framework_MockObject_MockObject
* @var Render|MockObject
*/
private $renderer;

/**
* @inheritdoc
*/
protected function setUp()
{
$objectManager = new \Magento\Framework\TestFramework\Unit\Helper\ObjectManager($this);
$this->registryMock = $this->createMock(\Magento\Framework\Registry::class);
$this->layerMock = $this->createMock(\Magento\Catalog\Model\Layer::class);
/** @var \PHPUnit_Framework_MockObject_MockObject|\Magento\Catalog\Model\Layer\Resolver $layerResolver */
/** @var MockObject|\Magento\Catalog\Model\Layer\Resolver $layerResolver */
$layerResolver = $this->getMockBuilder(\Magento\Catalog\Model\Layer\Resolver::class)
->disableOriginalConstructor()
->setMethods(['get', 'create'])
Expand Down Expand Up @@ -137,7 +140,12 @@ protected function tearDown()
$this->block = null;
}

public function testGetIdentities()
/**
* Verify identitioes
*
* @return void
*/
public function testGetIdentities(): void
{
$productTag = 'cat_p_1';
$categoryTag = 'cat_c_p_1';
Expand Down Expand Up @@ -201,7 +209,56 @@ public function testGetIdentities()
);
}

public function testGetAddToCartPostParams()
/**
* Verify addToolbarBlock not ovveride product collection
*
* @return void
*/
public function testAddToolbarBlockCollection(): void
{
$currentCategory = $this->createMock(\Magento\Catalog\Model\Category::class);
$currentCategory->expects($this->any())
->method('getId')
->will($this->returnValue('1'));
$this->layerMock->expects($this->once())
->method('getProductCollection')
->willReturn($this->prodCollectionMock);
$this->layerMock->expects($this->any())
->method('getCurrentCategory')
->will($this->returnValue($currentCategory));
$this->layoutMock->expects($this->exactly(2))
->method('getBlock')
->will($this->returnValue($this->toolbarMock));
$this->toolbarMock->expects($this->at(3))
->method('getCollection')
->willReturn($this->prodCollectionMock);
$this->toolbarMock->expects($this->once())
->method('setCollection')
->with($this->prodCollectionMock);

$this->block->getLoadedProductCollection();
$addToolbarBlock = $this->getMethod('addToolbarBlock');
$addToolbarBlock->invokeArgs($this->block, [$this->prodCollectionMock]);
}

/**
* Access protected method
*
* @param string $name
*/
private function getMethod(string $name)
{
$class = new \ReflectionClass(\Magento\Catalog\Block\Product\ListProduct::class);
$method = $class->getMethod($name);
$method->setAccessible(true);
return $method;
}
/**
* Verify add to cart post params
*
* @return void
*/
public function testGetAddToCartPostParams(): void
{
$url = 'http://localhost.com/dev/';
$id = 1;
Expand Down Expand Up @@ -233,7 +290,12 @@ public function testGetAddToCartPostParams()
$this->assertEquals($expectedPostData, $result);
}

public function testSetIsProductListFlagOnGetProductPrice()
/**
* Verify ListFlag on get product price
*
* @return void
*/
public function testSetIsProductListFlagOnGetProductPrice(): void
{
$this->renderer->expects($this->once())
->method('setData')
Expand Down