Skip to content

Commit

Permalink
Improve \Magento\Framework\Api\SortOrder
Browse files Browse the repository at this point in the history
Purpose
==

Make the specification of sort directions on sort criteria easier for developers.

Background
==

This PR addresses several issues:

**PHPDoc type hint mismatch**
All parameter type hints expecting a sort direction specify `@param string $direction`,
but the constants `SearchCriteriaInterface::SORT_ASC` and `SearchCriteriaInterface::SORT_DESC`
are integers.
As a developer, when I see a method `setDirection()` that takes a string, I expect the
argument to be either `ASC` or `DESC`. Neither work, but there is no error helpful error
message.

**Unintuitive placement of constants**
Also the location of the constants is not intuitive. When working with the code in question,
a developer is focused on the classes SortOrder and SortOrderBuilder. So the expectation
would be to find the class constants on either of those classes.

**Missing validation**
The sort direction can be specified as a scalar argument, but there is no validation.
This PR adds validation and throws an explanatory exception on validation failure.

Reasons for this PR
==

The motivation is to make it easier and more intuitive for developers to specify a sort order.
  • Loading branch information
Vinai committed Jul 21, 2015
1 parent fb011a0 commit 8e66e3a
Show file tree
Hide file tree
Showing 35 changed files with 243 additions and 75 deletions.
3 changes: 1 addition & 2 deletions app/code/Magento/Catalog/Model/ProductRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
use Magento\Framework\Api\Data\ImageContentInterfaceFactory;
use Magento\Framework\Api\ImageContentValidatorInterface;
use Magento\Framework\Api\ImageProcessorInterface;
use Magento\Framework\Api\SearchCriteriaInterface;
use Magento\Framework\Api\SortOrder;
use Magento\Framework\Exception\InputException;
use Magento\Framework\Exception\NoSuchEntityException;
Expand Down Expand Up @@ -658,7 +657,7 @@ public function getList(\Magento\Framework\Api\SearchCriteriaInterface $searchCr
$field = $sortOrder->getField();
$collection->addOrder(
$field,
($sortOrder->getDirection() == SearchCriteriaInterface::SORT_ASC) ? 'ASC' : 'DESC'
($sortOrder->getDirection() == SortOrder::SORT_ASC) ? 'ASC' : 'DESC'
);
}
$collection->setCurPage($searchCriteria->getCurrentPage());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

use Magento\Catalog\Model\Layer\Filter\Dynamic\AlgorithmFactory;
use Magento\Framework\Api\Data\ImageContentInterface;
use Magento\Framework\Api\SortOrder;
use Magento\Framework\TestFramework\Unit\Helper\ObjectManager;
use Magento\Store\Model\ScopeInterface;

