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

[CoreBundle] Move serialization to AdminApiBundle and move ProductVar… #8115

Merged
merged 5 commits into from
Jun 5, 2017

Conversation

Niiko
Copy link

@Niiko Niiko commented May 29, 2017

…iant relations serialization from ProductBundle to AdminApiBundle

Q A
Bug fix? no
New feature? no
BC breaks? yes
Related tickets fixes #8087
License MIT

Move serialization from CoreBundle to AdminApiBundle
Move relations of ProductVariant serialization from ProductBundle to AdminApiBundle

I'm not sure to do it the right way @lchrusciel.

If i'm correct, every relations with admin_api routes have to be moved the same way no ?

…iant relations serialization from ProductBundle to AdminApiBundle
@lchrusciel lchrusciel added the API APIs related issues and PRs. label May 30, 2017
lchrusciel
lchrusciel previously approved these changes May 30, 2017
@lchrusciel
Copy link
Member

lchrusciel commented May 30, 2017

I don't have a better idea how to tackle it. Can you move relation from the rest of routes? Currently, they aren't useful anyway.

@lchrusciel lchrusciel added this to the v1.0.0 milestone May 30, 2017
@lchrusciel
Copy link
Member

We will need a note in UPGRADE file about it. And also, I'm pretty sure it is BC break, but we need it.

@lchrusciel lchrusciel added BC Break PRs introducing BC breaks (do not even try to merge). Bug Fix labels May 30, 2017
@lchrusciel lchrusciel dismissed their stale review May 30, 2017 06:27

I haven't seen lack of UPGRADE.md

@Niiko
Copy link
Author

Niiko commented May 30, 2017

Ok, i move a lot of relations, but on some, i have questions

  • Country
  • Province
  • Zone
  • Currency
  • ExchangeRate
  • Locale
  • ProductAssociationType
  • ProductAttribute
  • ProductOption
  • ShipmentUnit
  • ShippingCategory
  • TaxCategory

They are not in corebundle, do you want i create them ?

@Niiko Niiko closed this May 30, 2017
@Niiko Niiko reopened this May 30, 2017
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.

A few small tweaks + note in UPGRADE.md and it is ready to merge :)

@@ -24,4 +32,4 @@ Sylius\Component\Core\Model\OrderItem:
parameters:
code: expr(object.getVariant().getCode())
productCode: expr(object.getVariant().getProduct().getCode())
version: 1
version: 1
Copy link
Member

Choose a reason for hiding this comment

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

Missing blank line at the end of file

@@ -6,6 +6,14 @@ Sylius\Component\Core\Model\OrderItem:
expose: true
groups: [Default, Detailed, DetailedCart]
relations:
- rel: order
exclusion:
groups: [Default, Detailed, DetailedCart]
Copy link
Member

Choose a reason for hiding this comment

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

Something went wrong with the indentation

code: expr(object.getProduct().getCode())
version: 1
exclusion:
groups: [Detailed]
Copy link
Member

Choose a reason for hiding this comment

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

Missing blank line at the end of file

Copy link
Author

Choose a reason for hiding this comment

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

added

UPGRADE.md Outdated
@@ -156,20 +156,79 @@
* from `SyliusCoreBundle:Email:userRegistration.html.twig` to `SyliusShopBundle:Email:userRegistration.html.twig`
* from `SyliusCoreBundle:Email:passwordReset.html.twig` to `SyliusShopBundle:Email:passwordReset.html.twig`
* from `SyliusCoreBundle:Email:verification.html.twig` to `SyliusShopBundle:Email:verification.html.twig`


* The following serialization configuration was moved from CoreBundle/Resources/config/app/config.yml to AdminApiBundle/Resources/config/app/config.yml
Copy link
Member

Choose a reason for hiding this comment

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

It should b part of migration from FROM 1.0.0-beta.2 to 1.0.0 (new section)

Copy link
Author

Choose a reason for hiding this comment

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

my bad, read it too quickly

@lchrusciel lchrusciel requested a review from Arminek May 31, 2017 09:27
@pjedrzejewski pjedrzejewski merged commit c927cda into Sylius:master Jun 5, 2017
@pjedrzejewski
Copy link
Member

Thank you Nicolas & Łukasz!

@Niiko Niiko deleted the move-serialization branch June 5, 2017 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API APIs related issues and PRs. BC Break PRs introducing BC breaks (do not even try to merge).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Product][Serializer] ProductVariant and Taxon serializers has to be not dependent on SyliusAdminApiBundle
3 participants