-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Allow injection of Magento\Catalog\Model\View\Asset\ImageFactory #9670
Allow injection of Magento\Catalog\Model\View\Asset\ImageFactory #9670
Conversation
No need for an underscore for member names, yeah? |
I followed the conventions in the rest of the class... |
I know, but some of it is legacy/wrong. M2 uses PSR-2 for its code style, and that suggests not to use underscores. E.g. #3401 (comment) |
@@ -173,7 +173,7 @@ class Image extends \Magento\Framework\Model\AbstractModel | |||
/** | |||
* @var \Magento\Catalog\Model\View\Asset\ImageFactory | |||
*/ | |||
private $viewAssetImageFactory; | |||
protected $_viewAssetImageFactory; |
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.
please, revert this change. The property must be private
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.
Please read #9503 (comment) more carefully.
So, even if you preferred inheritance for your customization and extended this class, you don't need to change property visibility to protected.
You may simply specify another class for this argument in di.xml
for your new class.
\Magento\Catalog\Model\View\Asset\ImageFactory::class | ||
); | ||
} else { | ||
$this->_viewAssetImageFactory = $viewAssetImageFactory; |
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.
Please check this out: https://github.com/magento/magento2/blob/develop/app/code/Magento/Backend/Block/System/Account/Edit/Form.php#L60
- new dependency shall be added as last one
- there is no need in
if
statement - just make a one-liner - always use imported class name like
OptionInterface::class
I have made changes according to the requests by @okorshenko and @orlangur. Please let me know if this is sufficient to be considered for merging. Thanks! |
@@ -173,7 +173,7 @@ class Image extends \Magento\Framework\Model\AbstractModel | |||
/** | |||
* @var \Magento\Catalog\Model\View\Asset\ImageFactory | |||
*/ | |||
private $viewAssetImageFactory; | |||
protected $_viewAssetImageFactory; |
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.
Could you revert this change? Variable names should not be prefixed with underscore according to PSR-2. Also protected variables should be replaced with private for all cases where it's possible
@@ -226,6 +228,13 @@ public function __construct( | |||
$this->_assetRepo = $assetRepo; | |||
$this->_viewFileSystem = $viewFileSystem; | |||
$this->_scopeConfig = $scopeConfig; | |||
if ($viewAssetImageFactory == null) { |
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.
Would be great to replace this if block with ternary operators
@rolftimmermans thank you for your contribution to Magento 2 project |
It is not really possible to override the behaviour of Magento\Catalog\Model\View\Asset\ImageFactory.
Based on the suggestions given in #9503 (comment) this implementation aims to fix that by allowing injection of a factory via the constructor.