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
[BUG] Product options null value returns 400 instead of 500 #12501
Conversation
SirDomin
commented
Apr 1, 2021
Q | A |
---|---|
Branch? | 1.8 |
Bug fix? | yes |
New feature? | no |
BC breaks? | no |
Deprecations? | no |
Related tickets | |
License | MIT |
201c765
to
78c165d
Compare
@@ -45,7 +45,7 @@ public function addOptionValue(ProductOptionValueInterface $optionValue): void; | |||
|
|||
public function removeOptionValue(ProductOptionValueInterface $optionValue): void; | |||
|
|||
public function hasOptionValue(ProductOptionValueInterface $optionValue): bool; | |||
public function hasOptionValue(?ProductOptionValueInterface $optionValue): bool; |
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.
I wonder if this change is not a BC Break, although it expands the accepted argument, wdyt @lchrusciel
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.
🚨 That's a BC break, all classes implementing this interface have to change their method signature.
78c165d
to
2eedf53
Compare
2eedf53
to
e11571e
Compare
{ | ||
return $this->optionValues->contains($optionValue); | ||
return $optionValue === null ? true : $this->optionValues->contains($optionValue); |
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.
Why true instead of false?
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.
this is abit tricky, because by returning true we force it ProductVariant to dont allow null as argument and handle it as bad request.
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.
It is tricky because it feels wrong to check whether a product variant has an option value null
. I think we should fix it upstream and do not let null
to be passed to this method in the first place.
Thank you, @SirDomin! 🥇 |