Describe your bug or feature request.
One can't add a tax adjustment with a percentage property set to 0 (zero).
From Drupal\commerce_order\Adjustment:
/**
* Constructs a new Adjustment object.
*
* @param array $definition
* The definition.
*/
public function __construct(array $definition) {
foreach (['type', 'label', 'amount'] as $required_property) {
if (empty($definition[$required_property])) {
throw new \InvalidArgumentException(sprintf('Missing required property %s.', $required_property));
}
}
if (!$definition['amount'] instanceof Price) {
throw new \InvalidArgumentException(sprintf('Property "amount" should be an instance of %s.', Price::class));
}
$adjustment_type_manager = \Drupal::service('plugin.manager.commerce_adjustment_type');
$types = $adjustment_type_manager->getDefinitions();
if (empty($types[$definition['type']])) {
throw new \InvalidArgumentException(sprintf('%s is an invalid adjustment type.', $definition['type']));
}
if (!empty($definition['percentage'])) {
if (is_float($definition['percentage'])) {
throw new \InvalidArgumentException(sprintf('The provided percentage "%s" must be a string, not a float.', $definition['percentage']));
}
if (!is_numeric($definition['percentage'])) {
throw new \InvalidArgumentException(sprintf('The provided percentage "%s" is not a numeric value.', $definition['percentage']));
}
}
// Assume that 'custom' adjustments are always locked, for BC reasons.
if ($definition['type'] == 'custom' && !isset($definition['locked'])) {
$definition['locked'] = TRUE;
}
$this->type = $definition['type'];
$this->label = (string) $definition['label'];
$this->amount = $definition['amount'];
$this->percentage = !empty($definition['percentage']) ? $definition['percentage'] : NULL;
$this->sourceId = !empty($definition['source_id']) ? $definition['source_id'] : NULL;
$this->included = !empty($definition['included']);
$this->locked = !empty($definition['locked']);
}
Given $definition['percentage'] == 0
this is how it's gonna be evaluated:
$this->percentage = !empty($definition['percentage']) ? $definition['percentage'] : NULL;
$this->percentage = !empty(0) ? 0 : NULL;
$this->percentage = NULL;
If a bug, provide steps to reproduce it from a clean install.
- Create Norwegian Tax rate using norwegian_vat tax plugin
- Install https://www.drupal.org/project/commerce_product_tax to easily assign a tax rate to the product
- Create 2 products
- Assign a 25% rate to product A
- Add product A to cart
- Check (in the database) that the adjustment's percentage is set to 25
- Assign 0% rate to product B
- Add product B to cart
- Check (in the database) that the adjustment's percentage is set to null rather than 0%
The example above is using tax adjustments but all adjustment types are applicable.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | 3187199-9.patch | 1.92 KB | zaporylie |
| #9 | 3187199-9-TEST-ONLY.patch | 710 bytes | zaporylie |
| #6 | 3187199-6.patch | 1.88 KB | zaporylie |
| #6 | 3187199-6-TEST-ONLY.patch | 662 bytes | zaporylie |
Comments
Comment #2
jsacksick commentedso I guess we should just replace the
!empty()byisset().Comment #3
zaporylieThat was my impression as well but wanted to check first (with git blame) whether there was a reason for this.
Comment #4
zaporylieThe current code originates in #2894792: Add Adjustment::getPercentage() (https://git.drupalcode.org/project/commerce/-/commit/289a07ebcb6e90b6c4f...) and it seems it wasn't intended to work like this so swapping
!emptywithissetwould be a good call imho.Comment #5
zaporylieHere's a quick patch but I guess this needs a proper test coverage as well.
Comment #6
zaporylieOk, here's #6 with basic test addition + test only patch.
Comment #9
zaporylieooops, missing Adjustment constructor in test. Hopefully, that will do.
Comment #11
jsacksick commentedlooks good to me.
Comment #12
jsacksick commentedCommitted, thanks!