Expand Down Expand Up @@ -602,7 +603,7 @@ public function testGetList()
$searchCriteriaMock->expects($this->once())->method('getSortOrders')->willReturn([$sortOrderMock]);
$sortOrderMock->expects($this->once())->method('getField')->willReturn('field');
$sortOrderMock->expects($this->once())->method('getDirection')
->willReturn(\Magento\Framework\Api\SearchCriteriaInterface::SORT_ASC);
->willReturn(SortOrder::SORT_ASC);
$collectionMock->expects($this->once())->method('addOrder')->with('field', 'ASC');
$searchCriteriaMock->expects($this->once())->method('getCurrentPage')->willReturn(4);
$collectionMock->expects($this->once())->method('setCurPage')->with(4);
Expand Down
4 changes: 2 additions & 2 deletions app/code/Magento/Cms/Model/BlockRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
use Magento\Cms\Api\Data;
use Magento\Cms\Api\BlockRepositoryInterface;
use Magento\Framework\Api\DataObjectHelper;
use Magento\Framework\Api\SearchCriteriaInterface;
use Magento\Framework\Api\SortOrder;
use Magento\Framework\Exception\CouldNotDeleteException;
use Magento\Framework\Exception\CouldNotSaveException;
use Magento\Framework\Exception\NoSuchEntityException;
Expand Down Expand Up @@ -146,7 +146,7 @@ public function getList(\Magento\Framework\Api\SearchCriteriaInterface $criteria
foreach ($sortOrders as $sortOrder) {
$collection->addOrder(
$sortOrder->getField(),
($sortOrder->getDirection() == SearchCriteriaInterface::SORT_ASC) ? 'ASC' : 'DESC'
($sortOrder->getDirection() == SortOrder::SORT_ASC) ? 'ASC' : 'DESC'
);
}
}
Expand Down
5 changes: 3 additions & 2 deletions app/code/Magento/Cms/Model/PageRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
use Magento\Cms\Api\Data;
use Magento\Cms\Api\PageRepositoryInterface;
use Magento\Framework\Api\DataObjectHelper;
use Magento\Framework\Api\SearchCriteriaInterface;
use Magento\Framework\Api\SortOrder;
use Magento\Framework\Exception\CouldNotDeleteException;
use Magento\Framework\Exception\CouldNotSaveException;
use Magento\Framework\Exception\NoSuchEntityException;
Expand Down Expand Up @@ -143,10 +143,11 @@ public function getList(\Magento\Framework\Api\SearchCriteriaInterface $criteria
$searchResults->setTotalCount($collection->getSize());
$sortOrders = $criteria->getSortOrders();
if ($sortOrders) {
/** @var SortOrder $sortOrder */
foreach ($sortOrders as $sortOrder) {
$collection->addOrder(
$sortOrder->getField(),
($sortOrder->getDirection() == SearchCriteriaInterface::SORT_ASC) ? 'ASC' : 'DESC'
($sortOrder->getDirection() == SortOrder::SORT_ASC) ? 'ASC' : 'DESC'
);
}
}
Expand Down
4 changes: 2 additions & 2 deletions app/code/Magento/Cms/Test/Unit/Model/BlockRepositoryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
namespace Magento\Cms\Test\Unit\Model;

use Magento\Cms\Model\BlockRepository;
use Magento\Framework\Api\SearchCriteriaInterface;
use Magento\Framework\Api\SortOrder;

/**
* Test for Magento\Cms\Model\BlockRepository
Expand Down Expand Up @@ -236,7 +236,7 @@ public function testGetList()
$storeFilter->expects($this->any())->method('getField')->willReturn('store_id');
$storeFilter->expects($this->once())->method('getValue')->willReturn(1);
$sortOrder->expects($this->once())->method('getField')->willReturn($sortField);
$sortOrder->expects($this->once())->method('getDirection')->willReturn(SearchCriteriaInterface::SORT_DESC);
$sortOrder->expects($this->once())->method('getDirection')->willReturn(SortOrder::SORT_DESC);

/** @var \Magento\Framework\Api\SearchCriteriaInterface $criteria */

Expand Down
4 changes: 2 additions & 2 deletions app/code/Magento/Cms/Test/Unit/Model/PageRepositoryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
namespace Magento\Cms\Test\Unit\Model;

use Magento\Cms\Model\PageRepository;
use Magento\Framework\Api\SearchCriteriaInterface;
use Magento\Framework\Api\SortOrder;

/**
* Test for Magento\Cms\Model\PageRepository
Expand Down Expand Up @@ -236,7 +236,7 @@ public function testGetList()
$storeFilter->expects($this->any())->method('getField')->willReturn('store_id');
$storeFilter->expects($this->once())->method('getValue')->willReturn(1);
$sortOrder->expects($this->once())->method('getField')->willReturn($sortField);
$sortOrder->expects($this->once())->method('getDirection')->willReturn(SearchCriteriaInterface::SORT_DESC);
$sortOrder->expects($this->once())->method('getDirection')->willReturn(SortOrder::SORT_DESC);

/** @var \Magento\Framework\Api\SearchCriteriaInterface $criteria */

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ public function getList(SearchCriteriaInterface $searchCriteria)
$field = $sortOrder->getField();
$collection->addOrder(
$field,
($sortOrder->getDirection() == SearchCriteriaInterface::SORT_ASC) ? 'ASC' : 'DESC'
($sortOrder->getDirection() == SortOrder::SORT_ASC) ? 'ASC' : 'DESC'
);
}
$collection->setCurPage($searchCriteria->getCurrentPage());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use Magento\Framework\Api\DataObjectHelper;
use Magento\Framework\Api\ImageProcessorInterface;
use Magento\Framework\Api\SearchCriteriaInterface;
use Magento\Framework\Api\SortOrder;
use Magento\Framework\Exception\InputException;
use Magento\Framework\Exception\NoSuchEntityException;

