[New routing] Refactoring update product variant positions by loic425 · Pull Request #18882

[New routing] Refactoring update product variant positions by loic425 · Pull Request #18882 · Sylius/Sylius

Q	A
Branch?	new-routing
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets	partially #18212
License	MIT

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Choose a reason for hiding this comment

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

Request changes is due missing not found check on product variants

Comment on lines +43 to +44

		$csrfToken = new CsrfToken(self::CSRF_TOKEN_NAME, $data['_csrf_token'] ?? '');
		$this->validateCsrfProtection($csrfToken);

Choose a reason for hiding this comment

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

	$csrfToken = new CsrfToken(self::CSRF_TOKEN_NAME, $data['_csrf_token'] ?? '');
	$this->validateCsrfProtection($csrfToken);
	$this->validateCsrfProtection(
	new CsrfToken(self::CSRF_TOKEN_NAME, $data['_csrf_token'] ?? '')
	);


		$productVariantsToUpdate = $data['productVariants'] ?? [];

		if (in_array($request->getMethod(), ['POST', 'PUT', 'PATCH'], true)) {

Choose a reason for hiding this comment

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

Why this check is necessary? Cannot route have these definitions?

But if necessary, then would be better to avoid wrapping logic with if

        if (!in_array($request->getMethod(), ['POST', 'PUT', 'PATCH'], true)) {
            throw new BadRequestException();
        }

Comment on lines +58 to +59

		$productVariant = $this->productVariantRepository->findOneBy(['id' => $productVariantToUpdate['id']]);
		$productVariant->setPosition((int) $productVariantToUpdate['position']);

Choose a reason for hiding this comment

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

Missing check that productVariant is found with NotFoundException

		/** @var ProductVariantInterface $productVariant */
		$productVariant = $this->productVariantRepository->findOneBy(['id' => $productVariantToUpdate['id']]);
		$productVariant->setPosition((int) $productVariantToUpdate['position']);
		$this->manager->flush();

Choose a reason for hiding this comment

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

Maybe worth adding comment, that in-loop flush is necessary due how gedmo/sortable works otherwise it doesn't look like good code.