Skip to content

Remove zend json from json controller #10342

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

Merged
merged 6 commits into from
Jul 27, 2017
Merged

Remove zend json from json controller #10342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
baeb58c
Remove the usage of Zend_Json from the controller results json file.
dmanners Jul 17, 2017
db6f353
Update the return type for the Adminhtml tax rate controllers
dmanners Jul 19, 2017
4f78388
Add ignore for unused parameters on the setData method
dmanners Jul 25, 2017
7e46cea
Check to see if we are dealing with an object with a toJson method
dmanners Jul 25, 2017
ff0581b
Update Controller/Result/Json setData
dmanners Jul 26, 2017
41e118b
Remove comments in prepration for PR
dmanners Jul 26, 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
3 changes: 2 additions & 1 deletion app/code/Magento/Tax/Controller/Adminhtml/Rate/AjaxLoad.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ class AjaxLoad extends \Magento\Tax\Controller\Adminhtml\Rate
/**
* Json needed for the Ajax Edit Form
*
* @return void
* @return \Magento\Framework\Controller\Result\Json
* @throws \InvalidArgumentException
*/
public function execute()
{
Expand Down
1 change: 1 addition & 0 deletions app/code/Magento/Tax/Controller/Adminhtml/Rate/AjaxSave.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ class AjaxSave extends \Magento\Tax\Controller\Adminhtml\Rate
* Save Tax Rate via AJAX
*
* @return \Magento\Framework\Controller\Result\Json
* @throws \InvalidArgumentException
*/
public function execute()
{
Expand Down
55 changes: 47 additions & 8 deletions lib/internal/Magento/Framework/Controller/Result/Json.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,24 +29,63 @@ class Json extends AbstractResult
protected $json;

/**
* @param \Magento\Framework\Translate\InlineInterface $translateInline
* @var \Magento\Framework\Serialize\Serializer\Json
*/
public function __construct(InlineInterface $translateInline)
{
private $serializer;

/**
* @param InlineInterface $translateInline
* @param \Magento\Framework\Serialize\Serializer\Json|null $serializer
* @throws \RuntimeException
*/
public function __construct(
InlineInterface $translateInline,
\Magento\Framework\Serialize\Serializer\Json $serializer = null
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we rely on particular implementation and not interface by the way?
https://github.com/magento/magento2/blob/develop/app/etc/di.xml#L162

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#8331 (comment) was the original comment from the team on the matter,

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I expected something like this. Not sure why such interface was introduced at all if we keep creating json-only code.

Needs to be mentioned somewhere to avoid confusion like #10335 (comment)

) {
$this->translateInline = $translateInline;
$this->serializer = $serializer ?: \Magento\Framework\App\ObjectManager::getInstance()
->get(\Magento\Framework\Serialize\Serializer\Json::class);
}

/**
* Set json data
*
* @param mixed $data
* @param boolean $cycleCheck Optional; whether or not to check for object recursion; off by default
* @param array $options Additional options used during encoding
* @return $this
* @param array|string|\Magento\Framework\DataObject $data
* @param bool $cycleCheck
* @param array $options
* @return Json
* @throws \InvalidArgumentException
* @throws \Magento\Framework\Exception\LocalizedException
* @SuppressWarnings(PHPMD.UnusedFormalParameter)
* @deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree with deprecating this method. setData is for setting data which will be encoded and setJsonData is for setting raw data.

Two arguments which has no effect should be deprecated though.

Please change method description accordingly.

*/
public function setData($data, $cycleCheck = false, $options = [])
{
$this->json = \Zend_Json::encode($data, $cycleCheck, $options);
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is just setter, no need to call another setter, Magento is already slow enough.

if ($data instanceof \Magento\Framework\DataObject) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why need this explicitly?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is needed to maintain compatibility with the old implementation which took care of this inside.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see...

public static function encode($valueToEncode, $cycleCheck = false, $options = array())
    {
        if (is_object($valueToEncode)) {
            if (method_exists($valueToEncode, 'toJson')) {
                return $valueToEncode->toJson();
            } elseif (method_exists($valueToEncode, 'toArray')) {
                return self::encode($valueToEncode->toArray(), $cycleCheck, $options);
            }
        }

Why it is not maintained for any object then?

return $this->setArrayData($data->toArray());
}

if (is_array($data)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this if.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please clarify

Copy link
Contributor

Choose a reason for hiding this comment

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

setData should be as old one, thus setData([]) just encodes array with no necessity for additional logic.

return $this->setArrayData($data);
}

if (is_string($data)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Passing string parameter did encode $data before, please remove this if.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please clarify

Copy link
Contributor

Choose a reason for hiding this comment

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

setData('someString') was setting json_encode('someString') and not just 'someString'.

return $this->setJsonData($data);
}

throw new \Magento\Framework\Exception\LocalizedException(
new \Magento\Framework\Phrase('Invalid argument type')
);
}

/**
* @param array $data
* @return $this
* @throws \InvalidArgumentException
*/
public function setArrayData(array $data)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this method as it does not bring any value.

Copy link
Contributor

Choose a reason for hiding this comment

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

This method was introduced to deprecate and remove setData method, which has too much responsibilities.

Copy link
Contributor

@orlangur orlangur Aug 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This is what I strongly disagree with. Its responsibility is quite clear - encode value and set it. There is no need to think which value you're passing.

setData - pass value and it will be encoded
setJsonData - pass string and it will be set as is

I do agree that setData($object) could be discouraged and corresponding if could be marked as deprecated for further removal and setData method simplification. But introduction of setArrayData method looks like a mess to me.

{
$this->setJsonData($this->serializer->serialize($data));
return $this;
}

Expand Down