-
Notifications
You must be signed in to change notification settings - Fork 18
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
Expose product variants via export API #160
Conversation
/** | ||
* Product variant data-object interface. | ||
*/ | ||
interface VariantInterface |
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.
We agreed to do not use interfaces for DTO.
Also, you can generate DTO with this tool: https://github.com/magento/saas-export/pull/69/files#diff-cd518b16965245507c614234e1e945e1R1. (PR is still in progress due to related need to be fixed fist: https://github.com/magento/catalog-storefront/pull/159/files, but we can use it already)
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.
I will delete interface if we agreed to it. But don't see any need to use those tool in my case.
/** | ||
* @inheritDoc | ||
*/ | ||
public function getOptionValueIds(): array |
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.
DTO and schema does not match: https://github.com/magento/saas-export/pull/67/files#diff-f3c59568cea39e5d8221a349960c1da1R118. (it will lead to error when you will try to use Export API)
I propose to create correct et_shema fist, and then generate classes from schema, so we will have one source of true.
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.
schema doesn't exists yet. You are referencing on the PR that isn't under review yet.
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.
Shema should be created first as a source of true as was agreed with @akaplya
Any manual work is redundant since we have a tool to generate classes
please check comment in #160 (review)
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.
Manual work has already done. I don't see any needs to undo this work.
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.
You may see after connecting 2 parts and use them together to propagate data from one system to another.
if something you can automate, just do it, don't spend your time on routine work
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.
DTO and et_schema do not match https://github.com/magento/saas-export/pull/67/files#diff-f3c59568cea39e5d8221a349960c1da1R118. (it will lead to error when you will try to use Export API)
I propose to create correct et_shema fist, and then generate classes from schema, so we will have one source of true.
Please, work on #95 as well due to they are connected to each other (we can merge it in one task)
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.
Don't delete anything for now, bacause after deletion you will need to adapt classes.. which can be generated, so you will just spend time for nothing.
Lets move forward to another task in scope of this PR. And please, use branch-naming strategy
private const PRICE = 'price'; | ||
|
||
/** | ||
* @inheritDoc |
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.
Our static tests will possible failed for inheritDoc (with camel case)
$data | ||
); | ||
} | ||
} |
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.
Add empty line at the end of the file
public const PRODUCT_ENTITY = 'product'; | ||
public const PRODUCT_VARIANT_ENTITY = 'product_variant'; |
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 to add empty line between constants to match class codestile
*/ | ||
class IdentificatorSerializer implements SerializerInterface | ||
{ | ||
public function serialize(array $ids): string |
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 add description
return json_encode($ids); | ||
} | ||
|
||
public function deserialize(string $ids): array |
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 add description
Description (*)
Subtask of #27
app/code/Magento/CatalogExport/etc/webapi.xml
Related Pull Requests
Fixed Issues (if relevant)
Questions or comments
Contribution checklist (*)