-
Notifications
You must be signed in to change notification settings - Fork 8
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
fix/DEN-299 optionally allow keys for tax-categories #46
fix/DEN-299 optionally allow keys for tax-categories #46
Conversation
7ccff8b
to
644e007
Compare
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.
... And please add the key check to the existing unit test!
* @param mixed $changedValue | ||
* @param ClassMetadataInterface $metadata | ||
* @param array $changedData | ||
* @param array $oldData | ||
* @param mixed $sourceObject | ||
* | ||
* @return AbstractAction[] | ||
* @todo The documentation does not tell of the staged param in the action. what does that mean? |
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.
Please revert to default style guide.
*/ | ||
public function createUpdateActions( | ||
$changedValue, | ||
ClassMetadataInterface $metadata, | ||
array $changedData, | ||
array $oldData, | ||
$sourceObject | ||
): array { | ||
): array |
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.
What happened here?
$action = new ProductSetTaxCategoryAction(); | ||
|
||
if ($changedValue) { | ||
$action->setTaxCategory(TaxCategoryReference::ofId($changedValue['id'])); | ||
if (isset($changedValue['id']) && $changedValue['id'] !== null) { |
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.
Please remove the double null check.
$action->setTaxCategory(TaxCategoryReference::ofId($changedValue['id'])); | ||
} else { | ||
if (isset($changedValue['key']) && $changedValue['key'] !== null) { | ||
$action->setTaxCategory(TaxCategoryReference::ofKey($changedValue['key'])); |
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.
If the reference with keyw work now, then this is fine for me.
(and this is no fix, but a feature). |
Since I couldn't update the branch to add the tests, I've created a new PR (#48) and am closing this one. |
This fixes the problem that tax-categories are null (which is not allowed) by either using id's or keys (new), if there is no id given. This is necessary for DEN-299.
Lars Neujeffski