Skip to content
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

[RFC][WIP] Expose product option information in API #7547

Closed
wants to merge 2 commits into from

Conversation

alcaeus
Copy link
Contributor

@alcaeus alcaeus commented Feb 17, 2017

Q A
Bug fix? yes
New feature? yes
BC breaks? yes
License MIT

The serializer configuration for product options currently does not correctly expose values: options only expose their codes, no names, no values. The name property is no longer used, instead this should be a virtual getName. The same goes for option values, affecting the value attribute.

Unfortunately, exposing option values triggers an error when creating a new option. The ProductOptionValueType creates a new ProductOptionValue object through the forms empty_data closure. However, the resulting object has no locale information, so when the result of the create is serialized, the following error occurs:

No locale has been set and current locale is undefined.

Obviously, the forms should create new objects not through the default empty_data function, but rather through the respective resource factory. Since such a change affects all form classes and might be considered a BC break (it involves adding a required factory argument to AbstractResourceType::__construct), I wanted to get some input on the issue before going out and changing all forms and services. What do you think?

Copy link
Member

@lchrusciel lchrusciel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Andreas for contributing! we really appreciate it.

But this part of API will be also reworked #7482. At least presenting ProductOptions. Maybe you would like to base your work on that PR?

Exposing a whole translation array should resolve the problem, as it is a stateless approach. Probably, the exception is thrown, because we didn't have any locale defined at all.

Anyway, if you need some insight into project or any help, feel free to ask or join us at our Slack :)

values:
expose: true
virtual_properties:
getName:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of fetching the name with virtual property, we should expose all product option translations

virtual_properties:
getName:
serialized_name: name
getValue:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above. Both virtual properties should be removed and we should expose all translations

@alcaeus
Copy link
Contributor Author

alcaeus commented Feb 17, 2017

Thanks for the quick reply, Łukasz. I'll hold off until #7482 is merged and then rebase and continue working on this.

@alcaeus
Copy link
Contributor Author

alcaeus commented Mar 23, 2017

Superseded by #7811.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants