New temporary German VAT rates starting from 1st July until 31 december 2020

I’m thinking we need an event for \Drupal\commerce_tax\Plugin\Commerce\TaxType\LocalTaxTypeBase::getZones

  /**
   * {@inheritdoc}
   */
  public function getZones() {
    if (empty($this->zones)) {
      $this->zones = $this->buildZones();
      // @todo dispatch event for altering the zones
    }

Who knows what VAT changes will come about. We can add an event to make this flexible. Maybe we then just make a commerce_tax_covid contribs that allows folks to contribute special tax changes due to the pandemic, and not everyone having to roll their own.

This is what the DE zone would look like with the change:

    $zones['de'] = new TaxZone([
      'id' => 'de',
      'label' => $this->t('Germany'),
      'display_label' => $labels['vat'],
      'territories' => [
        // Germany without Heligoland and Büsingen.
        ['country_code' => 'DE', 'excluded_postal_codes' => '27498, 78266'],
        // Austria (Jungholz and Mittelberg).
        ['country_code' => 'AT', 'included_postal_codes' => '6691, 6991:6993'],
      ],
      'rates' => [
        [
          'id' => 'standard',
          'label' => $labels['standard'],
          'percentages' => [
            ['number' => '0.19', 'start_date' => '2007-01-01'],
            ['number' => '0.16', 'start_date' => '2020-07-01', 'end_date' => '2020-12-01'],
          ],
          'default' => TRUE,
        ],
        [
          'id' => 'reduced',
          'label' => $labels['reduced'],
          'percentages' => [
            ['number' => '0.07', 'start_date' => '1983-07-01'],
          ],
        ],
      ],
    ]);

Specifically to add

            ['number' => '0.16', 'start_date' => '2020-07-01', 'end_date' => '2020-12-01'],

As this could become a dynamic thing (other countries reducing their VAT rates as well over the next weeks/months, or changing them again) could lead to the need of updating the module quite often. What about investing a bit more time and making it flexible by storing the rules in some JSON format (which canbe fetched and processed via cron from a public url, e. g.on drupal.org or drupalcommerce.org)? You could add an updated timestamp in the file and storing the timestamp of the last file version to check if the vat rules have changed or not.

@mglaman, you forgot the reduced rate change in #3

I would agree with #4.

It should IMO be possible to modify a regular tax rate during a defined time range in general.

We don't know what comes during the next years (another pandemic? a stock market crash? Hopefully not, but who knows?), so a government might have to react dynamically.

Problem is in case of the current situation, that the time runs away.

The commerce invoice module might be affected as well or does it get it's data from the same source?

Made a contrib part fort the start under https://www.drupal.org/project/commerce_tax_covid.

I just need to add an example for Germany.

  /**
   * {@inheritdoc}
   */
  public function getZones() {
    if (empty($this->zones)) {
      $this->zones = $this->buildZones();
      $event = new TaxCovidZonesEvent($this->zones);
      $this->zones = $this->eventDispatcher->dispatch(TaxCovidEvents::COMMERCE_TAX_COVID_ZONES, $event);
    }
    return $this->zones;
  }

I would say for core Commerce we should make two changes.

1. Add event to the getZones() as in the contrib version
2. I think we need to split buildZones() into two functions
a) defineZones() - where we hold the only array of definitions
b) buildZones() - looping trough array and create TaxZone object

  /**
   * {@inheritdoc}
   */
  public function getZones() {
    if (empty($this->zones)) { 
     $definitions = $this->defineZones()
       $event = new TaxZone($definitions);
      $definitions = = $this->eventDispatcher->dispatch(TaxEvents::COMMERCE_TAX_ZONES, $event);
      $this->zones = $this->buildZones($definitions);
    }
    return $this->zones;
  }

The event should be triggered after defineZones() where we would have all data in an array.
With that, altering the data in an event subscriber is going to be much easier for anyone. It's just manipulating arrays.

Currently, with event after build zones, we get an array of TaxZone objects where if we want to alter that tax zone it is needed either to copy entire definitions from the core and add additional lines or writing a lot of code to be able to do new TaxZone definition in the event itself.

Regarding comments that changing rates should be done trough UI, think it would need significant changes to the code, with moving to the configuration based definitions for all plugins, and doing initial import of default rates. But could be done also.

The Austrian Government just announced a temporary tax reduction as well: https://www.diepresse.com/5825263/mehrwertsteuer-fur-gastronomie-kultur-... (German)

They reduce both the standard Value Added Tax rate (20%) and the reduced VAT rate (10%) to 5% from 1 July to 31 December 2020.

But typically for Austria they add some complexity to it. That means that this reduction is not general but only applies to the following sectors:

• Arts
• Culture
• Gastronomy
• Hotels

This is still pending approval by the EU Commission though.

well, technically that means that we have a new super_reduced rate of 5% for the rest of the year, and developers need to take care that this one is used for any concerned products

Retitling as after internal discussions, we've decided to give a shot at reintroducing the "commerceguys/tax" library so we can fetch the tax rates from there.

Requiring the commerceguys/tax library allows us to not tag a new Commerce release everytime there's a change to a tax rate.

Attaching a patch with my first attempt at this (I have no idea if the tests will pass).

I noticed that the Canadian Sales tax, and Norwegian VAT rates aren't present in the library, so we'll have to add that, right now I'm fetching the rates from the library for EU and Switzerland.

A new TaxTypeRepository was introduced to allow people to decorate the service and potentially alter the tax types returned.

commerceguys/tax needs significant improvements. It still uses the abandoned commerceguys/zone library: https://github.com/commerceguys/tax/blob/master/composer.json#L10
Converting to the zone code we added to commerceguys/addressing in v1.0 will be more than a search/replace.

Also, the dataset wasn't kept in sync with the one in Commerce, it was maintained by the wider PHP community separately. All JSON definitions will need to be reviewed.

There is also a noticeable performance regression. Instead of a PHP array already present in code we are now loading ~60 files from disk (for the EU tax type), and then decoding JSON. Avoiding this performance hit was the motivation behind the commerce_tax rewrite in the 2.0 beta days, during which the library was abandoned.

This is going to be a lot of work, so that instead of tagging patch Commerce releases (e.g. 3.0.1) we tag patch library releases (e.g. 0.8.3)

@bojanz: These are valid concerns (especially the performance one, although I think there are ways to mitigate them), but didn't pay attention to the fact that the zone library itself was abandoned.

I still think tagging Commerce releases everytime a tax rate is updated is far from an ideal solution... But well, perhaps we should first focus on fixing the immediate need (i.e new DE tax rates that come into effect from July 1st) and then focus on finding the proper long term solution.

Ok, so the attached patch is adding the new temporary tax rates for Germany (The standard + the reduced).

It also adds a new event to allow altering the Tax zones).

