Fix sort order in \Magento\Framework\Api\Search\SearchCriteriaBuilder

[smartexcan]

Description (*)

When using Magento\Framework\Api\SortOrderBuilder while loading a ui listing component (outside of the base DataProvider), trying to sort some columns might not work, depending on which column is being sorted.

If the column applies its sort order after the SortOrderBuilder is used, the sort will work.

The issue is in the \Magento\Framework\Api\Search\SearchCriteriaBuilder class.
Column ui components apply their sort order to the DataProvider, which adds it to the SearchCriteriaBuilder.

magento2/lib/internal/Magento/Framework/Api/Search/SearchCriteriaBuilder.php

Lines 84 to 89 in 41883fa

	public function addSortOrder($field, $direction)
	{
	$this->sortOrderBuilder->setDirection($direction)
	->setField($field);
	return $this;
	}

Because the SortOrder object is not created until the SearchCriteriaBuilder create() method is called, other code can use SortOrderBuilder and create an object from it, resulting in the fields from the Column no longer being present.

magento2/lib/internal/Magento/Framework/Api/Search/SearchCriteriaBuilder.php

Line 63 in 41883fa

$this->data[SearchCriteria::SORT_ORDERS] = [$this->sortOrderBuilder->create()];

Fixed Issues (if relevant)

1. Fixes Ui Listing sorting sometimes broken when SortOrderBuilder is used outside DataProvider #37731

Manual testing scenarios (*)

1. Create ui component listing
2. Add some columns (id, name, etc)
3. Add a Select column, which gets its options from a class that uses SortOrderBuilder.
4. Add another non-select column after
5. Apply sort to column before select column
6. Sort should be applied as expected.

Questions or comments

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
• All automated tests passed successfully (all builds are green)

[m2-assistant]

Hi @smartexcan. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.

Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s)
  For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests
13. Semantic Version Checker

You can find more information about the builds here
ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.

---

For more details, review the Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

[smartexcan]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[ihor-sviziev]

When you multiple times will call the addSortOrder method, it will replace sort orders. I think it's not the expected result.
In stead, I think we should add a new sort order to the sortOrders array.
What do you think?

Also, I think we should cover it with unit or integration test. Could you please do so?

[smartexcan]

I only had it replacing sort orders as that is what it was doing previously. If adding multiple sort orders is desired we can update it to do so.

As for tests, i will look into how to make those, havent done any before

[smartexcan]

I've changed the addSortOrder method to add multiple sort orders, and added a setSortOrders method to replace the sort orders in the builder, which is the same functionality as the regular SearchCriteriaBuilder.

[smartexcan]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[smartexcan]

I haven't written a unit test before, so not sure if this is correct, but here is a start of something.
The steps to verify its working properly are here, but not sure if the full implementation is correct for a test.

<?php
/**
 * Copyright © Magento, Inc. All rights reserved.
 * See COPYING.txt for license details.
 */
declare(strict_types=1);

namespace Magento\Framework\Api\Test\Unit\Search;

use Magento\Framework\Api\FilterBuilder;
use Magento\Framework\Api\ObjectFactory;
use Magento\Framework\Api\SortOrder;
use Magento\Framework\Api\SortOrderBuilder;
use Magento\Framework\Api\Search\FilterGroupBuilder;
use Magento\Framework\Api\Search\SearchCriteriaBuilder;
use PHPUnit\Framework\TestCase;
use PHPUnit\Framework\MockObject\MockObject;

class SearchCriteriaBuilderTest extends TestCase
{
    /**
     * @var MockObject|ObjectFactory
     */
    private $objectFactoryMock;

    /**
     * @var SearchCriteriaBuilder
     */
    private $searchCriteriaBuilder;

    /**
     * @var SortOrderBuilder
     */
    private $sortOrderBuilder;

    /**
     * @var string
     */
    private $sortField;

    /**
     * @var string
     */
    private $sortDirection;

    /**
     * Set up
     */
    protected function setUp(): void
    {
        $this->objectFactoryMock = $this->createMock(ObjectFactory::class);

        // SortOrderBuilder is a shared object
        $this->sortOrderBuilder = new SortOrderBuilder($this->objectMockFactory);

        $this->sortField = 'orderOneField';
        $this->sortDirection = SortOrder::SORT_ASC;

        $this->searchCriteriaBuilder = new SearchCriteriaBuilder(
            $this->objectFactoryMock,
            new FilterGroupBuilder(
                $this->objectFactoryMock,
                new FilterBuilder($this->objectFactoryMock)
            ),
            $this->sortOrderBuilder
        );

        $this->searchCriteriaBuilder->addSortOrder(
            $this->sortField,
            $this->sortDirection
        );

        $this->sortOrderBuilder->setField('orderTwoField')
            ->setDirection(SortOrder::SORT_DESC)
            ->create();
    }

    public function testCreate()
    {
        $searchCriteria = $this->searchCriteriaBuilder->create();
        $sortOrders = $searchCriteria->getSortOrders();

        $this->assertIsArray($sortOrders);
        $this->assertCount(1, $sortOrders);
        $this->assertEquals($sortOrders[0]->getField(), $this->sortField);
        $this->assertEquals($sortOrders[0]->getDirection(), $this->sortDirection);
    }
}

[smartexcan]

Also the semantic version checker test failed due to addition of setSortOrders method

[smartexcan]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[smartexcan]

B2B integration test is failing due to this change.

Before changes in the pr, Magento\Framework\Api\Search\SearchCriteriaBuilder used to always have one SortOrder when creating its object, even if addSortOrder() was not called.
That SortOrder had an empty field and direction.

Now the SearchCriteriaBuilder returns an empty array if addSortOrder() or setSortOrders() are not called, which is the expected behavior from its parent class.

Magento\PurchaseOrderRule\Model\Rule\DataProvider::getData()
This function currently expects there to be at least one sort order, even if none get added.
It uses current() on the result of getSortOrders() from the search criteria, which returns false if the array is empty.

An additional check should be added to that data provider to check if $sortOrder is false.

[smartexcan]

The original issue this PR was created to fix looks to be mitigated as of version 2.4.6 thanks to 967f817.

I'm going to leave this PR open though, since the original use of the SortOrderBuilder in the changed file is very strange and I don't like to recommend that it's used that way.