[New routing] Refactoring update product variant positions#18882
[New routing] Refactoring update product variant positions#18882loic425 wants to merge 1 commit intoSylius:new-routingfrom
Conversation
loic425
commented
Mar 3, 2026
| Q | A |
|---|---|
| Branch? | new-routing |
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Related tickets | partially #18212 |
| License | MIT |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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. Comment |
❗ Preview Environment deployment failed on BunnyshellSee: Environment Details | Pipeline Logs Check https://github.com/Sylius/Sylius/actions/runs/22622528366 for details. Available commands:
|
diimpp
left a comment
There was a problem hiding this comment.
Request changes is due missing not found check on product variants
| $csrfToken = new CsrfToken(self::CSRF_TOKEN_NAME, $data['_csrf_token'] ?? ''); | ||
| $this->validateCsrfProtection($csrfToken); |
There was a problem hiding this comment.
| $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)) { |
There was a problem hiding this comment.
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();
}| $productVariant = $this->productVariantRepository->findOneBy(['id' => $productVariantToUpdate['id']]); | ||
| $productVariant->setPosition((int) $productVariantToUpdate['position']); |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
Maybe worth adding comment, that in-loop flush is necessary due how gedmo/sortable works otherwise it doesn't look like good code.