-
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
Reduce requirements for parameter in catalog product type factory #26821
Reduce requirements for parameter in catalog product type factory #26821
Conversation
Hi @hws47a. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
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, @hws47a .
Thank you for your contribution.
Except the changes provided, I think it would be nice to execute some code cleanup - at least replace fully qualified class names with import, and probably add missing @throws
tags.
Hi @novikor, thanks for your review. My idea is opposite, I do small changes but which bother me. I believe in every single core class there are plenty of code which could be cleaned up, but it'll take ages if I'll start. So please accept the PR as it is if it passes checks. |
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.
@hws47a, As you wish
Hi @novikor, thank you for the review. |
✔️ QA Passed |
Failed functional tests not related to the changes in this PR. |
Hmm. There were no SVC failures at the moment of approval. |
@Nazar65, please do not add new commits to PRs which are already approved next time :) |
It seems that SVC false alarm happened. There are no methods return types changes by the fact. |
Thanks @novikor |
The ticket to SVC approval https://jira.corp.magento.com/browse/MC-31723 |
@@ -164,7 +161,7 @@ public function priceFactory($productType) | |||
* Get Product Price Info object | |||
* | |||
* @param Product $saleableItem | |||
* @return \Magento\Framework\Pricing\PriceInfoInterface |
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.
Changing of input/return annotations in docblocks should be reverted (for all methods except construct).
Theoretically, this class can be used in WebAPI requests.
The current implementation of WebAPI "request/response" parser has a limitation:
Valid object types include a fully qualified class name or a fully qualified interface name.
https://devdocs.magento.com/guides/v2.3/extension-dev-guide/service-contracts/service-to-web-service.html
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.
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 fix the PR according to @naydav comment
Pull Request state was updated. Re-review required.
@@ -116,8 +113,8 @@ public function __construct( | |||
/** | |||
* Factory to product singleton product type instances | |||
* | |||
* @param \Magento\Catalog\Model\Product $product | |||
* @return \Magento\Catalog\Model\Product\Type\AbstractType | |||
* @param ProductInterface $product |
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.
@slavvka is this type also might be reverted ? or we can leave Magento\Catalog\Api\Data\ProductInterface
?
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.
Use full qualified type for ProductInterface. Don't import it.
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.
ok, i mean that in this case return type was changed from \Magento\Catalog\Model\Product
to Magento\Catalog\Api\Data\ProductInterface
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.
Exactly, without it there's no reason for making this PR
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.
done ✔️
Hi @novikor could you please review this one again ? |
Hi @novikor, thank you for the review. |
✔️ QA Passed |
Hi @hws47a, thank you for your contribution! |
Description (*)
In the factory method, only API Interface method is used, but PHPDoc asks for the Product Model instead. Fix for this.
Contribution checklist (*)