[API] Pick up cart implementation #11524
Conversation
lchrusciel
commented
May 28, 2020
•
lchrusciel
commented
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Related tickets | Related to #11250 |
| License | MIT |
lchrusciel
added
Feature
src/Sylius/Bundle/ApiBundle/PropertyInfo/Extractor/EmptyPropertyListExtractor.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/ApiBundle/spec/PropertyInfo/Extractor/EmptyPropertyListExtractorSpec.php
Outdated
Show resolved
Hide resolved
| { | ||
| $response = $this->cartsClient->getLastResponse(); | ||
| Assert::true( | ||
| $this->responseChecker->isCreationSuccessful($response), |
There was a problem hiding this comment.
I'm wondering, cannot we really check is there nothing in the created cart? Of course, it needs to be created first, but the content is crucial for this step imo 🐴
There was a problem hiding this comment.
For now, I would like to leave it as it is due to time constraints
There was a problem hiding this comment.
👍 but still should be improved 🚀
|
|
||
| $this->orderManager->persist($cart); | ||
|
|
||
| return $cart; |
There was a problem hiding this comment.
How orthodox we want to be regarding command and handlers in API? 😄 I theory, the command handler should not return any value
There was a problem hiding this comment.
There is no other way ATM. At least without a custom data transformer at the end.
There was a problem hiding this comment.
So probably we should implement a custom data transformer as fast as possible. The limits our used tool should no be an excuse for the wrong CQS pattern usage 🖖
src/Sylius/Bundle/ApiBundle/PropertyInfo/Extractor/EmptyPropertyListExtractor.php
Show resolved
Hide resolved
| @@ -56,4 +56,6 @@ public function updateFiles(array $newFiles): void; | |||
| public function addSubResource(string $key, array $subResource): void; | |||
|
|
|||
| public function removeSubResource(string $subResource, string $id): void; | |||
|
|
|||
| public function authorize(string $token): void; | |||
There was a problem hiding this comment.
What is the reason for extracting this method?
There was a problem hiding this comment.
I need to make a request without authorization, therefore the previous create method was no-go for me. However, the rest of the methods still needs to authorize so IMHO it is better to provide it as a part of Public API. BTW, I would like completely decouple Request creation from APIClient and I see these changes as a first steps in this direction.
|
Thanks, Łukasz! 🥇 |
