[API] Refactor ApiPlatformClient ver.1 #11210
Conversation
Zales0123
added
DX
src/Sylius/Behat/Context/Api/Admin/ManagingTaxCategoriesContext.php
Outdated
Show resolved
Hide resolved
|
|
||
| $this->request('GET', $path, ['HTTP_ACCEPT' => 'application/ld+json']); | ||
| } | ||
| $path = sprintf('/new-api/%s?%s', $this->resource, $query); |
There was a problem hiding this comment.
Could we include a dedicated method in Request object?
There was a problem hiding this comment.
Yes, I've mentioned it in the PR description 👍
| $this->content[$key][] = $subresource; | ||
| } | ||
|
|
||
| private function mergeArraysUniquely(array $firstArray, array $secondArray): array |
There was a problem hiding this comment.
What's the difference from array_merge_recursive? Would be cool to mention.
There was a problem hiding this comment.
It does not duplicate values for the same keys. But yes, it should be in the comment 👌
|
|
||
| public function updateRequestData(array $data): void; | ||
| public function show(string $id): void; |
There was a problem hiding this comment.
It'd be cool if it could return a response.
src/Sylius/Behat/Context/Api/Admin/ManagingCustomerGroupsContext.php
Outdated
Show resolved
Hide resolved
src/Sylius/Behat/Context/Api/Admin/ManagingCustomerGroupsContext.php
Outdated
Show resolved
Hide resolved
GSadee
force-pushed
the
refactor-api-client
branch
2 times, most recently
from
7a50318
to
91de8e7
Compare
src/Sylius/Behat/Context/Api/Admin/ManagingProductOptionsContext.php
Outdated
Show resolved
Hide resolved
| use Sylius\Behat\Client\ApiClientInterface; | ||
| use Webmozart\Assert\Assert; | ||
|
|
||
| final class NotificationContext implements Context |
There was a problem hiding this comment.
Maybe it wasn't a bad idea to have these checks in one place?
There was a problem hiding this comment.
I agree, but I have no idea, how it should be implemented since we have api client per resource 😞
There was a problem hiding this comment.
It wasn't, but we also would have to register this context with each api_platform_client, as there is a requirement to have the resource property set. In theory - it doesn't matter, as we only check the status. But we still need to get the response from the client :/ So in a result - it's worth to be refactored, but probably not now 🖖
GSadee
force-pushed
the
refactor-api-client
branch
from
91de8e7
to
709e9ac
Compare
GSadee
force-pushed
the
refactor-api-client
branch
from
709e9ac
to
351a34f
Compare
| */ | ||
| public function iShouldBeNotifiedThatItHasBeenSuccessfullyCreated(): void | ||
| { | ||
| Assert::true($this->responseChecker->isCreationSuccessful($this->client->getLastResponse())); |
There was a problem hiding this comment.
There are a few true/false assertion without any comment, which will end up with an awesome error: "Expected true, got false"
We should do something about it in the next iteration
|
Thanks, Mateusz! 🥇 |
|
And Grzegorz! 🐎 |
…Sadee) This PR was merged into the api branch. Discussion ---------- | Q | A | --------------- | ----- | Branch? | api | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Related tickets | fixes #11210 (comment) | License | MIT Commits ------- e360744 [API][Behat] Add comment to notification assertion
|
Part of #11250 |
Changes:
Requestobject, that knows about all technical details of the sent request (headers, method, URL, optionally content)ApiPlatformClientinto two services: one sending requests (still namedApiPlatformClient), the other for checking the response and getting its values (namedResponseChecker)ApiPlatformClientto not being forced to specify it on each requestKnown issues:
Requesttype)ApiSecurityServiceis still not refactored (the question is - should it)It should also allow us to operate on multiple responses easier 🖖
I'm open for the discussion regarding naming and proposed solutions - we should make this service as perfect as possible 💃