Allow 0% tax rates to proceed to calculation

I'm not sure this is actually a bug, as a tax rate in Commerce is expected to entail a title / machine-name and an actual rate (i.e. a valid percentage). This function deals with applying a particular rate as described above to a given line item. If a rate is 0%, then there's nothing to do to apply it. However, if a module wants to be able to define a "percentageless" or "variable rate" tax rate and apply it using some extra criteria, it should be implementing hook_commerce_tax_type_calculate_rates(). In that case you'd have the opportunity to perform any custom calculation you'd like to determine a tax and add it to the same line item's unit price array. For a concrete example of this, see Commerce CyberSource's tax service integration.

Now, that's the current behavior and seems pretty straightforward, but I'm not sure you'll find those precise definitions littered throughout the inline documentation. It could be that simply defining the intended use of the API could resolve this issue unless you have a use case where the method described above would not be sufficient.

No, that's not there for the intention of altering any other rates. It simply lets you apply tax rates to a give line item arbitrarily, again the reference example being taxes calculated by CyberSource's tax calculation service. I thought the same thing with respect to the calculation_callback, thinking perhaps it's a semantic issue (should it instead be an "application" callback?) - but it does serve the purpose of actually calculating the tax based on the given rate and creating the necessary price component.

I'm still failing to see a use case, though, where you'd need the logic introduced by the patch. Given that we already have a hook for arbitrary tax calculation, and given that your scenario requires additional code (i.e. you can't just create a 0% tax rate in the UI and expect it to do anything, because it's still going to reference the same callback that presupposes a multiplication of that rate by the unit price amount), the only scenario I can imagine changing the code here supporting would be a calculated service - but we already have an integration point with a reference implementation using it.

The one-line hook description doesn't catch every use case, but the description inside the body of the function itself fleshes the intention out. Perhaps we just move that description inside the body into the doc block and use the usual // No example. inside the body. From commerce_tax.api.php:

/**
 * Allows modules to calculate taxes that don't determine applicability through
 *   default Rules components.
 *
 * @param $tax_type
 *   The tax type object whose rates should be calculated.
 * @param $line_item
 *   The line item to which the taxes should be applied.
 *
 * @see commerce_tax_type_calculate_rates()
 */
function hook_commerce_tax_type_calculate_rates($tax_type, $line_item) {
  // An implementation might contact a web service and apply the tax to the unit
  // price of the line item based on the returned data.
}

At the same time, I'd update the documentation for the calculation_callback property to indicate its relation to the rate actually specified by the tax rate, referring to the other hook for variable or unknown rate calculation. I think it could be fixed by just dropping 'rate' from the first line so it's apparent it's calculating the tax given a rate, not calculating / overriding the tax rate that will be used:

 *   - calculation_callback: name of the function used to calculate the tax rate
 *     for a given line item, returning either a tax price array to be added as
 *     a component to the line item's unit price or FALSE to not include
 *     anything; defaults to 'commerce_tax_rate_calculate'.

I'm confused about the purpose of the calculation callback, then. I have a custom module that provides a tax rate that uses the calculation callback to look up tax rates based on counties (kept up to date by state dept of revenue records).

This means that the 'rate' of the tax rate isn't actually used, but it's still required for the callback to be triggered.

I've provided a tax rate in code, why is it desirable to use a hook to modify an existing tax rate?

All I know is, for accounting purposes, it would be really nice to be able to assign an "Out of State" tax rate, set at 0%, with a negate rule for my home state. All the sales tax forms want it, but Commerce doesn't apply a 0% tax so my commerce_reports don't report what I need to file the tax forms.

@delzhand The purpose of the callback is best defined in the docblock of the default calculation callback: "Calculates a price array for the tax on the unit price of a line item." Notice what it's doing - it's taking a given tax rate and calculating the actual amount of tax that that rate requires the merchant to collect. It's not calculating the rate itself. The idea here is that a tax rate exists as an immutable thing; tax rates typically aren't variable, but there may be more than one tax rate that applies to a single product or different tax rates used for different classes of products. There are some flat taxes on goods, but those don't really fall under the use case of this part of the tax system.

Also, I don't understand your last question.

@derelict-en_us I can understand the accounting question, but the problem with using a 0% tax like this is that it's going to show up on your price component displays. That said, I don't suppose there's any reason for us to not apply 0% taxes, so I'm fine making this change:

    if (isset($tax_rate['rate']) && is_numeric($tax_rate['rate'])) {

It used to be !empty(), and I honestly can't say if it was intentional to not support a 0% tax rate or to just be used as an isset() check. Combined with the is_numeric() check, we can be assured that we won't be trying to apply a NULL tax rate - i.e. it will be 0 or some other valid number.

Lastly, I'm going to clean up the documentation so it's clear that the calculation callback isn't for determining the actual percentage of a tax rate but for calculating the actual amount of tax collected. Our desire at the time was to ensure that every tax collected is represented by a distinct tax rate, not that all manner of taxes would be collected at varying percentages under the name of a single tax rate. However, given the way CyberSource's tax system works, that simply isn't possible - with tax calculation services like this, you have to do this and trust the tax service to provide a report on who you actually need to pay.

To summarize the tax process here:

1. A line item represents a product being purchased and is passed through the price calculation system.
2. A default tax rule executes that for each tax type (sales tax, VAT, etc.) loads all tax rates of that type and attempts to apply them one by one in commerce_tax_type_calculate_rates().
3. The application process for each individual tax rate involves invoking the tax rate's calculation callback to create the necessary price component that will be added to the line item's unit price array.
4. After individual tax rates have been checked, hook_commerce_tax_type_calculate_rates() is invoked, allowing you to apply any other arbitrary tax to the line item.

Couldn't upload this in the previous comment for some reason, but here's what a 0% tax would appear like in the order totals: