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.

Sign up for GitHub

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

#7698 - modularize admin global search #9192

Closed
wants to merge 15 commits into from
Closed

#7698 - modularize admin global search #9192

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
36f3410
#7698 - modularize admin global search
strajeru Apr 10, 2017
3af1834
#7698 - modularize admin global search
strajeru Apr 10, 2017
cec4e90
fix failing test
strajeru Apr 12, 2017
ce0d756
fix typo
strajeru Apr 12, 2017
a0d3bc2
add interface for search items
strajeru Jul 11, 2017
5c5d676
Merge remote-tracking branch 'origin/develop' into M2-7698
strajeru Jul 11, 2017
3391728
Merge pull request #2 from magento/develop
tzyganu Jul 12, 2017
4a3090c
merge latest develop
strajeru Jul 12, 2017
ce31871
separated get results from search query
strajeru Jul 12, 2017
3e5a937
separated get results from search query
strajeru Jul 12, 2017
4dc0667
7698: Fix failing test
strajeru Jul 12, 2017
6f26c3e
7698: Fix failing static tests
strajeru Jul 12, 2017
820c116
7698: fix code style
strajeru Jul 13, 2017
f95d6e2
7698: fix code style
strajeru Jul 13, 2017
fa050cb
7698: fix code style
strajeru Jul 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions app/code/Magento/Backend/Api/Search/ItemsInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
<?php
/**
*
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
namespace Magento\Backend\Api\Search;

/**
* @api
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please provide description for this interface.
All interfaces marked as @ api should be accompanied with clear and precise description

*/
interface ItemsInterface
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest renaming this interface, as Items is too broad definition.
Also getResults method in ItemsInterface creates confusion, since there may not be results of items.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ishakhsuvarov do you have a suggestion for @tzyganu in this case? I could think that ResultsInterface and getResults would work well.

{
const LIMIT = 'limit';
const START = 'start';
const QUERY = 'query';

/**
* get the search result items
*
* @return array
*/
public function getResults();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This interface is pretty bad one because it has a temporal coupling
on the level of contract. Because contract implies an order in which these methods would be called.
Firstly initialize all setters, and then call getResuls()

BTW This interface looks like a good application of Builder pattern for me.
So, if we would rename it to SearchResultBuilder that will make more sense.


/**
* set offset
*
* @param int $start
* @return ItemsInterface
Copy link
Contributor

@maghamed maghamed Jul 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't recommend to use "fluent interfaces" and "method chaining", we support CQS where the command should return void instead of $this.
Returning $this from the setter method is allowed just for Builder implementation
(I see that we have this chaining in code before your changes, just think that if we started to improve the code quality, let's fix this out)
or let's use Builder pattern here

Thus, there are two options @tzyganu :

  1. Build/Fill kind of SearchCriterial and introduce an independent interface for processing this SearchCriteria - in this case, we will segregate responsibilities.
  2. Or use Builder pattern - all setters change the state, getResults() - build objects and reset the state

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maghamed I was trying the SearchCriteria approach, but hit a wall.
I was doing it like this.
have an ItemInterface with a single method getResults(\Magento\Framework\Api\SearchCriteria $searchCriteria) but I cannot add the filter in the search criteria for the query because the query filtering is dependent on the entity I'm filtering. So maybe I can do it via an additional parameter?. Have getResults look like this getResults($searchCriteria, $query) and the classes that implement this interface are responsible for handling the query.
I could also avoid changing the $searchCriteria parameter by using an additional searchCriteriaBuilder inside the implementations of getResults. (this is already like that for the customer search)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tzyganu I think the existing SearchCriteria we have in Framework \Magento\Framework\Api\SearchCriteria would not fit your needs. It's too heavyweight for your specific case.
When I said "SearchCriteria" I meant similar approach we use with SearchCriteria:

  1. To have a dedicated interface to describe your request (aka SearchCriteria). In your case that would be something like:
namespace Your\Custom\Api;

class SearchCriteria
{
      public function setStart($start);
      public function setQuery($query);
      public function setLimit($limit);
      public function getStart();
      public function getQuery();
      public function getLimit();
}
  1. To have Search interfaces which will process a search query based on the SearchCriteria given as Input parameter. Something like:
namespace Your\Custom\Api;

class QueryProcessor
{
      public function getResults(SearchCriteria $searchCriteria);  // or maybe Search(SearchCriteria $searchCriteria)
}

Doing so we make a responsibility segregation and don't have problems with managing and keeping the state in entity responsible for search request execution because all the parameters come as input from the outside each time.

*/
public function setStart($start);

/**
* set search query
*
* @param string $query
* @return ItemsInterface
*/
public function setQuery($query);

/**
* set limit
*
* @param int $limit
* @return ItemsInterface
*/
public function setLimit($limit);
}
115 changes: 90 additions & 25 deletions app/code/Magento/Backend/Controller/Adminhtml/Index/GlobalSearch.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@
*/
namespace Magento\Backend\Controller\Adminhtml\Index;

