Update the AdminNotification AjaxMarkAsRead controller #10433
Conversation
- stop the direct usage of object manager in the execute method, - modify the execute method to use the resultFactory for building the response
| ); | ||
|
|
||
| /** @var \Magento\Framework\Controller\Result\Json $resultJson */ | ||
| $resultJson = $this->resultFactory->create(ResultFactory::TYPE_JSON); |
There was a problem hiding this comment.
$this->getResponse()->representJson() vs $this->resultFactory->create(ResultFactory::TYPE_JSON) which is the option that should be considered for usage?
There was a problem hiding this comment.
I think the second option is more up to date.
| * Mark notification as read (AJAX action) | ||
| * | ||
| * @return void | ||
| * @return \Magento\Framework\Controller\Result\Json|void |
There was a problem hiding this comment.
Are mixed returns of \Magento\Framework\Controller\Result\Json|void ok or should it always be at least of type \Magento\Framework\App\ResponseInterface\ResultInterface
There was a problem hiding this comment.
I think it should follow the \Magento\Framework\App\ActionInterface interface signature, since it implements it.
|
|
||
| /** @var \Magento\Framework\Controller\Result\Json $resultJson */ | ||
| $resultJson = $this->resultFactory->create(ResultFactory::TYPE_JSON); | ||
| $resultJson->setData($responseData); |
There was a problem hiding this comment.
Calling setData on array of response data or calling setJsonData or already formatted data what is "best practice" or does it not matter?
There was a problem hiding this comment.
Taking into account the conversation happening in #10342 I can not say which one is the best practice atm. I assume you should use the one which is currently not deprecated.
There was a problem hiding this comment.
setData is an obvious leader as you don't want encodeJson in every controller which would also require additional JSON serializer dependency.
There was a problem hiding this comment.
@ishakhsuvarov both are not deprecated ATM due to revert :)
…into update-admin-ajax-controller
|
@magento-team Will this be backported to 2.2? |
|
@Ctucker9233 no, it cannot
|
Update the AdminNotification AjaxMarkAsRead controller
Description
This PR is really more a question with regards to best practice when building AjaxControllers.
Questions:
$this->getResponse()->representJson()vs$this->resultFactory->create(ResultFactory::TYPE_JSON)which is the option that should be considered for usage?\Magento\Framework\Controller\Result\Json|voidok or should it always be at least of type\Magento\Framework\App\ResponseInterface\ResultInterfacesetDataon array of response data or callingsetJsonDataor already formatted data?Contribution checklist