[API] Product options - update vol.1 #11144
Conversation
| */ | ||
| public function thisProductOptionNameShouldBe(ProductOptionInterface $productOption, string $name): void | ||
| { | ||
| $this->client->show('product_options', $productOption->getCode()); |
There was a problem hiding this comment.
| $this->client->show('product_options', $productOption->getCode()); | |
| $this->client->show('product_options', $productOption->getId()); |
?
There was a problem hiding this comment.
We defined the code as identifier of currency (check here), so I've done the same with product options. It's another decision we need to take. We use codes in Sylius (ui routes), but, on the other hand, it's not reliable as it's always just and autogenerated integer 鈿栵笍 I'm for using codes, but cc @pamil @lchrusciel
There was a problem hiding this comment.
okey I was confused because in apiClient we have public function buildUpdateRequest(string $resource, string $id): void so in this functions we should have $code instead $id if i understand it correctly 馃拑
There was a problem hiding this comment.
To be honest... I'm not sure 馃槃 For us, in Sylius, it's a code indeed, but for ApiPlatform it's an identifier (even if you take a look at the URL there is {id}, not {code}). So it still makes sense to me to keep it as it is 馃殌 But I'm not forcing it if it's confusing for some more people as well 馃枛
There was a problem hiding this comment.
Maybe we should use an IRI as the unique identifier in API Platform (ApiPlatform\Core\Api\IriConverterInterface)?
src/Sylius/Bundle/ApiBundle/Resources/config/api_resources/ProductOptionTranslation.xml
Outdated
Show resolved
Hide resolved
Zales0123
force-pushed
the
product-options-api-update
branch
from
45efa05
to
5229ec4
Compare
| @@ -35,6 +35,11 @@ public function index(string $resource): void | |||
| $this->client->request('GET', '/new-api/'.$resource, [], [], ['HTTP_ACCEPT' => 'application/ld+json']); | |||
| } | |||
|
|
|||
| public function show(string $resource, string $id): void | |||
| { | |||
| $this->client->request('GET', sprintf('/new-api/%s/%s', $resource, $id), [], [], ['HTTP_ACCEPT' => 'application/ld+json']); | |||
There was a problem hiding this comment.
Maybe we can use ApiPlatform\Core\Api\IriConverterInterface to generate an iri in this client?
There was a problem hiding this comment.
I agree 馃憤 However, do you think it can be a part of next PR, that would refactor ApiPlatformClient a little bit? It's needed, nevertheless
src/Sylius/Behat/Context/Api/Admin/ManagingProductOptionsContext.php
Outdated
Show resolved
Hide resolved
| */ | ||
| public function thisProductOptionNameShouldBe(ProductOptionInterface $productOption, string $name): void | ||
| { | ||
| $this->client->show('product_options', $productOption->getCode()); |
There was a problem hiding this comment.
Maybe we should use an IRI as the unique identifier in API Platform (ApiPlatform\Core\Api\IriConverterInterface)?
Zales0123
changed the title
features/product/managing_product_options/editing_product_option.feature
Show resolved
Hide resolved
Zales0123
force-pushed
the
product-options-api-update
branch
from
5229ec4
to
38a52e8
Compare
Zales0123
changed the title
Zales0123
force-pushed
the
product-options-api-update
branch
from
38a52e8
to
5c8c88f
Compare
src/Sylius/Behat/Context/Api/Admin/ManagingProductOptionsContext.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/ApiBundle/Resources/config/serializer/ProductOptionTranslation.xml
Outdated
Show resolved
Hide resolved
|
Thanks, Mateusz! 馃 |
This PR was merged into the api branch. Discussion ---------- | Q | A | --------------- | ----- | Branch? | api | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Related tickets | continuation of #11144 | License | MIT - production option name validation - default sorting of product options Commits ------- cb2dd4e [Behat][Api] Finish tests for product option validation d1ca81c [Behat][Api] Default sorting for product options
|
Part of #11250 |
TO-DO:
handle code immutability properly - even though serialization groups are defined properly, so code change is impossible, we should still get a validation error instead of just not having a code changed 馃拑 - see this line(EDIT: we decided that the current behavior is good, at least for now 馃悗)ApiPlatformClientto not make it too big 馃槃 (but I suppose it would be better to do it in a separate PR)