use Magento\Backend\Model\Search\ItemsFactory;

/**
* @api
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

everything marked as @ api should be accompanied with description

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current Controller became big enough and now contains Business logic inside.
It's better to introduce dedicated handler/model for this.

In M2 we are trying to make Controllers as light/ thin as possible

This Handler would be marked as @ api intead of current controller

*/
class GlobalSearch extends \Magento\Backend\Controller\Adminhtml\Index
{
/**
Expand All @@ -20,17 +25,34 @@ class GlobalSearch extends \Magento\Backend\Controller\Adminhtml\Index
*/
protected $_searchModules;

/**
* modules that support preview
Copy link
Contributor

@maghamed maghamed Jul 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw this controller should be marked with PHP DocBlock as @ api
because now it represents extension point for other search modules.

Btw it makes sense to use another extension point for these purposes

*
* @var array
*/
private $previewModules;

/**
* @var ItemsFactory
*/
private $itemsFactory;

/**
* @param \Magento\Backend\App\Action\Context $context
* @param \Magento\Framework\Controller\Result\JsonFactory $resultJsonFactory
* @param array $searchModules
* @param array $previewModules
*/
public function __construct(
\Magento\Backend\App\Action\Context $context,
ItemsFactory $itemsFactory,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New argument to constructor may only be added as the last on the list to preserve backwards compatibility.
Please refer to Backwards Compatible Development guide for more information

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By last, you mean last one that does not have a default value, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, @ishakhsuvarov means last in a list of all existing arguments (after all existing arguments).
And each new argument added should be optional (nullable)

\Magento\Framework\Controller\Result\JsonFactory $resultJsonFactory,
array $searchModules = []
array $searchModules = [],
array $previewModules = []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such changes to the class constructor violate backwards compatibility policy:

  • New dependencies must not be added to the middle, only as a last parameter.
  • All newly added arguments must be nullable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like

array $searchModules = [],
ItemFactory $itemFactory = null,
SearchCriteriaFactory $criteriaFactory = null,
array $previewModules = []

And then loading the optional constructor if null via $newDependency ?: \Magento\Framework\App\ObjectManager::getInstance()->get(\New\Dependency\Interface::class) would work well here I think.

) {
$this->itemsFactory = $itemsFactory;
$this->_searchModules = $searchModules;
$this->previewModules = $previewModules;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider extracting this logic to a dedicated model, since It does not look like it is in controllers responsibility.
Magento 2 is aiming to implement controllers as lightweight as possible, moving all possible parts outside.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ishakhsuvarov I agree whit this, but I don't see a solution to move the search logic to a separate class and keep backwards compatibility (as suggested in other comments) at the same time. Maybe I can move this to a separate class, and you release it when you break BC for something else anyway.

parent::__construct($context);
$this->resultJsonFactory = $resultJsonFactory;
}
Expand All @@ -52,7 +74,11 @@ public function execute()
'description' => __('You need more permissions to do this.'),
];
} else {
if (empty($this->_searchModules)) {
$previewItems = $this->getPreviewItems();
$searchItems = $this->getSearchItems();
$items = array_merge_recursive($items, $previewItems, $searchItems);

if (empty($items)) {
$items[] = [
'id' => 'error',
'type' => __('Error'),
Expand All @@ -61,34 +87,73 @@ public function execute()
'Please make sure that all global admin search modules are installed and activated.'
),
];
} else {
$start = $this->getRequest()->getParam('start', 1);
$limit = $this->getRequest()->getParam('limit', 10);
$query = $this->getRequest()->getParam('query', '');
foreach ($this->_searchModules as $searchConfig) {
if ($searchConfig['acl'] && !$this->_authorization->isAllowed($searchConfig['acl'])) {
continue;
}

$className = $searchConfig['class'];
if (empty($className)) {
continue;
}
$searchInstance = $this->_objectManager->create($className);
$results = $searchInstance->setStart(
$start
)->setLimit(
$limit
)->setQuery(
$query
)->load()->getResults();
$items = array_merge_recursive($items, $results);
}
}
}

