Skip to content

Fix app/code/Magento/Backend/Block/Media/Uploader.php getConfigJson() method #11665

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.

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

Conversation

adrian-martinez-interactiv4
Copy link
Contributor

This method is using an undefined class property _coreData, that method cannot work, only crash if called, since it will try to call jsonEncode() method from a null property (even if defined dynamically).

Description

Adapted to use \Magento\Framework\Json\EncoderInterface instead of (probably) legacy code, the way it's done at \Magento\Downloadable\Block\Adminhtml\Catalog\Product\Edit\Tab\Downloadable\Links::getConfigJson.

Fixed Issues (if relevant)

It may be related with some open issue, but after searching, I didn't find one that may be caused by this method.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@@ -28,15 +30,24 @@ class Uploader extends \Magento\Backend\Block\Widget
protected $_fileSizeService;

/**
* @var \Magento\Framework\Json\EncoderInterface
*/
protected $_jsonEncoder;
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 _ and make it private

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the way it's done at \Magento\Downloadable\Block\Adminhtml\Catalog\Product\Edit\Tab\Downloadable\Links::getConfigJson, but I'll change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@okorshenko okorshenko self-assigned this Oct 24, 2017
@okorshenko okorshenko added this to the October 2017 milestone Oct 24, 2017
@adrian-martinez-interactiv4 adrian-martinez-interactiv4 force-pushed the FR#FIX-BLOCK-MEDIA-UPLOADER-GETCONFIGJSON branch from 724f747 to 86f3c39 Compare October 24, 2017 01:16
*/
public function __construct(
\Magento\Backend\Block\Template\Context $context,
\Magento\Framework\File\Size $fileSize,
array $data = []
array $data = [],
\Magento\Framework\Json\EncoderInterface $jsonEncoder = null
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 deprecated, please check https://github.com/magento/magento2/pull/10342/files#r129685891 for an up-to-date way to inject JSON encoder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know it and the reason why 👍

) {
$this->jsonEncoder = $jsonEncoder ?:
ObjectManager::getInstance()->get(\Magento\Framework\Json\EncoderInterface::class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Try using imports (Alt+Enter in PhpStorm) at least for such cases, probably it could fit in one line.

@adrian-martinez-interactiv4 adrian-martinez-interactiv4 force-pushed the FR#FIX-BLOCK-MEDIA-UPLOADER-GETCONFIGJSON branch from 86f3c39 to 2d6458a Compare October 24, 2017 12:23
@adrian-martinez-interactiv4
Copy link
Contributor Author

@orlangur Done.

okorshenko pushed a commit that referenced this pull request Oct 25, 2017
@okorshenko okorshenko merged commit 6f72198 into magento:2.3-develop Oct 25, 2017
okorshenko pushed a commit that referenced this pull request Oct 25, 2017
@adrian-martinez-interactiv4 adrian-martinez-interactiv4 deleted the FR#FIX-BLOCK-MEDIA-UPLOADER-GETCONFIGJSON branch October 27, 2017 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants