SearchCriteriaBuilder or SearchCriteriaBuilderFactory?

Essentials of SearchCriteriaBuilder

Knowing about the Magento\Framework\Api\SearchCriteriaBuilder class is essential for understanding this blog. Once you inject a repository - say, the product reposistory - in your own class to retrieve a listing of products, you will need to pass an instance of Magento\Framework\Api\SearchCriteria to its getList() method. And this instance is created through a builder:

$searchCriteria = $searchCriteriaBuilder->create();
$searchResult = $productRepository->getList($searchCriteria);

In this case, we're just retrieving all products. However, the builder supports various methods through which you can finetine your search:

$searchCriteriaBuilder->addFilter('status', 1);
$searchCriteriaBuilder->addFilter('name', 'foobar');
$searchCriteriaBuilder->setPageSize(1);
$searchCriteriaBuilder->setCurrentPage(1);
$searchCriteria = $searchCriteriaBuilder->create();

But note that the criteria are first set in the builder, and then passed on to the created criteria. In other words, the builder has a state, which is then used to create the other instance.

Singletons, shared instances, dependency injection

Here comes my point: Every dependency you inject through constructor-based DI in Magento is injected - by default - as a shared instance. They are singletons. Practically - when zooming in on the SearchCriteriaBuilder - when the SearchCriteriaBuilder is injected into class A and class B at the same time, it is the same SearchCriteriaBuilder instance. And the problem here is that if class A sets a filter, then that filter is also set in class B.

Luckily enough, our coding sample above ends with the create() method: By calling upon the create(), the state of SearchCriteriaBuilder is cleaned up. But what if some code forgets to clean up?

The culprit example?

Let's say that class ExampleA and class ExampleB are both making use of the SearchCriteriaBuilder. And let's say that class ExampleA is loaded before ExampleB. If both classes are properly making use of the SearchCriteriaBuilder, they should call in the end upon the create() method. But what if ExampleA does not?

class ExampleA
{
    private SearchCriteriaBuilder $searchCriteriaBuilder;

    public function __construct(
        SearchCriteriaBuilder $searchCriteriaBuilder
    ) {
        $this->searchCriteriaBuilder = $searchCriteriaBuilder;
    }

    public function executeDirective66()
    {
        $this->searchCriteriaBuilder->addFilter('name', 'foobar');
    }
}

Then actually, you would expect the builder is already initialized with that same filter, when loading it in the other class ExampleB:

class ExampleB
{
    private SearchCriteriaBuilder $searchCriteriaBuilder;

    public function __construct(
        SearchCriteriaBuilder $searchCriteriaBuilder
    ) {
        $this->searchCriteriaBuilder = $searchCriteriaBuilder;
    }

    public function getSearchCriteria()
    {
        return $this->searchCriteriaBuilder->create(); // filter is set
    }
}

Integration test to proof the point

To proof this, I can also build an integration test, which actually fails: I might be expecting that with the second call on $searchCriteriaBuilder->getData(), I get an empty result. But the previous filters are still there, so the test fails:

namespace Yireo\Example\Test\Integration;

use Magento\Framework\Api\SearchCriteriaBuilder;
use Magento\TestFramework\Helper\Bootstrap;
use PHPUnit\Framework\TestCase;

class SearchCriteriaBuilderTest extends TestCase
{
    public function testIfSearchCriteriaBuilderNeedsFactory()
    {
        $objectManager = Bootstrap::getObjectManager();
        $searchCriteriaBuilder = $objectManager->get(SearchCriteriaBuilder::class);
        $searchCriteriaBuilder->addFilter('status', 1);
        $searchCriteriaBuilder->addFilter('name', 'foobar');
        $searchCriteriaBuilder->setPageSize(1);
        $searchCriteriaBuilder->setCurrentPage(1);
        $data = $searchCriteriaBuilder->getData();
        $this->assertNotEmpty($data);

        $searchCriteriaBuilder = $objectManager->get(SearchCriteriaBuilder::class);
        $data = $searchCriteriaBuilder->getData();
        $this->assertEmpty($data);
    }
}

Reason is that I'm calling upon $objectManager->get() to return a singleton, instead of calling upon $objectManager->create() to return a fresh instance. (But bare with me, we're not there yet to make an end-conclusion.)

Don't use SearchCriteriaBuilder as a singleton

The key point here is that you don't know if your own copy of SearchCriteriaBuilder is clean or not - because Magento it's singleton behaviour. Don't blame Magento, blame your own code: If you expect a clean instance, you should ask for it. One way to do this, is to inject a factory for the SearchCriteriaBuilder:

class ExampleB
{
    private SearchCriteriaBuilder $searchCriteriaBuilder;

    public function __construct(
        SearchCriteriaBuilderFactory $searchCriteriaBuilderFactory
    ) {
        $this->searchCriteriaBuilder = $searchCriteriaBuilderFactory->create();
    }

    public function getSearchCriteria()
    {
        return $this->searchCriteriaBuilder->create();
    }
}

Another way is to set the SearchCriteriaBuilder to never be a singleton by creating a DI type definition with the shared flag disabled (which is enabled by default).

But wait, do we actually need to do this ourselves, when everyone will suffer from the same type of problem here? Well, actually, no. The core app/etc/di.xml instance contains the following:

<type name="Magento\Framework\Api\SearchCriteriaBuilder" shared="false" />

But the integration test failed?

Hey, but why did the integration test fail then? The same DI type definition is there in the integration testing sandbox folder as well, so you would expect that calling upon the SearchCriteriaBuilder twice would give you different instances.

I didn't really get to the bottom of this, but I do know that the object manager is called upon differently while running integration tests then with the normal app. Even more, the integration tests make use of a different object manager (Magento\TestFramework\ObjectManager) that has specific calls upon shared instances, which makes me always say that the default DI configuration is (partially) wiped out.

So, I was just a (...) to try to proof my point with an integration test.

Conclusion

Should you just inject SearchCriteriaBuilder or should you inject a SearchCriteriaBuilderFactory instead? Answer: Injecting the SearchCriteriaBuilder is just fine, because the default DI configuration of Magento prevents it from being a singleton.