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

Refactored product variant gateway and added handler to load stock of… #105

Merged
merged 2 commits into from
Sep 24, 2020

Conversation

rs2487
Copy link
Contributor

@rs2487 rs2487 commented Sep 24, 2020

… variant. Update Requirements and fixed codestyles and phpstan issues.

… variant. Update Requirements and fixed codestyles and phpstan issues.

public function getLocale(Request $request)
{
return $request->query->get('locale') ?? '';
Copy link
Contributor

Choose a reason for hiding this comment

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

'' is a strange fallback here. this could lead to confusing behaviour. i think i would expect an error here if no locale is given

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return $request->query->get('locale') ?? '';
$locale = $request->query->get('locale');
Assert::string($locale);
return $locale;

@@ -80,6 +80,6 @@ public function getSecurityContext()

public function getLocale(Request $request)
{
return $request->query->get('locale');
return $request->query->get('locale') ?? '';
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

@@ -29,8 +30,9 @@ protected function createProductInformation(string $productId, string $locale):
]
);

/** @var ProductInterface $product */
Copy link
Contributor

Choose a reason for hiding this comment

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

whats the reason for this? also i dont think that type is right, cant this be null too? 🤔

@@ -29,7 +29,7 @@ protected function createProduct(string $code): Product
return $product;
}

protected function findProduct(string $code): ?Product
protected function findProduct(string $code): ?object
Copy link
Contributor

Choose a reason for hiding this comment

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

whats the reason for this?

@@ -75,10 +78,13 @@ protected function findProductInformation(string $id, string $locale): ?ProductI
]
);

return $this->getEntityManager()->find(
/** @var ProductInformation $productInformation */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** @var ProductInformation $productInformation */
/** @var ProductInformation|null $productInformation */

@@ -95,13 +101,16 @@ protected function findProductInformationByCode(string $code, string $locale): ?
throw new \RuntimeException('Product not fount');
}

return $this->getEntityManager()->find(
/** @var ProductInformation $productInformation */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** @var ProductInformation $productInformation */
/** @var ProductInformation|null $productInformation */

@niklasnatter niklasnatter merged commit 0616ff7 into sulu:master Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants