-
Notifications
You must be signed in to change notification settings - Fork 248
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
Add Services for Default Stock manipulation #121
Conversation
*/ | ||
interface DefaultSourceRepositoryInterface | ||
{ | ||
const DEFAULT_SOURCE= 1; |
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.
currently, we have a requirement to provide a description to all the constants declared,
because constant introduces public contract the same as the public method.
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.
But in this case, I propose to remove this constant and leave an interface method
DefaultSourceResolverInterface::getId(): int
in this case, somebody could customize it. For example, change ID of the default source/stock
Having constant - there is no such possibility
* | ||
* @api | ||
*/ | ||
interface DefaultSourceRepositoryInterface |
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.
It's better not to call this service as Repository. We have a strict pattern for Repositories and which methods should be placed there.
Here you can read more about this rules - https://github.com/magento-engcom/msi/wiki/Service-Contracts#magento-repositories-basics
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.
In this case, I propose to rename service to
DefaultSourceResolverInterface::getId(): int
which will return Id of the default Source (in our case 1 )
* | ||
* @api | ||
*/ | ||
interface DefaultStockRepositoryInterface |
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.
Same as above
[LYNX] Jquery upgrade compatibility changes
Description
Fixed Issues (if relevant)
Contribution checklist