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 all 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
23 changes: 23 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,23 @@
<?php
/**
*
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
namespace Magento\Backend\Api\Search;

use Magento\Backend\Model\Search\SearchCriteria;

/**
* @api
*/
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.

{
/**
* Get the search result items
*
* @param SearchCriteria $searchCriteria
* @return array
*/
public function getResults(SearchCriteria $searchCriteria);
}
119 changes: 94 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,10 @@
*/
namespace Magento\Backend\Controller\Adminhtml\Index;

use Magento\Backend\Model\Search\ItemFactory;
use Magento\Backend\Model\Search\SearchCriteria;
use Magento\Backend\Model\Search\SearchCriteriaFactory;

/**
* @api
*/
Expand All @@ -24,16 +28,44 @@ 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 ItemFactory
*/
private $itemFactory;

/**
* @var SearchCriteriaFactory
*/
private $criteriaFactory;

/**
* Initialize dependencies
*
* @param \Magento\Backend\App\Action\Context $context
* @param \Magento\Framework\Controller\Result\JsonFactory $resultJsonFactory
* @param ItemFactory $itemFactory
* @param SearchCriteriaFactory $criteriaFactory
* @param array $searchModules
* @param array $previewModules
*/
public function __construct(
\Magento\Backend\App\Action\Context $context,
\Magento\Framework\Controller\Result\JsonFactory $resultJsonFactory,
array $searchModules = []
ItemFactory $itemFactory,
SearchCriteriaFactory $criteriaFactory,
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->itemFactory = $itemFactory;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please refer to the Adding a constructor parameter section of Backwards Compatible Development Guide for a definition and code example for adding constructor parameters.

$this->criteriaFactory = $criteriaFactory;
$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 @@ -55,7 +87,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 @@ -64,34 +100,67 @@ 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 (!isset($previewConfig['url']) || !isset($previewConfig['text'])) {
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
*
* @return array
*/
private function getSearchItems()
{
$items = [];
$start = $this->getRequest()->getParam('start', 1);
$limit = $this->getRequest()->getParam('limit', 10);
$query = $this->getRequest()->getParam('query', '');
/** @var SearchCriteria $searchCriteria */
$searchCriteria = $this->criteriaFactory->create();
$searchCriteria->setLimit($limit);
$searchCriteria->setStart($start);
$searchCriteria->setQuery($query);
foreach ($this->_searchModules as $searchConfig) {
if ($searchConfig['acl'] && !$this->_authorization->isAllowed($searchConfig['acl'])) {
continue;
}

$className = $searchConfig['class'];
if (empty($className)) {
continue;
}
$searchInstance = $this->itemFactory->create($className);
$results = $searchInstance->getResults($searchCriteria);
$items = array_merge_recursive($items, $results);
}
return $items;
}
}
45 changes: 45 additions & 0 deletions app/code/Magento/Backend/Model/Search/ItemFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<?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 ItemFactory
Copy link
Contributor

Choose a reason for hiding this comment

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

Brief description for this class may be added here

{
/**
* @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 string $instanceName
* @param array $data
* @return ItemsInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

@throws annotation may be added here as well

*/
public function create($instanceName, array $data = [])
{
$implements = class_implements($instanceName);
if (!isset($implements[ItemsInterface::class])) {
throw new \LogicException(
"The class '{$instanceName}' does not implement " . ItemsInterface::class
);
}
$object = $this->objectManager->get($instanceName, $data);
return $object;
}
}
76 changes: 76 additions & 0 deletions app/code/Magento/Backend/Model/Search/SearchCriteria.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/

namespace Magento\Backend\Model\Search;

class SearchCriteria
{
/**
* @var int
*/
private $limit;

/**
* @var int
*/
private $start;

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

/**
* @return int
*/
public function getLimit()
{
return $this->limit;
}

/**
* @param int $limit
* @return void
*/
public function setLimit($limit)
{
$this->limit = $limit;
}

/**
* @return int
*/
public function getStart()
{
return $this->start;
}

/**
* @param int $start
* @return void
*/
public function setStart($start)
{
$this->start = $start;
}

/**
* @return string
*/
public function getQuery()
{
return $this->query;
}

/**
* @param string $query
* @return void
*/
public function setQuery($query)
{
$this->query = $query;
}
}
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
20 changes: 4 additions & 16 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 @@ -17,29 +17,17 @@
class="search-global-input"
id="search-global"
name="query"
data-mage-init='<?= /* @noEscape */ $this->helper('Magento\Framework\Json\Helper\Data')->jsonEncode($block->getWidgetInitOptions()) ?>'>
data-mage-init='<?php /* @noEscape */ echo $this->helper('Magento\Framework\Json\Helper\Data')->jsonEncode($block->getWidgetInitOptions()) ?>'>
<button
type="submit"
class="search-global-action"
title="<?= /* @escapeNotVerified */ __('Search') ?>"
title="<?php /* @escapeNotVerified */ echo __('Search') ?>"
></button>
</div>
</form>
<script data-template="search-suggest" type="text/x-magento-template">
<ul class="search-global-menu">
<li class="item">
<a id="searchPreviewProducts" href="<?= /* @escapeNotVerified */ $block->getURL('catalog/product/index/') ?>?search=<%- data.term%>" class="title">"<%- data.term%>" in Products</a>
</li>
<li class="item">
<a id="searchPreviewOrders" href="<?= /* @escapeNotVerified */ $block->getURL('sales/order/index/') ?>?search=<%- data.term%>" class="title">"<%- data.term%>" in Orders</a>
</li>
<li class="item">
<a id="searchPreviewCustomers" href="<?= /* @escapeNotVerified */ $block->getURL('customer/index/index/') ?>?search=<%- data.term%>" class="title">"<%- data.term%>" in Customers</a>
</li>
<li class="item">
<a id="searchPreviewPages" href="<?= /* @escapeNotVerified */ $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 All @@ -52,7 +40,7 @@
<% } else { %>
<li>
<span class="mage-suggest-no-records">
<?= /* @escapeNotVerified */ __('No records found.') ?>
<?php /* @escapeNotVerified */ echo __('No records found.') ?>
</span>
</li>
<% } %>
Expand Down
Loading