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

Update to api platform v2.6 #12510

Merged
merged 10 commits into from Apr 13, 2021
Merged

Conversation

Tomanhez
Copy link
Contributor

@Tomanhez Tomanhez commented Apr 6, 2021

Q A
Branch? master
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? no
License MIT

@Tomanhez Tomanhez added Maintenance CI configurations, READMEs, releases, etc. API APIs related issues and PRs. labels Apr 6, 2021
@Tomanhez Tomanhez requested a review from a team as a code owner April 6, 2021 18:31
@Tomanhez Tomanhez force-pushed the update-to-api-platform-v2.6 branch 4 times, most recently from 04602b3 to d37bdd3 Compare April 7, 2021 11:35
@Tomanhez Tomanhez force-pushed the update-to-api-platform-v2.6 branch 2 times, most recently from 1622445 to 708b568 Compare April 7, 2021 14:14
@Tomanhez Tomanhez force-pushed the update-to-api-platform-v2.6 branch from 708b568 to 312ffe0 Compare April 7, 2021 14:39
@Tomanhez Tomanhez force-pushed the update-to-api-platform-v2.6 branch from aca98d3 to 9b466a1 Compare April 8, 2021 08:30
UPGRADE-1.10.md Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Shop/AddressContext.php Outdated Show resolved Hide resolved
@Tomanhez Tomanhez force-pushed the update-to-api-platform-v2.6 branch from b66e752 to ba50004 Compare April 9, 2021 07:22
Copy link
Member

@GSadee GSadee left a comment

Choose a reason for hiding this comment

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

Could you check if overwriting ExtractorPropertyMetadataFactory service is still needed after upgrade?

composer.json Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Shop/AddressContext.php Outdated Show resolved Hide resolved
@@ -447,7 +447,7 @@ public function iShouldNotBeAbleToConfirmOrderBecauseDoNotBelongsToShippingCateg

$response = $this->ordersClient->getLastResponse();

Assert::same($response->getStatusCode(), 400);
Assert::same($response->getStatusCode(), 422);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if 422 status code is a proper one here, wdyt @lchrusciel ?

Copy link
Member

Choose a reason for hiding this comment

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

422 means that we are skipping validation and handling an exception. I would say, that it would be better to keep 400

@@ -72,8 +72,8 @@ private function isSameSubresource(array $context, array $currentContext): bool
$subresources = array_keys($context['subresource_resources']);
$currentSubresources = [];

foreach ($currentContext['identifiers'] as $identifierContext) {
$currentSubresources[] = $identifierContext[1];
foreach ($currentContext['identifiers'] as [$class]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

First time seeing this syntax, what does it do? 😨

Copy link
Contributor Author

@Tomanhez Tomanhez Apr 12, 2021

Choose a reason for hiding this comment

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

This solution is the same like new approach in RouteNameResolver in api-platform 2.6, If I good understand, this syntax means that if element of array is not array it is null, and you have directly access to content of element :
Screenshot 2021-04-12 at 10 04 46

@@ -413,7 +413,7 @@ public function iShouldBeNotifiedThatItHasBeenSuccessfullyChanged(): void
*/
public function iShouldBeNotifiedThatProvidedPasswordIsDifferentThanTheCurrentOne(): void
{
Assert::same($this->customerClient->getLastResponse()->getStatusCode(), 400);
Assert::same($this->customerClient->getLastResponse()->getStatusCode(), 422);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why some assertions are rewritten from expecting 400 to 422 and some other ones (like the one below in this file) are changed from expecting 400 to either 400 or 422?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pamil @lchrusciel @GSadee This code was 422 but was wrong showed in docs, this PR fixed it in api-platform 2.6: api-platform/core#4086

@Tomanhez Tomanhez force-pushed the update-to-api-platform-v2.6 branch from ba50004 to ee39c62 Compare April 12, 2021 07:26
@Tomanhez Tomanhez force-pushed the update-to-api-platform-v2.6 branch from 594cfad to 155dd7a Compare April 12, 2021 09:23
@GSadee GSadee merged commit 18b1ed4 into Sylius:master Apr 13, 2021
@GSadee
Copy link
Member

GSadee commented Apr 13, 2021

Thank you, Tomasz! 🎉

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. Maintenance CI configurations, READMEs, releases, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants