Problem/Motivation

Given a store with french VAT, price included tax, in the checkout, if the client select (or Fill) an Address in another Country than FR or EU (test with USA), in the order infromation step, when going to the next step (Review) then we've got a fatal error.

TypeError : Argument 1 passed to Drupal\commerce_product_tax\Resolver\TaxRateResolver::setTaxType() must implement interface Drupal\commerce_tax\Entity\TaxTypeInterface, null given, called in /private/var/www/usine_mercuris/web/modules/contrib/commerce/modules/tax/src/Resolver/ChainTaxRateResolver.php on line 51 dans Drupal\commerce_product_tax\Resolver\TaxRateResolver->setTaxType() (

The store is configured for the Tax settings, with France as Tax registrations.

I'am using the Commerce Product Tax module, which provide a Resolver which is Tax Type Awre, ans so trigger the fatal error.

Looks like the root cause is on TaxOrderProcessor->getDefaultRates() in commerce_tax module

foreach ($this->getStoreZones($store) as $zone) {
      $rate = $this->chainRateResolver->resolve($zone, $order_item, $store_profile);
      if (is_object($rate)) {
        $rates[$zone->getId()] = $rate;
      }
    }

The chainRateResolver is called but without setting the taxType as does the LocalTaxTypeBase->resolveRates()

// Provide the tax type entity to the resolvers.
$this->chainRateResolver->setTaxType($this->parentEntity);

If the customer country is not VAT applicable, as the USA for French shop, so the $tax_adjustments is empty.

if (empty($tax_adjustments)) {
          $unit_price = $order_item->getUnitPrice();
          $rates = $this->getDefaultRates($order_item, $store);
          ...
        }

And then we call the getDefaultRates() method. And then we call the chainTaxRateResolver service but without any tax_type provided, and if some resolvers implements TaxTypeAwareInterface then everything breaks

Proposed resolution

The tricky part is now : how to provide the right tax_type to the getDefaultsRates() method.

If we have two tax type enabled on the site, let's say a "european_union_vat" tax type and a "shipping" tax type, the eu_vat is resolved correctly (let's say the reduced rate), but the shipping tax type is resolved to the default rate (standard).

And we can't know which is the right one to use.

I don't see any solutions by checking if taxType is NULL or not in the chainTaxRateResolver. Because the resolvers that are tax type aware need anyway this tax type and are called too anyway by the chainTaxRateResolver.

The only viable solution I can see should be to add a new settings on Tax Type, let's say "pass this tax type to resolvers in case tax not applicable", or even a settings 'Default Tax Type" and then we could iterable again on the tax types, if $tax_adjustments is empty and price include tax, to be able to collect the right tax types (configurable) and call the getDefaultsRate() with those tax type, and be able to remove adjustments from the unit price.

From comment #22
We need to split getStoreZones() into getStoreTaxType($store) and getStoreZones($store, $tax_type), and call both from getDefaultRates().
We'll then have the $tax_type to set on the resolvers.

Remaining tasks

- demonstrate the fatal error with tax type aware test
- find the right way to solve this issue :-)
- write the patch

User interface changes

(None

API changes

A new setting on the Tax Type ?

Data model changes

A new setting on the Tax Type ?

CommentFileSizeAuthor
#39 3113768-39-fix-tax-crash.patch5.95 KBbojanz
#33 3113768-interdiff-28-33.txt1.25 KBflocondetoile
#33 3113768-33.patch11.96 KBflocondetoile
#28 3113768-interdiff-25-28.txt702 bytesflocondetoile
#28 3113768-28.patch11.96 KBflocondetoile
#26 interdiff-3113768-23-25.txt4.71 KBflocondetoile
#25 3113768-25.patch11.94 KBflocondetoile
#23 3113768-23-text-only.patch7.96 KBflocondetoile
#10 3113768-10.patch1.95 KBflocondetoile
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

flocondetoile created an issue. See original summary.

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented
Issue summary: View changes
flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented

Not sure this issue is related to this module.
Looks like the root cause is on TaxOrderProcessor->getDefaultRates() in commerce_tax module

foreach ($this->getStoreZones($store) as $zone) {
      $rate = $this->chainRateResolver->resolve($zone, $order_item, $store_profile);
      if (is_object($rate)) {
        $rates[$zone->getId()] = $rate;
      }
    }

The chainRateResolver is called but without setting the taxType as does the LocalTaxTypeBase->resolveRates()

// Provide the tax type entity to the resolvers.
$this->chainRateResolver->setTaxType($this->parentEntity);
flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented
Issue summary: View changes
flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented
Project: Commerce Product Tax » Commerce Core
Version: 8.x-1.x-dev » 8.x-2.x-dev
Component: Code » Tax

Moving the issue in the right queue

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented

If the country is not VAT applicable, as the USA for French shop, so the $tax_adjustments is empty.

if (empty($tax_adjustments)) {
          $unit_price = $order_item->getUnitPrice();
          $rates = $this->getDefaultRates($order_item, $store);
          ...
        }

And then we call the getDefaultRates() method. And then we call the chainTaxRateResolver service but without any tax_type provided, and if some resolvers implements TaxTypeAwareInterface then everything breaks

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented
Issue summary: View changes
flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented
Title: Fatal error with the TaxTypeResolver » Fatal error when resolver are called by the chainRateResolver without a taxType set
flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented

The tricky part is now : how to provide the right tax_type to the getDefaultsRates() method.

If we have two tax type enabled on the site, let's say a "european_union_vat" tax type and a "shipping" tax type, the eu_vat is resolved correctly (let's say the reduced rate), but the shipping tax type is resolved to the default rate (standard).

