-
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
Custom option type select - Allow modify list of single selection option types #21744
Custom option type select - Allow modify list of single selection option types #21744
Conversation
Hi @ihor-sviziev. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
42f73ee
to
e3eedf1
Compare
e3eedf1
to
8ea824e
Compare
Hi @rodrigowebjump, |
Hi @rodrigowebjump, thank you for the review. |
$this->singleSelectionTypes = $singleSelectionTypes ?: [ | ||
\Magento\Catalog\Api\Data\ProductCustomOptionInterface::OPTION_TYPE_DROP_DOWN, | ||
\Magento\Catalog\Api\Data\ProductCustomOptionInterface::OPTION_TYPE_RADIO, | ||
]; |
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.
@ihor-sviziev, could you please clarify, why we need default values here, cause we already have them in di.xml?
Also, here we have numeric array, and in di.xml we have associative array for default values.
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.
Hi @nmalevanec,
It's needed for backward compatiblity, when last argument will not be sent. For instance when someone extended this class in his code when this attribute was missing.
About numeric array VS associative array - ok, will unify them
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.
Hi @ihor-sviziev , consider defining default arguments in the di.xml, with this approach this parameter will become configurable instead of hardcoded.
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.
Hi @sidolov,
As we discussed in Slack - it's needed for backward compatibility, in case if someone will extend this class & will not pass last argument in parent::__construct(...)
- we have to have default value, similar as we're getting object from object manager by fallback.
…ions type Make associative array as a default value in constructor
$this->singleSelectionTypes = $singleSelectionTypes ?: [ | ||
\Magento\Catalog\Api\Data\ProductCustomOptionInterface::OPTION_TYPE_DROP_DOWN, | ||
\Magento\Catalog\Api\Data\ProductCustomOptionInterface::OPTION_TYPE_RADIO, | ||
]; |
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.
Hi @ihor-sviziev , consider defining default arguments in the di.xml, with this approach this parameter will become configurable instead of hardcoded.
Hi @sidolov, thank you for the review. |
Hi @ihor-sviziev, thank you for your contribution! |
…selection option types magento#21744
Description (*)
When I wanted to add one more type for "select" option group:
But there is no ability to mark own custom option type as "single select".
This PR adds ability to add own custom option type to this list by adding following code to di.xml file of own module:
This method is used in a lot of places, for example during validating custom option values during adding product to cart.
Fixed Issues (if relevant)
N/A
Manual testing scenarios (*)
N/A
Contribution checklist (*)