Expand Down Expand Up @@ -270,10 +271,11 @@ public function getList(SearchCriteriaInterface $searchCriteria)
$searchResults->setTotalCount($collection->getSize());
$sortOrders = $searchCriteria->getSortOrders();
if ($sortOrders) {
/** @var SortOrder $sortOrder */
foreach ($searchCriteria->getSortOrders() as $sortOrder) {
$collection->addOrder(
$sortOrder->getField(),
($sortOrder->getDirection() == SearchCriteriaInterface::SORT_ASC) ? 'ASC' : 'DESC'
($sortOrder->getDirection() == SortOrder::SORT_ASC) ? 'ASC' : 'DESC'
);
}
}
Expand Down
5 changes: 3 additions & 2 deletions app/code/Magento/Customer/Model/Resource/GroupRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use Magento\Customer\Model\Resource\Group\Collection;
use Magento\Framework\Api\Search\FilterGroup;
use Magento\Framework\Api\SearchCriteriaInterface;
use Magento\Framework\Api\SortOrder;
use Magento\Framework\Exception\InputException;
use Magento\Framework\Exception\State\InvalidTransitionException;
use Magento\Tax\Api\Data\TaxClassInterface;
Expand Down Expand Up @@ -184,13 +185,13 @@ public function getList(SearchCriteriaInterface $searchCriteria)
$this->addFilterGroupToCollection($group, $collection);
}
$sortOrders = $searchCriteria->getSortOrders();
/** @var \Magento\Framework\Api\SortOrder $sortOrder */
/** @var SortOrder $sortOrder */
if ($sortOrders) {
foreach ($searchCriteria->getSortOrders() as $sortOrder) {
$field = $this->translateField($sortOrder->getField());
$collection->addOrder(
$field,
($sortOrder->getDirection() == SearchCriteriaInterface::SORT_ASC) ? 'ASC' : 'DESC'
($sortOrder->getDirection() == SortOrder::SORT_ASC) ? 'ASC' : 'DESC'
);
}
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ public function testGetList()
->willReturn('Field');
$sortOrder->expects($this->once())
->method('getDirection')
->willReturn(1);
->willReturn(\Magento\Framework\Api\SortOrder::SORT_ASC);
$collection->expects($this->once())
->method('addOrder')
->with('Field', 'ASC');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -737,7 +737,7 @@ public function testGetList()
->willReturn('Field');
$sortOrder->expects($this->once())
->method('getDirection')
->willReturn(1);
->willReturn(\Magento\Framework\Api\SortOrder::SORT_ASC);
$searchCriteria->expects($this->once())
->method('getCurrentPage')
->willReturn(1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

use Magento\Framework\Api\SearchCriteria;
use Magento\Customer\Model\Resource\Group\Grid\ServiceCollection;
use Magento\Framework\Api\SortOrder;

/**
* Unit test for \Magento\Customer\Model\Resource\Group\Grid\ServiceCollection
Expand Down Expand Up @@ -81,7 +82,7 @@ public function testGetSearchCriteriaImplicitEq()
{
$sortOrder = $this->sortOrderBuilder
->setField('name')
->setDirection(SearchCriteria::SORT_ASC)
->setDirection(SortOrder::SORT_ASC)
->create();
/** @var SearchCriteria $expectedSearchCriteria */
$expectedSearchCriteria = $this->searchCriteriaBuilder
Expand Down Expand Up @@ -111,7 +112,7 @@ public function testGetSearchCriteriaOneField()
$value = '35';
$sortOrder = $this->sortOrderBuilder
->setField('name')
->setDirection(SearchCriteria::SORT_ASC)
->setDirection(SortOrder::SORT_ASC)
->create();
/** @var SearchCriteria $expectedSearchCriteria */
$filter = $this->filterBuilder->setField($field)->setConditionType($conditionType)->setValue($value)->create();
Expand Down Expand Up @@ -143,7 +144,7 @@ public function testGetSearchCriteriaOr()

$sortOrder = $this->sortOrderBuilder
->setField('name')
->setDirection(SearchCriteria::SORT_ASC)
->setDirection(SortOrder::SORT_ASC)
->create();
/** @var SearchCriteria $expectedSearchCriteria */
$expectedSearchCriteria = $this->searchCriteriaBuilder
Expand Down Expand Up @@ -179,7 +180,7 @@ public function testGetSearchCriteriaAnd()

$sortOrder = $this->sortOrderBuilder
->setField('name')
->setDirection(SearchCriteria::SORT_ASC)
->setDirection(SortOrder::SORT_ASC)
->create();
/** @var SearchCriteria $expectedSearchCriteria */
$expectedSearchCriteria = $this->searchCriteriaBuilder
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ public function testGetList()
->with('Field', 'ASC');
$sortOrder->expects($this->once())
->method('getDirection')
->willReturn(1);
->willReturn(\Magento\Framework\Api\SortOrder::SORT_ASC);
$searchCriteria->expects($this->once())
->method('getCurrentPage')
->willReturn(1);
Expand Down
6 changes: 3 additions & 3 deletions app/code/Magento/Eav/Model/AttributeRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
namespace Magento\Eav\Model;

use Magento\Eav\Model\Resource\Entity\Attribute\Collection;
use Magento\Framework\Api\SearchCriteriaInterface;
use Magento\Framework\Api\SortOrder;
use Magento\Framework\Exception\InputException;
use Magento\Framework\Exception\NoSuchEntityException;
use Magento\Framework\Exception\StateException;
Expand Down Expand Up @@ -118,11 +118,11 @@ public function getList($entityTypeCode, \Magento\Framework\Api\SearchCriteriaIn
foreach ($searchCriteria->getFilterGroups() as $group) {
$this->addFilterGroupToCollection($group, $attributeCollection);
}
/** @var \Magento\Framework\Api\SortOrder $sortOrder */
/** @var SortOrder $sortOrder */
foreach ((array)$searchCriteria->getSortOrders() as $sortOrder) {
$attributeCollection->addOrder(
$sortOrder->getField(),
($sortOrder->getDirection() == SearchCriteriaInterface::SORT_ASC) ? 'ASC' : 'DESC'
($sortOrder->getDirection() == SortOrder::SORT_ASC) ? 'ASC' : 'DESC'
);
}

Expand Down
5 changes: 3 additions & 2 deletions app/code/Magento/Quote/Model/QuoteRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
*/
namespace Magento\Quote\Model;

use Magento\Framework\Api\SortOrder;
use Magento\Framework\Exception\NoSuchEntityException;
use Magento\Quote\Model\Quote;
use Magento\Store\Model\StoreManagerInterface;
use Magento\Framework\Api\SearchCriteria;
use Magento\Framework\Api\Search\FilterGroup;
use Magento\Quote\Model\Resource\Quote\Collection as QuoteCollection;
use Magento\Framework\Exception\InputException;
Expand Down Expand Up @@ -220,10 +220,11 @@ public function getList(\Magento\Framework\Api\SearchCriteria $searchCriteria)
$searchData->setTotalCount($this->quoteCollection->getSize());
$sortOrders = $searchCriteria->getSortOrders();
if ($sortOrders) {
/** @var SortOrder $sortOrder */
foreach ($sortOrders as $sortOrder) {
$this->quoteCollection->addOrder(
$sortOrder->getField(),
$sortOrder->getDirection() == SearchCriteria::SORT_ASC ? 'ASC' : 'DESC'
$sortOrder->getDirection() == SortOrder::SORT_ASC ? 'ASC' : 'DESC'
);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

use \Magento\Quote\Model\QuoteRepository;

use Magento\Framework\Api\SearchCriteria;
use Magento\Framework\Api\SortOrder;
use Magento\Framework\Api\ExtensionAttribute\JoinProcessorInterface;

class QuoteRepositoryTest extends \PHPUnit_Framework_TestCase
Expand Down Expand Up @@ -404,8 +404,8 @@ public function testGetListSuccess($direction, $expectedDirection)
public function getListSuccessDataProvider()
{
return [
'asc' => [SearchCriteria::SORT_ASC, 'ASC'],
'desc' => [SearchCriteria::SORT_DESC, 'DESC']
'asc' => [SortOrder::SORT_ASC, 'ASC'],
'desc' => [SortOrder::SORT_DESC, 'DESC']
];
}
}
4 changes: 1 addition & 3 deletions app/code/Magento/Tax/Model/Calculation/RateRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,9 @@
use Magento\Directory\Model\CountryFactory;
use Magento\Directory\Model\RegionFactory;
use Magento\Framework\Api\Search\FilterGroup;
use Magento\Framework\Api\SearchCriteria;
use Magento\Framework\Api\SortOrder;
use Magento\Framework\Exception\InputException;
use Magento\Framework\Exception\LocalizedException;
use Magento\Framework\Exception\AlreadyExistsException;
use Magento\Tax\Model\Calculation\Rate;
use Magento\Tax\Model\Calculation\Rate\Converter;
use Magento\Tax\Model\Resource\Calculation\Rate\Collection;
Expand Down Expand Up @@ -168,7 +166,7 @@ public function getList(\Magento\Framework\Api\SearchCriteriaInterface $searchCr
foreach ($sortOrders as $sortOrder) {
$collection->addOrder(
$this->translateField($sortOrder->getField()),
($sortOrder->getDirection() == SearchCriteria::SORT_ASC) ? 'ASC' : 'DESC'
($sortOrder->getDirection() == SortOrder::SORT_ASC) ? 'ASC' : 'DESC'
);
}
}
Expand Down
3 changes: 1 addition & 2 deletions app/code/Magento/Tax/Model/TaxClass/Repository.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

use Magento\Framework\Api\FilterBuilder;
use Magento\Framework\Api\Search\FilterGroup;
use Magento\Framework\Api\SearchCriteria;
use Magento\Framework\Api\SearchCriteriaBuilder;
use Magento\Framework\Api\SortOrder;
use Magento\Framework\Exception\CouldNotDeleteException;
Expand Down Expand Up @@ -214,7 +213,7 @@ public function getList(\Magento\Framework\Api\SearchCriteriaInterface $searchCr
foreach ($searchCriteria->getSortOrders() as $sortOrder) {
$collection->addOrder(
$sortOrder->getField(),
($sortOrder->getDirection() == SearchCriteria::SORT_ASC) ? 'ASC' : 'DESC'
($sortOrder->getDirection() == SortOrder::SORT_ASC) ? 'ASC' : 'DESC'
);
}
}
Expand Down
3 changes: 1 addition & 2 deletions app/code/Magento/Tax/Model/TaxRuleRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
namespace Magento\Tax\Model;

use Magento\Framework\Api\Search\FilterGroup;
use Magento\Framework\Api\SearchCriteria;
use Magento\Framework\Api\SortOrder;
use Magento\Framework\Exception\LocalizedException;
use Magento\Framework\Exception\CouldNotSaveException;
Expand Down Expand Up @@ -163,7 +162,7 @@ public function getList(\Magento\Framework\Api\SearchCriteria $searchCriteria)
foreach ($sortOrders as $sortOrder) {
$collection->addOrder(
$this->translateField($sortOrder->getField()),
($sortOrder->getDirection() == SearchCriteria::SORT_ASC) ? 'ASC' : 'DESC'
($sortOrder->getDirection() == SortOrder::SORT_ASC) ? 'ASC' : 'DESC'
);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
*/
namespace Magento\Tax\Test\Unit\Model\Calculation;

use Magento\Framework\Api\SortOrder;
use \Magento\Tax\Model\Calculation\RateRepository;

use Magento\Framework\Api\SearchCriteria;
use Magento\Framework\TestFramework\Unit\Helper\ObjectManager;
use Magento\Framework\Exception\LocalizedException;
use Magento\Framework\Exception\AlreadyExistsException;
Expand Down Expand Up @@ -391,7 +391,7 @@ public function testGetListWhenFilterGroupExists()
->method('getSortOrders')
->will($this->returnValue([$sortOrderMock]));
$sortOrderMock->expects($this->once())->method('getField')->willReturn('field_name');
$sortOrderMock->expects($this->once())->method('getDirection')->willReturn(SearchCriteria::SORT_ASC);
$sortOrderMock->expects($this->once())->method('getDirection')->willReturn(SortOrder::SORT_ASC);
$collectionMock->expects($this->once())->method('addOrder')->with('main_table.field_name', 'ASC');
$currentPage = 1;
$pageSize = 100;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@

namespace Magento\Tax\Test\Unit\Model\TaxClass;

use Magento\Framework\Api\SortOrder;
use \Magento\Tax\Model\TaxClass\Repository;

use Magento\Framework\Exception\CouldNotDeleteException;
use Magento\Framework\Exception\LocalizedException;
use Magento\Framework\Api\SearchCriteria;

class RepositoryTest extends \PHPUnit_Framework_TestCase
{
Expand Down Expand Up @@ -215,7 +215,7 @@ public function testGetList()

$searchCriteria->expects($this->exactly(2))->method('getSortOrders')->willReturn([$sortOrder]);
$sortOrder->expects($this->once())->method('getField')->willReturn('field');
$sortOrder->expects($this->once())->method('getDirection')->willReturn(SearchCriteria::SORT_ASC);
$sortOrder->expects($this->once())->method('getDirection')->willReturn(SortOrder::SORT_ASC);
$collection->expects($this->once())->method('addOrder')->with('field', 'ASC');
$searchCriteria->expects($this->once())->method('getPageSize')->willReturn(20);
$searchCriteria->expects($this->once())->method('getCurrentPage')->willReturn(0);
Expand Down
Loading

0 comments on commit 8e66e3a

Please sign in to comment.