And we can't know which is the right one to use.

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented
FileSize
3113768-10.patch1.95 KB

A first and quick patch. Still needs work and tests. So I let the issue in Active status

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented

I know how you can reproduce.

LocalTaxBase sets the tax type that gets passed down.

Tax type is null. System goes boom

I recently had this happen too and couldn't figure out how reproduce to save my life. But there's a singular way the tax type is set and then used later on. We need to do checking if the tax type is null

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented

No resolver that is tax type aware gets set the tax type by a tax plugin. That's what causes the crash.

US address so EU Vat didn't apply. But the system assumes the tax type would apply. It's like a gap if the tax type didn't avoid applying due to conditions?

Like I said, it definitely happened to me and I couldn't figure out why. But it was a EU site with VAT and I may have had a US address (didn't even think about that)

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented

I don't see any solutions by checking if taxType is NULL or not in the chainTaxRateResolver. Because the resolvers that are tax type aware need anyway this tax type and are called too anyway by the chainTaxRateResolver.

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented

The only viable solution I can see should be to add a new settings on Tax Type, let's say "pass this tax type to resolvers in case tax not applicable", and then we could iterable again on the tax types, if $tax_adjustments is empty and price include tax, to be able to collect the right tax types (configurable) and call the getDefaultsRate() with those tax type, and be able to remove adjustments from the unit price.

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented
Status: Active » Needs review

Just to trigger the bot to see what happens

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented
Status: Needs review » Needs work

We need tests about Tax Type Aware.

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented
Assigned: Unassigned » flocondetoile

Working on this.

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented
Issue summary: View changes
flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented
Issue summary: View changes
flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented
Issue summary: View changes
flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented
Issue summary: View changes

summarize all the debug steps done in the comments into the issue summary.

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented

Great work chasing this down, flocondetoile. I am very surprised we don't have test coverage for the code in question.

We need to split getStoreZones() into getStoreTaxType($store) and getStoreZones($store, $tax_type), and call both from getDefaultRates().
We'll then have the $tax_type to set on the resolvers.

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented
Status: Needs work » Needs review
FileSize
3113768-23-text-only.patch7.96 KB

A test only patch, wich provide a resolver tax type aware and order test integration with this kind of resolvers. Trigger the bot.

Status: Needs review » Needs work

The last submitted patch, 23: 3113768-23-text-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented
Status: Needs work » Needs review
FileSize
3113768-25.patch11.94 KB

Thanks for the input @bojanz. Attached the patch with the suggested approach.

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented
FileSize
interdiff-3113768-23-25.txt4.71 KB

Forgot the interdiff 23-25

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented

Tests are green but

1. may be, the tests would need to be improved by providing a second type of tax like the one provided by the commerce_shipping module ?

2. So the first tax type that matches the shop address is the only one that is ultimately retained. It seems that the order defined in the UI of the tax types is very important. We don't need this following anymore

// Assume that only a single tax type's zones will match.
        if (count($this->storeZones[$store_id]) > 0) {
          break;
        }

because we iterate on all the tax types, and the first which match the store adress is set in the $this->$storeZones[$store_id] array which is then defined and returned directly.

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented
Assigned: flocondetoile » Unassigned
FileSize
3113768-28.patch11.96 KB
3113768-interdiff-25-28.txt702 bytes
flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented
Issue summary: View changes
flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented
Issue summary: View changes
flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented

Tested patch #28 on a real project (the one where I encountered the bug), and looks that all is working fine now.

Also, about

may be, the tests would need to be improved by providing a second type of tax like the one provided by the commerce_shipping module ?

Looks like this is not necessary, as this kind of Tax Type does not implement LocalTaxTypeInterface and so is removed from the tax types collected.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented

Thank you @flocondetoile! I'll try to look it over more.

  1. +++ b/modules/tax/src/TaxOrderProcessor.php
@@ -128,13 +135,15 @@ class TaxOrderProcessor implements OrderProcessorInterface {
+        if (is_object($rate)) {

    Shouldn't we check the proper instance type 

  2. +++ b/modules/tax/tests/modules/commerce_tax_type_aware_test/commerce_tax_type_aware_test.info.yml
@@ -0,0 +1,8 @@
+name: Commerce Tax Type Awre Test

    s/Awre/Aware

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented
FileSize
3113768-33.patch11.96 KB
3113768-interdiff-28-33.txt1.25 KB

Patch updated with review from #32

sekoz’s picture

sekoz CreditAttribution: sekoz as a volunteer commented

I'm applying that 3113768-33.patch, no other patch on the commerce module.
I tried with EU VAT setup by default and shipping to Switzerland.
Once I reach the /review step I click "Go Back" to /order_information and it deducts the VAT twice from the cart total.
then click "continue to review", then on /review click "go back" again, it keeps subtracting VAT, you can reiterate until it eventually reaches zero or negative.

Drupal core 8.8.2
Drupal Commerce 8.x-2.16
Commerce Product Tax 8.x-1.0-rc1

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented

@sekoz
That doesn't sound related to this issue, which is about fixing the crash. Are you using overridden prices by any chance? It sounds like you are.

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
CreditAttribution: flocondetoile at Flocon de toile commented

@sekoz I tried to reproduce your issue without success.
Can you tell us how are configured your store (Prices are entered with taxes included ?, Billing ? Shipping ?) and your vat plugin (Display taxes of this type inclusive in product prices. ?)

sekoz’s picture

sekoz CreditAttribution: sekoz as a volunteer commented

right, prices are entered with taxes included and the "Display taxes of this type inclusive in product prices." box is checked in the Tax type option for EU VAT.
there are shipping fees but no taxes on that shipping.
something else?

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Assigned: Unassigned » bojanz
Status: Needs review » Needs work

Here's an initial review. I'll roll the next patch.

+   * @return \Drupal\commerce_tax\Entity\TaxTypeInterface[]
+   *   The tax types.
    */
-  protected function getStoreZones(StoreInterface $store) {
+  protected function getStoreTaxType(StoreInterface $store) {

Let's rename this to getDefaultTaxType(StoreInterface $store), so that it matches getDefaultRates().
We should also retain the current one-tax-type-per-store behavior.

+  protected function getStoreZones(StoreInterface $store, TaxTypeInterface $tax_type) {
+    $store_id = $store->id();
+    if (!isset($this->storeZones[$store_id])) {

Now that we have a static cache for the store tax types, let's remove the static cache for zones.
Speeding up zone matching will be handled generically in #2876974: Statically cache tax zone resolving.

+++ b/modules/tax/tests/modules/commerce_tax_type_aware_test/commerce_tax_type_aware_test.info.yml
@@ -0,0 +1,8 @@
+name: Commerce Tax Type Aware Test
+type: module

We already have a commerce_tax_test module, let's use it. We try to avoid having multiple test modules when not needed.

+/**
+ * Returns the tax rate configured on the product variation.
+ */
+class TaxRateResolver implements TaxRateResolverInterface, TaxTypeAwareInterface {

This docblock is not correct. I am also wondering if we can simplify the entire resolver to just assert that the tax type is set, since that's all we technically need here.

+class OrderTaxTypeAwareIntegrationTest extends OrderKernelTestBase {

I would expect this coverage to be a part of the existing OrderIntegrationTest, since that's the kernel test which covers the tax order processor in general.

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Status: Needs work » Needs review
FileSize
3113768-39-fix-tax-crash.patch5.95 KB

Here is a reduced version of #33, addressing my review in #38.

I would do #2981436: Create a service for getting the store's default tax type / rates as a followup, adding the additional test coverage that was started here.

  • bojanz committed 92f5208 on 8.x-2.x authored by flocondetoile 
    Issue #3113768 by flocondetoile, bojanz: Fatal error when resolver are...
bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Status: Needs review » Fixed

Committed. Thanks, flocondetoile!

  • bojanz committed 9b2e346 on 8.x-2.x 
    Issue #3113768 followup: Guard against a null tax type.

Status: Fixed » Closed (fixed)

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