[Suggestion] Api\SearchResultsInterface and Data\SearchResultInterface needs improvement

[Vinai]

There are many classes and interfaces related to search results. Some of them seem to duplicate functionality or are in conflict with each other by exposing the same methods.
It seems as if even the core team is confused because sometimes one interface is implemented, but another's methods are also added, but partially only as method stubs.
Finally the similar names are extremely confusing and make it hard to reason about the code and see what it actually does.

These are the classes I'm talking about:

• \Magento\Framework\Data\AbstractSearchCriteriaBuilder
• \Magento\Framework\Data\AbstractSearchResult
• \Magento\Framework\Data\SearchResultInterface
• \Magento\Framework\Data\SearchResultIterator
• \Magento\Framework\Data\SearchResultIteratorFactory
• \Magento\Framework\Data\SearchResultProcessor
• \Magento\Framework\Data\SearchResultProcessorFactory
• \Magento\Framework\Data\SearchResultProcessorInterface
• \Magento\Framework\Api\CriteriaInterface
• \Magento\Framework\Api\SearchCriteria
• \Magento\Framework\Api\SearchCriteriaBuilder
• \Magento\Framework\Api\SearchResults
• \Magento\Framework\Api\SearchResultsInterface

(This list excludes many classes in the same namespaces which don't help, but I would like to keep this issue focused on the SearchResults, which to me are the most confusing)

I'm not sure how to best resolve this issue, but I think the following changes would help.

• Rename \Magento\Framework\Data\SearchResultInterface to \Magento\Framework\Data\CollectionResultInterface
• Rename \Magento\Framework\Data\SearchResultInterface::getSearchCriteria to \Magento\Framework\Data\SearchResultInterface::getCriteria() (or after the proposed rename: \Magento\Framework\Data\CollectionResultInterface::getCriteria(), as it returns a CriteriaInterfaceand not a SearchCriteriaInterface instance.
• Rename \Magento\Framework\Data\SearchResultIterator to \Magento\Framework\Data\CollectionResultIterator
• Rename \Magento\Framework\Data\SearchResultIteratorFactory to \Magento\Framework\Data\CollectionResultIteratorFactory
• Rename \Magento\Framework\Data\SearchResultProcessor to \Magento\Framework\Data\CollectionResultProcessor
• Rename \Magento\Framework\Data\SearchResultProcessorFactory to \Magento\Framework\Data\CollectionResultProcessorFactory
• Rename \Magento\Framework\Data\SearchResultProcessorInterface to \Magento\Framework\Data\CollectionResultProcessorInterface
• Rename \Magento\Framework\Data\AbstractSearchResult to \Magento\Framework\Data\AbstractCollectionResult
• Remove the method \Magento\Framework\Data\AbstractSearchResult::setSearchCriteria(), as this doesn't do anything, refers to Api\SearchCriteriaInterface in the method signature and thus adds to confusion between the two parts of the framework. It seems as if someone was attempting to implement both Data\SearchResultInterface and Api\SearchResultsInterface.

These changes would help developers like myself understand how the code is intended to be used. The reason I didn't go ahead with a PR directly is because I might be totally on the wrong track. Please let me know if you agree or disagree with my suggestion. If I'm off track please tell me how these classes and interfaces are supposed to be used. If you prefer a different naming scheme, I'd be happy to implement that, too. It would be great if these two parts of the framework where more easily distinguishable.

[Vinai]

Should I just go ahead with a PR for this as a base for discussion since the change will be impossible once M2 is released ;)
What do you think? A simple "lets see it" or "no thanks" would be enough feedback.

[joshdifabio]

Hopefully someone from Magento will respond, but regarding the proposed class names my personal opinion is that the name CollectionResult doesn't seem quite right. While in English terms it makes sense to say that a Search operation generally yields a Result, I'm not sure it makes as much sense to say that a Collection yields a Result; rather, a collection is the result of some operation(s). Maybe if Magento agree with renaming these classes (and interfaces) it would be better to replace SearchResult with Collection?

[Vinai]

Thanks for your response, good point. However, there already is a class \Magento\Framework\Data\Collection. The new class names needs to make the code more self-documenting and thus easier to understand. To a developer it should be clear when and how to use the interface or class just by looking at the name of the class, ideally even without taking the namespace into account.

Another suggestion: Instead of using CollectionResult, how about ResultCollection? To my foreign ears that sounds like better English. What do you think?

[kandy]

Maybe better to rename Class \Magento\Framework\Data\Collection to \Magento\Framework\Data\CollectionBuilder

[roshimon]

SearchCollection would be good too.

[Vinai]

@kandy The class \Magento\Framework\Data\Collection is not a builder, but rather the Magento 2 pendant of Varien_Data_Collection. It seems to be intended to be used as a base class for all other Collections in Magento 2. I personally think it would be best to declare it abstract, but that is a different issue.

@roshimon The class does not represent a collection of searches, so I don't think the name is fitting to be honest.

[roshimon]

Cool Vinai.

[kandy]

@Vinai Why not? We configure query and on getIterator/getData methods call we obtains collection of documents in same way as builder create object

[Vinai]

@kandy Afaik builder is used for the creation of a single aggregate objects. A collection is used to manage access to multiple objects of the same type. Instantiation is not necessarily part of a collection (see \Magento\Framework\Data\Collection::addItem()). On the other hand, by definition the builder pattern is.
I think much of the confusion comes from the Magento 1 legacy where collections where mixed with repository and even identity map concepts.

[joshdifabio]

However, there already is a class \Magento\Framework\Data\Collection.

Apologies, I should have looked at the code!

The class \Magento\Framework\Data\Collection is not a builder

I agree. Builder pattern is about explicit object creation, which is not what Collection does.

I think much of the confusion comes from the Magento 1 legacy where collections where mixed with repository and even identity map concepts.

I think this is the main issue; as in Magento 1, the Collection family of classes mixes several responsibilities and this is the root cause of the naming issues; it's always hard to name things which have multiple responsibilities. Furthermore, violating SRP like this has other, more damaging consequences. Are there any plans to change the design or even eliminate usage of the Magento 1-style Collection classes, or is that unrealistic?

[Vinai]

Please take what follows with a grain of salt, it's just what I think is the idea:
Collections as part of the Magento ORM (as in the model/resource model/collection triad) are now an implementation detail, that extension code ideally should no longer rely on.
Instead, a repository is used to load individual entities or a list of entities.
By convention (as it is not declared in any interface) all (?) repositories provide a getList() method.
This method takes a SearchCriteria instance which specifies which items will be included in the result list.
The result list is an instance of \Magento\Framework\Api\SearchResultsInterface.
The return type of the method SearchResultsInterface::getItems() is typehinted in the interface to be \Magento\Framework\Api\ExtensibleDataInterface[] (so no Collection :)).
The return type is usually typehinted to a more specific type interface extending the interface declared in the base interface.
From what @alankent posted in his blog, this would enable Magento to replace the ORM at one point in the future and all extensions that depend only on a repository would still continue to work.

[joshdifabio]

Thanks, @Vinai, that's a really helpful explanation. Overall it sounds like quite a pragmatic but a clever approach, but it's brilliant that you're investing the time to make it better.

If I'm able to find some time to get familiar with these classes then I might chime in again next week.

Keep it up, guys!

[daim2k5]

Hopefully @joshdifabio you got your answer if not please feel free to reopen the issue