/** @var \Magento\Framework\Controller\Result\Json $resultJson */
$resultJson = $this->resultJsonFactory->create();
return $resultJson->setData($items);
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please provide a clear description for this function

* retrieve links to certain entities in the global search
*
* @return array
*/
private function getPreviewItems()
{
$result = [];
$query = $this->getRequest()->getParam('query', '');
foreach ($this->previewModules as $previewConfig) {
if ($previewConfig['acl'] && !$this->_authorization->isAllowed($previewConfig['acl'])) {
continue;
}
if (!$previewConfig['url'] || !$previewConfig['text']) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not correct comparison
!$previewConfig['url'] || !$previewConfig['text']

use isset instead

continue;
}
$result[] = [
'url' => $this->getUrl($previewConfig['url']).'?search='.$query,
'name' => __($previewConfig['text'], $query)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't have translate="true" attribute in DI.xml , as we have for UI components configuration, thus translation would not work correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maghamed Any pointers on how to handle this so the translation will work and still be able to keep the texts in DI? O maybe move them somewhere else?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tzyganu I found a possibility to specify "string" type to be translatable in DI.xml
using the "translate" attribute
Here is the definition of this attribute for any xsi:type="string" attribute type in types.XSD schema which is used by ObjectManager

magento2/lib/internal/Magento/Framework/Data/etc/argument/types.xsd

Lines 31 to 33 in 99e85cb

<xs:extension base="argumentType">
<xs:attribute name="translate" use="optional" type="xs:boolean"/>
</xs:extension>

Here is how this attribute interpreted

magento2/lib/internal/Magento/Framework/Data/Argument/Interpreter/StringUtils.php

Lines 41 to 44 in 99e85cb

$needTranslation = isset($data['translate']) ? $this->booleanUtils->toBoolean($data['translate']) : false;
if ($needTranslation) {
$result = (string)new \Magento\Framework\Phrase($result);
}

But there is no any usage of this attribute in Magento.
And looks like all the tries to pass strings using DI.xml just don't translate at all

Like this one:

    <type name="Magento\Catalog\Model\Entity\Product\Attribute\Design\Options\Container">
        <arguments>
            <argument name="options" xsi:type="array">
                <item name="option1" xsi:type="array">
                    <item name="value" xsi:type="string">container1</item>
                    <item name="label" xsi:type="string">Product Info Column</item>
                </item>
                <item name="option2" xsi:type="array">
                    <item name="value" xsi:type="string">container2</item>
                    <item name="label" xsi:type="string">Block after Info Column</item>
                </item>
            </argument>
        </arguments>
    </type>

or this one:

    <type name="Magento\Customer\Ui\Component\MassAction\Group\Options">
        <arguments>
            <argument name="data" xsi:type="array">
                <item name="urlPath" xsi:type="string">customer/index/massAssignGroup</item>
                <item name="paramName" xsi:type="string">group</item>
                <item name="confirm" xsi:type="array">
                    <item name="title" xsi:type="string">Assign a Customer Group</item>
                    <item name="message" xsi:type="string">Are you sure to assign selected customers to new group?</item>
                </item>
            </argument>
        </arguments>
    </type>

];
}
return $result;
}

/**
* retrieve all entity items that should appear in global search
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please, start the sentences with the capital letter

*
* @return array
*/
private function getSearchItems()
{
$items = [];
$start = $this->getRequest()->getParam('start', 1);
$limit = $this->getRequest()->getParam('limit', 10);
$query = $this->getRequest()->getParam('query', '');
foreach ($this->_searchModules as $searchConfig) {
if ($searchConfig['acl'] && !$this->_authorization->isAllowed($searchConfig['acl'])) {
continue;
}

$className = $searchConfig['class'];
if (empty($className)) {
continue;
}
try {
$searchInstance = $this->itemsFactory->create($className);
$results = $searchInstance->setStart(
$start
)->setLimit(
$limit
)->setQuery(
$query
)->getResults();
$items = array_merge_recursive($items, $results);

} catch (\LogicException $exception) {
continue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exception swallowing is a bad practice.
at least we need to log critical message here.
But in general, LogicalException should not be just swallowed and silently continue script execution

if you don't know in this specific case how to handle Exception, maybe make sense not to catch it at all

}
}
return $items;
}
}
37 changes: 37 additions & 0 deletions app/code/Magento/Backend/Model/Search/ItemsAbstract.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
namespace Magento\Backend\Model\Search;

