Cannot delete product variations that have no referenced product

We don't have any tests for product variation or order item rollback. Only for product and order rollback. I think the first thing would be to add some level of tests, using the order and product rollback tests as an example.

Good point. And when working on it I found that the product rollback test is quite right either because the migrations need to be expanded first. There is a fail patch to demonstrate the problem.

Then a product variation rollback test is added.

+++ b/modules/ubercart/tests/src/Kernel/Migrate/uc6/ProductVariationRollbackTest.php
@@ -0,0 +1,29 @@
+class ProductVariationRollbackTest extends ProductVariationTest {
...
+    $product_variation_ids = [1, 2, 3, 4, 5];
+    foreach ($product_variation_ids as $product_variation_id) {
+      $product_variation = ProductVariation::load($product_variation_id);
+      $this->assertFalse($product_variation, "Product variation $product_variation_id exists.");
+    }

So, based on the IS it looks like we need to do some more to recreate the situation seen by Mike.

Based on what I'm seeing, I'm thinking that running a product_variation rollback prior to running a product migration will always fail, regardless of if there are any orphans

The test does run the product variation migration, then a rollback of product variations without errors. The product migration is not run at all.

I migrated the ck2 database using the UI, then used drush to rollback product variations. It worked for a while then stopped working on 'drinks'.

v@cm {.../modules/contrib/commerce_migrate} (pvrollbacktest)$ drush mr commerce1_product_variation
 [notice] Rolled back 54 items - done with 'commerce1_product_variation:tops'
 [notice] Rolled back 3 items - done with 'commerce1_product_variation:storage_devices'
 [notice] Rolled back 15 items - done with 'commerce1_product_variation:shoes'
 [notice] Rolled back 0 items - done with 'commerce1_product_variation:product'
 [notice] Rolled back 2 items - done with 'commerce1_product_variation:hats'
 [error]  Symfony\Component\Routing\Exception\InvalidParameterException: Parameter "commerce_product" for route "entity.commerce_product_variation.add_form" must match "[^/]++" ("" given) to generate a corresponding URL. in Drupal\Core\Routing\UrlGenerator->doGenerate() (line 204 of /opt/sites/d8/core/lib/Drupal/Core/Routing/UrlGenerator.php).

In SqlContentEntityStorage.php line 771:

  Parameter "commerce_product" for route "entity.commerce_product_variation.add_form" must match "[^/]++" ("" given) to generate a correspondin
  g URL.


In UrlGenerator.php line 204:

  Parameter "commerce_product" for route "entity.commerce_product_variation.add_form" must match "[^/]++" ("" given) to generate a correspondin
  g URL.

I debugged for a bit and got lost is routing, which I don't know so stopped. After that I was curious if this was limited to 'drinks'. I did many combinations of import and rollback of both products and product variations. And in the end 'drinks' always fails, 'storage devices' started to fail but 'tops' continued to import and rollback successfully. Also. once the error appears I wasn't able to use drush to get rid of it on rollback.

Im also experiencing the same issue when rolling back a commerce product variation migration.

Parameter "commerce_product" for route "entity.commerce_product_variation.add_form" must match "[^/]++"  [error]
("" given) to generate a corresponding URL. in Drupal\Core\Routing\UrlGenerator->doGenerate() 

I have observed that the same exception may be thrown when trying to remove an orphan product variation in a non-migration context, i.e. a custom module.

// Remove orphaned product variations.
if ($product_variation->getProduct() === NULL) {
  try {
    $product_variation->delete();
  }
  catch (EntityStorageException $e) {
  	// Error handling
  }
}

I've hit #9 also, so I think the bug here is upstream: Commerce should allow deleting variations that have lost their product reference.

WARNING: Super janky hack incoming.

If you need to repeatedly roll back migrations or bulk delete orphaned product variations, throw this in the Drupal\commerce_product\Entity\ProductVariation

I don't believe the product id even has to exist, but gets around the route generator limitation since a value now exists.

  /**
   * {@inheritdoc}
   */
  public function delete() {
    if (!$this->getProductId()) {
      $this->product_id = 1;
    }
    parent::delete();
  }

@andyg5000 Super janky hack from #11 works fine, I was just able to rollback variations with no issues.

If the problem is to reproduce the problem, here is a super easy way to reproduce it:

$v = \Drupal\commerce_product\Entity\ProductVariation::create([
  'type' => 'default',
  'title' => 'test',
  'sku' => '123456',
]);
$v->save();
$v->delete();

Just testing if this patch passes first, in then it should be super easy to write a test.

@andyg5000 #11 works for me while debugging migration. I love it when I get to hack Drupal :)

The patch provided in #13 solves the issue. Thank you also for providing steps to reproduce!

OK so here is a test only patch (that should fail) and a complete patch.

Just for reference, this only happens if you have the module menu_link_content enabled, which probably is the case for a lot of commerce sites.

So removing the needs tests tag.

Nice.

1. There may be less code.

--- a/modules/product/src/Entity/ProductVariation.php
+++ b/modules/product/src/Entity/ProductVariation.php
@@ -113,7 +113,7 @@ class ProductVariation extends CommerceContentEntityBase implements ProductVaria
       'delete-form',
       'collection',
     ];
