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.

Comments

zaporylie created an issue. See original summary.

jsacksick’s picture

so I guess we should just replace the !empty() by isset().

zaporylie’s picture

That was my impression as well but wanted to check first (with git blame) whether there was a reason for this.

zaporylie’s picture

The 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 !empty with isset would be a good call imho.

zaporylie’s picture

Status: Active » Needs review
StatusFileSize
new1.23 KB

Here's a quick patch but I guess this needs a proper test coverage as well.

zaporylie’s picture

StatusFileSize
new662 bytes
new1.88 KB

Ok, here's #6 with basic test addition + test only patch.

The last submitted patch, 6: 3187199-6-TEST-ONLY.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 6: 3187199-6.patch, failed testing. View results

zaporylie’s picture

Status: Needs work » Needs review
StatusFileSize
new710 bytes
new1.92 KB

ooops, missing Adjustment constructor in test. Hopefully, that will do.

The last submitted patch, 9: 3187199-9-TEST-ONLY.patch, failed testing. View results

jsacksick’s picture

Title: Adjustment percentage member proprty cannot be set to 0 » Adjustment percentage member property cannot be set to 0
Status: Needs review » Reviewed & tested by the community

looks good to me.

jsacksick’s picture

Status: Reviewed & tested by the community » Fixed

Committed, thanks!

  • jsacksick committed 5cfde49 on 8.x-2.x authored by zaporylie
    Issue #3187199 by zaporylie: Adjustment percentage member property...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.