In order to make it easier to manipulate tax zones (since they're immutable), I added toArray() and fromArray() methods to our value objects.
Doing that removes the need of an additional protected method to build the zone definitions which would be problematic and potentially introduce a breaking change (for people defining tax type plugins implementing/extending the local tax type interface/base).

It turns out we currently have no tests coverage for getZones() so I tried adding some.

I wonder if we shouldn't split the event + all the related changes and only commit the actual tax rates changes first.

@mglaman: thoughts?

Reviewing sometime today.

The problem with the approach of adding an event, is that it's up to the event subscriber to determine whether the rate being added isn't already present in core which makes it a bit impractical... and even though we're not tagging new releases everytime we make updates to tax rates, we should still keep them up to date.

We should still consider relying on the tax library after we fix it by not depending on an abandoned library.

Regarding the tax rate percentages... I think we need to set an end date to the current rates (not reduced ones, otherwise they're still going to be applied)...

See the following logic:

    foreach ($this->percentages as $percentage) {
      $start_date = $percentage->getStartDate($timezone);
      $start_time = $start_date->setTime(0, 0, 0)->format('U');
      $end_date = $percentage->getEndDate($timezone);
      $end_time = $end_date ? $end_date->setTime(0, 0, 0)->format('U') : 0;

      if (($start_time <= $time) && (!$end_time || $end_time >= $time)) {
        return $percentage;
      }
    }

And since the rate is technically still valid on december 31st, shouldn't we set the end date to January 1st 2021??

Ok, I updated the percentages array, with the patch from #13, because the previous standard rate doesn't have an end date, it'd always be the one applied...

We have 2 options:

Option 1 (approach implemented in the patch :

          'percentages' => [
            ['number' => '0.19', 'start_date' => '2007-01-01', 'end_date' => '2020-06-30'],
            ['number' => '0.16', 'start_date' => '2020-07-01', 'end_date' => '2020-12-31'],
            ['number' => '0.19', 'start_date' => '2021-01-01'],
          ],

Option 2:

          'percentages' => [
            ['number' => '0.16', 'start_date' => '2020-07-01', 'end_date' => '2020-12-31'],
            ['number' => '0.19', 'start_date' => '2007-01-01'],

While the option 2 "works", I think this isn't the right approach and it also seems more logical to me to have the percentages ordered by date (And this causes the rates summary table to be a bit confusing (See screenshots).

Option 1:

Option 2 :

1. +++ b/modules/tax/src/Plugin/Commerce/TaxType/EuropeanUnionVat.php
   @@ -300,7 +300,9 @@ class EuropeanUnionVat extends LocalTaxTypeBase {
   +            ['number' => '0.19', 'start_date' => '2007-01-01', 'end_date' => '2020-06-30'],
   +            ['number' => '0.16', 'start_date' => '2020-07-01', 'end_date' => '2020-12-31'],
   +            ['number' => '0.19', 'start_date' => '2021-01-01'],
   
   @@ -308,7 +310,9 @@ class EuropeanUnionVat extends LocalTaxTypeBase {
   -            ['number' => '0.07', 'start_date' => '1983-07-01'],
   +            ['number' => '0.07', 'start_date' => '1983-07-01', 'end_date' => '2020-06-30'],
   +            ['number' => '0.05', 'start_date' => '2020-07-01', 'end_date' => '2020-12-31'],
   +            ['number' => '0.07', 'start_date' => '2021-01-01'],
   

   I like this, option 1, it reads best.

2. +++ b/modules/tax/src/TaxRate.php
   @@ -61,6 +61,18 @@ class TaxRate {
   +  /**
   +   * Creates a new tax rate from the given array.
   +   *
   +   * @param array $rate
   +   *   The tax rate.
   +   *
   +   * @return static
   +   */
   +  public static function fromArray(array $rate) : TaxRate {
   +    return new static($rate);
   +  }
   

   Wait. Why do we need fromArray if the constructor is an array

3. +++ b/modules/tax/src/TaxRatePercentage.php
   @@ -56,6 +56,18 @@ class TaxRatePercentage {
   +  public static function fromArray(array $percentage) : TaxRatePercentage {
   +    return new static($percentage);
   +  }
   

   Again, do we need this method when it's the same as the constructor?

4. +++ b/modules/tax/src/TaxZone.php
   @@ -75,6 +75,18 @@ class TaxZone {
   +  public static function fromArray(array $zone) : TaxZone {
   +    return new static($zone);
   +  }
   

   See previous comments

5. +++ b/modules/tax/tests/modules/commerce_tax_test/src/EventSubscriber/BuildZonesEventSubscriber.php
   @@ -0,0 +1,48 @@
   +    $zones['de'] = new TaxZone($germany_zone);
   

   Per the previous comments: we don't even use the fromArray here, we construct a new zone. So I think we should drop the fromArray methods.

Beyond removing the fromArray methods, I give my +1

You're right, added fromArray() simply to mirror toArray() but it definitely feels unnecessary. Per our yesterday's discussions, I'm having second thoughts with the event we're adding since there are alternatives, which aren't necessarily complex (i.e by swapping the plugin class, or by defining a custom tax type plugin which simply extends European union VAT).

It probably is easier to just write a subscriber so maybe we should just stick with that solution (until we fix the library).

Let's keep the event for now. Until we re-integrate the tax library, or find a way to work it in there.

+++ b/modules/tax/src/TaxRatePercentage.php
@@ -56,6 +56,18 @@ class TaxRatePercentage {
+  /**
+   * Creates a new tax rate percentage from the given array.
+   *
+   * @param array $rate
+   *   The tax rate percentage.
+   *
+   * @return static
+   */
+  public static function fromArray(array $percentage) : TaxRatePercentage {
+    return new static($percentage);
+  }
+

😱 one remains

Committed!

Cool, thx 👍👍. Will there come a new stable release too?

https://www.drupal.org/project/commerce/releases/8.x-2.20

Just released :)