Skip to content
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.

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

SearchCriteriaBuilder builds wrong criteria (ORDER BY part) #5738

Closed
flancer64 opened this issue Jul 21, 2016 · 5 comments
Closed

SearchCriteriaBuilder builds wrong criteria (ORDER BY part) #5738

flancer64 opened this issue Jul 21, 2016 · 5 comments
Assignees
Labels
Fixed in 2.2.x The issue has been fixed in 2.2 release line Fixed in 2.3.x The issue has been fixed in 2.3 release line Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release

Comments

@flancer64
Copy link
Contributor

Empty SearchCriteria causes the error in \Magento\Framework\Data\Collection\AbstractDb::_setOrder
Target SQL has wrong ORDER BY clause (without any field inside):

SELECT ... ORDER BY  DESC

This is the code to get error:

class SalesRules
{
    /** @var \Magento\Framework\Api\Search\SearchCriteriaBuilder */
    protected $_builderSearchCriteria;
    /** @var \Magento\SalesRule\Api\RuleRepositoryInterface */
    protected $_repoRule;

    public function __construct(
        \Magento\Framework\Api\Search\SearchCriteriaBuilder $builderSearchCriteria,
        \Magento\SalesRule\Api\RuleRepositoryInterface $repoRule

    ) {
        $this->_builderSearchCriteria = $builderSearchCriteria;
        $this->_repoRule = $repoRule;
    }

    /**
     * Add sales rules.
     */
    public function init()
    {
        $crit = $this->_builderSearchCriteria->create();
        $all = $this->_repoRule->getList($crit);
    }
}

Method \Magento\SalesRule\Model\RuleRepository::getList adds order for $filed=null:
mage2_set_order_getlist

I suppose we need field validation in \Magento\Framework\Data\Collection\AbstractDb::_setOrder

private function _setOrder($field, $direction, $unshift = false)
{
    if($field) {
        ...
    }
    return $this;
}

or validation in the \Magento\SalesRule\Model\RuleRepository::getList:

    /** @var \Magento\Framework\Api\SortOrder $sortOrder */
    foreach ($sortOrders as $sortOrder) {
        $field = $sortOrder->getField();
        if($field) {
            $collection->addOrder(
                $field,
                ($sortOrder->getDirection() == SortOrder::SORT_ASC) ? 'ASC' : 'DESC'
            );
        }
    }
@flancer64
Copy link
Contributor Author

These are the variables in the \Magento\SalesRule\Model\RuleRepository::getList:
mage2_set_order_vars

@veloraven
Copy link
Contributor

@flancer64 thank you for your report.
Please, provide the used version. If the problem is actual for a specific tag, please, specify it and be sure that the latest update was used.

@flancer64
Copy link
Contributor Author

Magento ver. 2.1.0

@veloraven veloraven removed their assignment Aug 1, 2016
@magento-engcom-team magento-engcom-team added bug report Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed labels Sep 11, 2017
@magento-engcom-team magento-engcom-team added Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed 2.1.x Fixed in 2.3.x The issue has been fixed in 2.3 release line Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release labels Dec 4, 2017
@nmalevanec nmalevanec self-assigned this Dec 12, 2017
@ishakhsuvarov ishakhsuvarov added Fixed in 2.2.x The issue has been fixed in 2.2 release line and removed 2.1.x bug report labels Dec 12, 2017
@ishakhsuvarov
Copy link
Contributor

Hi @flancer64
Issue seems to be fixed in 2.2-develop now and is going to be available in 2.2.4.
You may check the merge commit 506d500 which contains the fix.

Thank you.

@flancer64
Copy link
Contributor Author

Thanks, great! :)

slavvka pushed a commit that referenced this issue May 29, 2020
[honey] MC-33700: TransportBuilder unable to send emails of Content-Type "text/plain"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed in 2.2.x The issue has been fixed in 2.2 release line Fixed in 2.3.x The issue has been fixed in 2.3 release line Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release
Projects
None yet
Development

No branches or pull requests

5 participants