#7698 - modularize admin global search #9192
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,17 +20,27 @@ class GlobalSearch extends \Magento\Backend\Controller\Adminhtml\Index | |
| */ | ||
| protected $_searchModules; | ||
|
|
||
| /** | ||
| * modules that support preview | ||
| * | ||
| * @var array | ||
| */ | ||
| protected $previewModules; | ||
|
There was a problem hiding this comment. don't use protected for newly created properties |
||
|
|
||
| /** | ||
| * @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, | ||
| \Magento\Framework\Controller\Result\JsonFactory $resultJsonFactory, | ||
| array $searchModules = [], | ||
| array $previewModules = [] | ||
|
There was a problem hiding this comment. Such changes to the class constructor violate backwards compatibility policy:
There was a problem hiding this comment. Something like And then loading the optional constructor if null via |
||
| ) { | ||
| $this->_searchModules = $searchModules; | ||
| $this->previewModules = $previewModules; | ||
|
There was a problem hiding this comment. Please consider extracting this logic to a dedicated model, since It does not look like it is in controllers responsibility. There was a problem hiding this comment. @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; | ||
| } | ||
|
|
@@ -52,7 +62,11 @@ public function execute() | |
| 'description' => __('You need more permissions to do this.'), | ||
| ]; | ||
| } else { | ||
| $previewItems = $this->getPreviewItems(); | ||
| $searchItems = $this->getSearchItems(); | ||
| $items = array_merge_recursive($items, $previewItems, $searchItems); | ||
|
|
||
| if (empty($items)) { | ||
| $items[] = [ | ||
| 'id' => 'error', | ||
| 'type' => __('Error'), | ||
|
|
@@ -61,34 +75,64 @@ public function execute() | |
| 'Please make sure that all global admin search modules are installed and activated.' | ||
| ), | ||
| ]; | ||
| } | ||
| } | ||
|
|
||
| /** @var \Magento\Framework\Controller\Result\Json $resultJson */ | ||
| $resultJson = $this->resultJsonFactory->create(); | ||
| return $resultJson->setData($items); | ||
| } | ||
|
|
||
| /** | ||
|
There was a problem hiding this comment. please provide a clear description for this function |
||
| * @return array | ||
| */ | ||
| protected function getPreviewItems() | ||
|
There was a problem hiding this comment. let's give just "private" visibility for all the new functions |
||
| { | ||
| $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']) { | ||
|
There was a problem hiding this comment. not correct comparison use isset instead |
||
| continue; | ||
| } | ||
| $result[] = [ | ||
| 'url' => $this->getUrl($previewConfig['url']).'?search='.$query, | ||
| 'name' => __($previewConfig['text'], $query) | ||
|
There was a problem hiding this comment. we don't have There was a problem hiding this comment. @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? There was a problem hiding this comment. @tzyganu I found a possibility to specify "string" type to be translatable in DI.xml magento2/lib/internal/Magento/Framework/Data/etc/argument/types.xsd Lines 31 to 33 in 99e85cb Here is how this attribute interpreted But there is no any usage of this attribute in Magento. Like this one: or this one: |
||
| ]; | ||
| } | ||
| return $result; | ||
| } | ||
|
|
||
| /** | ||
| * @return array | ||
| */ | ||
| protected 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; | ||
| } | ||
| $searchInstance = $this->_objectManager->create($className); | ||
|
There was a problem hiding this comment. why do we use ObjectManager here? There was a problem hiding this comment. @maghamed I didn't add this by myself. The code was there I just moved it around. But I will try to find a way around it. There was a problem hiding this comment. Marius, you right this looks like legacy code, even taking into account that it was written quite recently. What we usually do for such cases is to introduce new Interface with the common contract, and Factory which is responsible for producing an instance of this interface and making sure that object which would be created is an instance of the interface mentioned above. Doing so we get:
There was a problem hiding this comment. @maghamed already on it. |
||
| $results = $searchInstance->setStart( | ||
| $start | ||
| )->setLimit( | ||
| $limit | ||
| )->setQuery( | ||
| $query | ||
| )->load()->getResults(); | ||
| $items = array_merge_recursive($items, $results); | ||
| } | ||
| return $items; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
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