-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[ApiBundle][ProductVariant] Unification of channelCode in channelPricing #15705
[ApiBundle][ProductVariant] Unification of channelCode in channelPricing #15705
Conversation
Wojdylak
commented
Jan 5, 2024
Q | A |
---|---|
Branch? | 1.13 |
Bug fix? | no |
New feature? | yes |
BC breaks? | no |
Deprecations? | no |
Related tickets | N/A |
License | MIT |
Bunnyshell Preview Environment deletedAvailable commands:
|
...lius/Bundle/ApiBundle/Serializer/ProductVariantChannelPricingsChannelCodeKeyDenormalizer.php
Outdated
Show resolved
Hide resolved
@@ -35,7 +43,7 @@ | |||
</constraint> | |||
</property> | |||
<property name="channelCode"> | |||
<constraint name="NotBlank"> | |||
<constraint name="NotNull"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guessing we're missing a test what happens with a configuration like
['channelPricings' => ['' => []]];
just to be sure it won't blow up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is ExistingChannelCodeValidator
if ($value === null) {
return;
}
if ($this->channelRepository->findOneByCode($value) === null) {
$this->context->buildViolation($constraint->message)
->setParameter('{{ channelCode }}', $value)
->addViolation()
;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in case when it's an empty string we're unnecessarily calling the database.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, you're right. I'll revert to using NotBlank, and in the ExistingChannelCodeValidator, I'll add a return statement for empty value.
…ngsChannelCodeKeyDenormalizer class
@@ -36,7 +44,7 @@ | |||
</property> | |||
<property name="channelCode"> | |||
<constraint name="NotBlank"> | |||
<option name="message">sylius.channel_pricing.channel_code.not_null</option> | |||
<option name="message">sylius.channel_pricing.channel_code.not_blank</option> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be mentioned in UPGRADE? If someone overrode this particular translation, it won't work anymore for them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I see, there's no such translation key as not_null
.
Thank you, @Wojdylak! |