use Magento\Backend\Api\Search\ItemsInterface;
use Magento\Framework\DataObject;

abstract class ItemsAbstract extends DataObject implements ItemsInterface
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

introducing of abstract classes is a bad practice. In M2 we are trying to avoid using Inheritance Based API.

use composition instead, if your main reason for Inheritance usage to get rid of Copy-Paste

{
/**
* {@inheritdoc}
*/
public function setStart($start)
{
return $this->setData(self::START, (int)$start);
}

/**
* {@inheritdoc}
*/
public function setQuery($query)
{
return $this->setData(self::QUERY, $query);
}

/**
* {@inheritdoc}
*/
public function setLimit($limit)
{
return $this->setData(self::LIMIT, (int)$limit);
}

}
44 changes: 44 additions & 0 deletions app/code/Magento/Backend/Model/Search/ItemsFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/

namespace Magento\Backend\Model\Search;

use Magento\Backend\Api\Search\ItemsInterface;
use Magento\Framework\ObjectManagerInterface;

class ItemsFactory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider using ItemFactory name for this class to be consistent with other Factories.

{
/**
* @var \Magento\Framework\ObjectManagerInterface
*/
private $objectManager;

/**
* @param \Magento\Framework\ObjectManagerInterface $objectManager
*/
public function __construct(ObjectManagerInterface $objectManager)
{
$this->objectManager = $objectManager;
}

/**
* Create new search items provider instance
*
* @param $instanceName
* @param array $data
* @return ItemsInterface
*/
public function create($instanceName, array $data = [])
{
$object = $this->objectManager->create($instanceName, $data);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like it's better to use 'get' instead of 'create' as this should be stateless object

if ($object instanceof ItemsInterface) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's better to make a type check before the object instantiation.
Use string which represents class name and check whether it's an implementation of needed class

return $object;
}
throw new \LogicException(
"The class '{$instanceName}' does not implement ".ItemsInterface::class
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use spaces between concatenation
"" . ItemsInterface::class

);
}
}
14 changes: 0 additions & 14 deletions app/code/Magento/Backend/etc/di.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,20 +88,6 @@
<argument name="identifier" xsi:type="string">Magento_Backend::all</argument>
</arguments>
</type>
<type name="Magento\Backend\Controller\Adminhtml\Index\GlobalSearch">
<arguments>
<argument name="searchModules" xsi:type="array">
<item name="customers" xsi:type="array">
<item name="class" xsi:type="string">Magento\Backend\Model\Search\Customer</item>
<item name="acl" xsi:type="string">Magento_Customer::customer</item>
</item>
<item name="sales" xsi:type="array">
<item name="class" xsi:type="string">Magento\Backend\Model\Search\Order</item>
<item name="acl" xsi:type="string">Magento_Sales::sales</item>
</item>
</argument>
</arguments>
</type>
<virtualType name="Magento\Backend\Model\Auth\Session\Storage" type="Magento\Framework\Session\Storage">
<arguments>
<argument name="namespace" xsi:type="string">admin</argument>
Expand Down
14 changes: 1 addition & 13 deletions app/code/Magento/Backend/view/adminhtml/templates/system/search.phtml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,19 +27,7 @@
</form>
<script data-template="search-suggest" type="text/x-magento-template">
<ul class="search-global-menu">
<li class="item">
<a id="searchPreviewProducts" href="<?php /* @escapeNotVerified */ echo $block->getURL('catalog/product/index/'); ?>?search=<%- data.term%>" class="title">"<%- data.term%>" in Products</a>
</li>
<li class="item">
<a id="searchPreviewOrders" href="<?php /* @escapeNotVerified */ echo $block->getURL('sales/order/index/'); ?>?search=<%- data.term%>" class="title">"<%- data.term%>" in Orders</a>
</li>
<li class="item">
<a id="searchPreviewCustomers" href="<?php /* @escapeNotVerified */ echo $block->getURL('customer/index/index/'); ?>?search=<%- data.term%>" class="title">"<%- data.term%>" in Customers</a>
</li>
<li class="item">
<a id="searchPreviewPages" href="<?php /* @escapeNotVerified */ echo $block->getURL('cms/page/index/'); ?>?search=<%- data.term%>" class="title">"<%- data.term%>" in Pages</a>
</li>
<% if (data.items.length) { %>
<% if (data.items.length) { %>
<% _.each(data.items, function(value){ %>
<li class="item"
<%- data.optionData(value) %>
Expand Down
Loading