Product and variation id mapping issues

Is this the same thing as the failing test I found over in #2899321: Refactor Commerce Product and ProfileBilling tests? In there, I suggested we use migration_lookup and transform to the actual id. If this is better, fine. But I'd still like to see a failing test too, which we have over there.

@andyg5000, glad you found something that worked for you.

I agree with heddn that a migration_lookup is desirable. Though I don't see the path yet on that because of the back referencing going on. Somewhere else mglaman suggested that a post event subscriber was needed to handle this problem, similar to the existing one for orders. After mulling it over, I see that as the way to do this. I have made a initial patch with that in mind and added comments in the migrations to caution that the product id in the product variation is not correct after running the product variation migration. It is a start.

Only run the post event MigrateProduct when the migration id is 'd7_product_displa'.

Now, add in the changes in patch #4. changing the destination from product_id to variation_id in the product variation migration (d7_product.yml). There is now a variation_id that we can use with migration_lookup in the product migration (d7_product_display.yml).

Modify the test since all product_ids in the product variation migration are now NULL, reflecting that the product has not yet been migrated.

Unfortunately, needs a re-roll.

1. +++ b/commerce_migrate.services.yml
   @@ -7,3 +7,7 @@ services:
   \ No newline at end of file
   

   Nit: missing new-line.

2. +++ b/modules/commerce/migration_templates/d7_product.yml
   @@ -6,7 +6,13 @@ migration_tags:
   +  # migrated in the product migration, d7_product_display,
   

   This was recently re-named.

3. +++ b/modules/commerce/tests/src/Kernel/Migrate/d7/ProductTest.php
   @@ -54,7 +54,13 @@ class ProductTest extends Commerce1TestBase {
   +    $this->assertProductVariationEntity($variation_id, '0', 'TOT1-GRN-OS', '16.000000', 'USD', '15', 'Tote Bag 1', 'default');
   
   +++ b/modules/commerce/tests/src/Kernel/Migrate/d7/ProductVariationTest.php
   @@ -42,13 +42,13 @@ class ProductVariationTest extends Commerce1TestBase {
   +    $this->assertProductVariationEntity(1, '0', 'TOT1-GRN-OS', '16.000000', 'USD', NULL, 'Tote Bag 1', 'default');
   

   So, if I get this right, it is NULL in the variation test, but now its accurate in the ProductTest? Great. That's what I'd expect.

Rerolled. 1 and 2 fixed.

Is there something to do for #3. It looks like you answered your own question.

Missed renaming some migrations.

Changing component since this is really only for Drupal Commerce 1 sources.

And doesn't need a reroll.

And this is blocker for #2814003: Remove commerce_migrate_commerce_reference and refactor references to use default migration process and #2899153: Cannot add variation to cart, due to failed store detection

Thoughts:

* We should replicate the AddToCartForm submit method in a test to help verify products can be added to a cart.
* Saving a Product will automatically populate it's variations back references, see https://github.com/mglaman/commerce/blob/8.x-2.x/modules/product/src/Ent... line 314. That should simplify the event subscriber

Seems like some work to me. Back to NW for #19.

#19.1 I think that is out of scope. All for testing that functionality, but lets do that in #2899153: Cannot add variation to cart, due to failed store detection. Worst case is the we keep adding modules and tests here. I think we have fixed the problem in the IS.

#19.2 The event subscriber has been simplified.

That's fine, we can add the test elsewhere.

Fix looks good to me, event subscriber simplified and tests pass!

Oh. This hasn't been committed!

Went to commit it and there is an extra line in the patch in #21. Let's fix that first

Now set as fixed.

In hindsight, hope no one minds. After all it is a blocker and the whitespace was not a code change.