-    if (in_array($rel, $requires_product_routes) === TRUE && !$this->getProductId()) {
+    if (in_array($rel, $requires_product_routes) && !$this->getProductId()) {

2. Unused use statement.

--- a/modules/product/tests/src/Kernel/ProductVariationMenuLinkTest.php
+++ b/modules/product/tests/src/Kernel/ProductVariationMenuLinkTest.php
@@ -2,7 +2,6 @@

 namespace Drupal\Tests\commerce_product\Kernel;

-use Drupal\commerce_product\Entity\ProductVariation;

3. There may be less code.

   /**
-   * Modules to enable.
-   *
-   * @var array
+   * {@inheritdoc}
    */
   public static $modules = [
     'path',

4. Typo van/can.

@@ -47,7 +44,7 @@ class ProductVariationMenuLinkTest extends CommerceKernelTestBase {
   }

   /**
-   * Test that a variation van be deleted even if it has no parent product.
+   * Tests that a variation can be deleted even if it has no parent product.

Fixed #19 and RTBC+1

RTBC +1

Great patch from Krzysztof, problems solved for the issues above. There are still some issues when dealing with multi language.

`Symfony\Component\Routing\Exception\InvalidParameterException: Parameter "commerce_product" for route "entity.commerce_product_variation.content_translation_..." must match "[^/]++" ("" given) to
generate a corresponding URL. in Drupal\Core\Routing\UrlGenerator->doGenerate() (line 204 of`

This error gets thrown for the routes: content-translation-overview, add, edit and delete.

Works great. Thank you!

1. The ProductVariationMenuLinkTest is super small, let's merge it into the ProductVariationTest, which is a kernel test covering the entity in question.

2.

+    // If the parent product reference is lost, we have no way of generating
+    // URLs for these routes.
+    $requires_product_routes = [
+      'add-form',
+      'edit-form',
+      'duplicate-form',
+      'delete-form',
+      'collection',
+      'drupal:content-translation-overview',
+      'drupal:content-translation-add',
+      'drupal:content-translation-edit',
+      'drupal:content-translation-delete',
+    ];
+    if (in_array($rel, $requires_product_routes) && !$this->getProductId()) {

It's a bit unfortunate that we're hardcoding the route names here.
Can't we do something like:

    $link_templates = $this->linkTemplates();
    if (isset($link_templates[$rel])) {
      // Avoid a crash if a route depends on a product, but the
      // product ID is missing.
      if (strpos($link_templates[$rel], '{commerce_product}') && !$this->getProductId()) {
        throw new RouteNotFoundException();
      }
    }

Or maybe even something as extreme as:

      if (!$this->getProductId()) {
        // Most variation routes depend on the product and will crash without it.
        throw new RouteNotFoundException();
      }

Given that the next existing chunk of code was:

if (in_array($rel, ['canonical', 'revision'])) {
      $route_name = 'entity.commerce_product.canonical';
      $route_parameters = [
        'commerce_product' => $this->getProductId(),
      ];

and those weren't in the list of route names, I moved it to the most extreme, just does this have a product ID or not.

Also adjusted the test as per your comments.

Tweaked #27 and committed. Thanks, everyone!

This change needs a comment. In the future, we will not know why this module has been added.

    'commerce_product',
+    'menu_link_content',
  ];

After deleting this line, the test will still pass, but without 'menu_link_content' this test does not verify that everything is still working.

Ok, I see the comment in commit https://git.drupalcode.org/project/commerce/commit/7bf1a4e

also it looks like it only works with default product_variation.
what happens if a custom variation type is used?

 // Confirm that a variation can be deleted even if it has no reference.
+    $variation = ProductVariation::create([
+      'type' => 'default',