Merge branch '5.x' of github.com:craftcms/commerce into 5.x

lukeholder · lukeholder · commit 94bece0e9f92 · 2026-04-15T06:45:12.000-06:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,8 +2,10 @@
 
 ## Unreleased
 
+- Fixed a bug where variants weren’t being saved for product types with a Variant SKU Format that could cause duplicate SKUs. ([#4249](https://github.com/craftcms/commerce/issues/4249))
 - Fixed a PHP error that occurred when marking an inventory transfer as pending. ([#4267](https://github.com/craftcms/commerce/issues/4267))
 - Improved the performance of migrations when upgrading to 5.x. ([#4277](https://github.com/craftcms/commerce/issues/4277))
+- Fixed a bug where variants' sort order wasn’t being respected when reordering with disabled variants. ([#4270](https://github.com/craftcms/commerce/issues/4270))
 
 ## 5.6.1.1 - 2026-03-27
 
diff --git a/src/elements/Product.php b/src/elements/Product.php
@@ -14,6 +14,7 @@
 use craft\commerce\base\HasStoreInterface;
 use craft\commerce\base\StoreTrait;
 use craft\commerce\behaviors\CurrencyAttributeBehavior;
+use craft\commerce\db\Table;
 use craft\commerce\elements\actions\CreateDiscount;
 use craft\commerce\elements\actions\CreateSale;
 use craft\commerce\elements\conditions\products\ProductCondition;
@@ -27,6 +28,7 @@
 use craft\commerce\Plugin;
 use craft\commerce\records\Product as ProductRecord;
 use craft\controllers\ElementIndexesController;
+use craft\controllers\NestedElementsController;
 use craft\db\Query;
 use craft\elements\actions\CopyReferenceTag;
 use craft\elements\actions\Delete;
@@ -50,6 +52,7 @@
 use craft\helpers\ElementHelper;
 use craft\helpers\Html;
 use craft\helpers\Json;
+use craft\helpers\Sequence;
 use craft\helpers\StringHelper;
 use craft\helpers\UrlHelper;
 use craft\models\FieldLayout;
@@ -1166,13 +1169,13 @@ public function getCheapestVariant(bool $includeDisabled = false): ?Variant
     }
 
     /**
-     * Returns an array of the product's variants.
+     * Returns a collection of the product's variants.
      *
-     * @param bool $includeDisabled
+     * @param bool|null $includeDisabled
      * @return VariantCollection
      * @throws InvalidConfigException
      */
-    public function getVariants(bool $includeDisabled = false): VariantCollection
+    public function getVariants(?bool $includeDisabled = null): VariantCollection
     {
         if ($this->_variants === null) {
             if (!$this->id) {
@@ -1212,6 +1215,10 @@ public function getVariants(bool $includeDisabled = false): VariantCollection
             });
         }
 
+        // When reordering variants we need to make sure disabled variants are included when calculating sort order
+        // @TODO: Remove in 6.0 when updating `getVariants()` to start returning an element query instance.
+        $includeDisabled ??= Craft::$app->controller instanceof NestedElementsController;
+
         return $this->_variants->filter(fn(Variant $variant) => $includeDisabled || ($variant->getStatus() === self::STATUS_ENABLED));
     }
 
@@ -1778,6 +1785,34 @@ public function beforeValidate(): bool
                     Craft::error('Craft Commerce could not generate the supplied SKU format: ' . $e->getMessage(), __METHOD__);
                     $variant->sku = '';
                 }
+
+                if ($variant->sku) {
+                    $skuExistsQuery = function(string $sku, ?int $id) {
+                        $query = (new Query())
+                            ->select(['sku'])
+                            ->from(Table::PURCHASABLES)
+                            ->where(['sku' => $sku]);
+
+                        // Make sure it isn't for the purchasable we are currently saving
+                        if ($id) {
+                            $query->andWhere(['not', ['id' => $id]]);
+                        }
+
+                        return $query;
+                    };
+
+                    // Ensure there isn't a clash with an existing SKU when using auto formats
+                    if ($skuExistsQuery($variant->sku, $variant->id)->exists()) {
+                        // If there is a clash, we need to append a number to the end.
+                        $baseSku = $variant->sku;
+                        do {
+                            $seq = Sequence::next('sku::' . $baseSku);
+                            $newSku = $baseSku . '-' . $seq;
+                        } while ($skuExistsQuery($newSku, $variant->id)->exists());
+
+                        $variant->sku = $newSku;
+                    }
+                }
             }
         }
 
diff --git a/src/elements/Variant.php b/src/elements/Variant.php
@@ -42,6 +42,7 @@
 use craft\helpers\Db;
 use craft\helpers\ElementHelper;
 use craft\helpers\Html;
+use craft\helpers\Sequence;
 use craft\helpers\UrlHelper;
 use craft\models\FieldLayout;
 use Throwable;
@@ -725,6 +726,32 @@ public function updateSku(Product $product): void
             $language = Craft::$app->language;
             Craft::$app->language = $this->getSite()->language;
             $this->sku = Craft::$app->getView()->renderObjectTemplate($type->skuFormat, $this);
+
+            $skuExistsQuery = function(string $sku, ?int $id) {
+                $query = (new Query())
+                    ->select(['sku'])
+                    ->from(Table::PURCHASABLES)
+                    ->where(['sku' => $sku]);
+
+                // Make sure it isn't for the purchasable we are currently saving
+                if ($id) {
+                    $query->andWhere(['not', ['id' => $id]]);
+                }
+
+                return $query;
+            };
+
+            // Ensure there isn't a clash with an existing SKU when using auto formats
+            if ($skuExistsQuery($this->getSku(), $this->id)->exists()) {
+                // If there is a clash, we need to append a number to the end.
+                do {
+                    $seq = Sequence::next('sku::' . $this->sku);
+                    $newSku = $this->sku . '-' . $seq;
+                } while ($skuExistsQuery($newSku, $this->id)->exists());
+
+                $this->sku = $newSku;
+            }
+
             Craft::$app->language = $language;
         }
     }
diff --git a/tests/unit/elements/product/ProductGetVariantsTest.php b/tests/unit/elements/product/ProductGetVariantsTest.php
@@ -8,10 +8,12 @@
 namespace craftcommercetests\unit\elements\product;
 
 use Codeception\Test\Unit;
+use Craft;
 use craft\commerce\elements\db\VariantQuery;
 use craft\commerce\elements\Product;
 use craft\commerce\elements\Variant;
 use craft\commerce\elements\VariantCollection;
+use craft\controllers\NestedElementsController;
 use craftcommercetests\fixtures\ProductFixture;
 use ReflectionClass;
 
@@ -188,4 +190,88 @@ public function testGetVariantsIncludeDisabledParameter(): void
         }
         self::assertTrue($hasDisabledVariant);
     }
+
+    /**
+     * Tests every combination of the nullable $includeDisabled parameter against the
+     * NestedElementsController detection introduced alongside the signature change.
+     * Also asserts that the internal $_variants collection is never mutated by the filter.
+     *
+     * @dataProvider getVariantsNullableIncludeDisabledDataProvider
+     */
+    public function testGetVariantsNullableIncludeDisabled(?bool $includeDisabled, bool $useNestedElementsController, int $expectedCount): void
+    {
+        $originalController = Craft::$app->controller;
+
+        try {
+            if ($useNestedElementsController) {
+                $mockController = $this->getMockBuilder(NestedElementsController::class)
+                    ->disableOriginalConstructor()
+                    ->getMock();
+                Craft::$app->controller = $mockController;
+            }
+
+            $product = new Product();
+            $product->typeId = 2000;
+
+            $enabled = new Variant();
+            $enabled->enabled = true;
+            $enabled->sku = 'enabled-sku';
+
+            $disabled = new Variant();
+            $disabled->enabled = false;
+            $disabled->sku = 'disabled-sku';
+
+            $product->setVariants([$enabled, $disabled]);
+
+            $result = $product->getVariants($includeDisabled);
+            self::assertCount($expectedCount, $result);
+
+            // The internal collection must never be mutated by the filter —
+            // regardless of which parameter was passed, all set variants must be retained.
+            $reflection = new ReflectionClass($product);
+            $variantsProperty = $reflection->getProperty('_variants');
+            $variantsProperty->setAccessible(true);
+
+            /** @var VariantCollection $internalVariants */
+            $internalVariants = $variantsProperty->getValue($product);
+            self::assertInstanceOf(VariantCollection::class, $internalVariants);
+            self::assertCount(2, $internalVariants, '_variants must retain all variants regardless of the filter applied');
+        } finally {
+            // Clean up so the controller state does not leak into subsequent tests
+            Craft::$app->controller = $originalController;
+        }
+    }
+
+    /**
+     * @return array<string, array{includeDisabled: bool|null, useNestedElementsController: bool, expectedCount: int}>
+     */
+    public function getVariantsNullableIncludeDisabledDataProvider(): array
+    {
+        return [
+            // null resolves to false when no NestedElementsController is active
+            'null-no-controller-excludes-disabled' => [
+                'includeDisabled' => null,
+                'useNestedElementsController' => false,
+                'expectedCount' => 1,
+            ],
+            // null resolves to true when NestedElementsController is the active controller
+            'null-nested-elements-controller-includes-disabled' => [
+                'includeDisabled' => null,
+                'useNestedElementsController' => true,
+                'expectedCount' => 2,
+            ],
+            // Explicit false must override the NestedElementsController detection
+            'explicit-false-with-nested-elements-controller' => [
+                'includeDisabled' => false,
+                'useNestedElementsController' => true,
+                'expectedCount' => 1,
+            ],
+            // Explicit true must work even without a special controller
+            'explicit-true-without-controller' => [
+                'includeDisabled' => true,
+                'useNestedElementsController' => false,
+                'expectedCount' => 2,
+            ],
+        ];
+    }
 }
diff --git a/tests/unit/elements/product/ProductTest.php b/tests/unit/elements/product/ProductTest.php
@@ -11,9 +11,12 @@
 use craft\commerce\db\Table;
 use craft\commerce\elements\Product;
 use craft\commerce\elements\Variant;
+use craft\commerce\Plugin;
 use craft\db\Query;
+use craft\db\Table as CraftTable;
 use craftcommercetests\fixtures\ProductFixture;
 use DateTime;
+use ReflectionClass;
 
 /**
  * ProductTest
@@ -357,4 +360,110 @@ public function testSaveProductAndVariants(): void
         // Remove the product
         \Craft::$app->getElements()->deleteElementById($product->id, Product::class, null, true);
     }
+
+    /**
+     * @group Product
+     */
+    public function testSkuFormatGeneratesSkuWhenEmpty(): void
+    {
+        $this->setProductTypeSkuFormat(2001, 'generated-sku-from-format');
+
+        $product = new Product();
+        $product->title = 'SKU Format Test Product';
+        $product->typeId = 2001;
+        $product->enabled = false;
+
+        $variant = new Variant();
+        $variant->title = 'Test Variant';
+        // SKU intentionally not set — should be generated from skuFormat
+
+        $product->setVariants([$variant]);
+        $product->validate();
+
+        self::assertEquals('generated-sku-from-format', $variant->sku);
+
+        $this->setProductTypeSkuFormat(2001, null);
+    }
+
+    /**
+     * @group Product
+     */
+    public function testSkuFormatDeduplicatesWhenCollisionExists(): void
+    {
+        // Fixture variant 'rad-hood' already exists in the PURCHASABLES table.
+        // Reset the sequence so the suffix is always predictable (-1).
+        $this->resetSkuSequence('rad-hood');
+        $this->setProductTypeSkuFormat(2001, 'rad-hood');
+
+        $product = new Product();
+        $product->title = 'Collision Test Product';
+        $product->typeId = 2001;
+        $product->enabled = false;
+
+        $variant = new Variant();
+        $variant->title = 'Test Variant';
+        // No SKU — format generates 'rad-hood' which collides with the fixture variant
+
+        $product->setVariants([$variant]);
+        $product->validate();
+
+        self::assertEquals('rad-hood-1', $variant->sku);
+
+        $this->setProductTypeSkuFormat(2001, null);
+    }
+
+    /**
+     * @group Product
+     */
+    public function testSkuFormatDeduplicatesMultipleVariantsWithCollision(): void
+    {
+        // Fixture variant 'hct-white' already exists in the PURCHASABLES table.
+        // Reset the sequence so suffixes are always predictable (-1, -2).
+        $this->resetSkuSequence('hct-white');
+        $this->setProductTypeSkuFormat(2001, 'hct-white');
+
+        $product = new Product();
+        $product->title = 'Multi-Variant Collision Test';
+        $product->typeId = 2001;
+        $product->enabled = false;
+
+        $variant1 = new Variant();
+        $variant1->title = 'Variant One';
+
+        $variant2 = new Variant();
+        $variant2->title = 'Variant Two';
+
+        $product->setVariants([$variant1, $variant2]);
+        $product->validate();
+
+        self::assertEquals('hct-white-1', $variant1->sku);
+        self::assertEquals('hct-white-2', $variant2->sku);
+
+        $this->setProductTypeSkuFormat(2001, null);
+    }
+
+    private function resetSkuSequence(string $baseSku): void
+    {
+        \Craft::$app->getDb()->createCommand()
+            ->delete(CraftTable::SEQUENCES, ['name' => 'sku::' . $baseSku])
+            ->execute();
+    }
+
+    private function setProductTypeSkuFormat(int $typeId, ?string $skuFormat): void
+    {
+        $productTypesService = Plugin::getInstance()->getProductTypes();
+        // Ensure types are loaded into cache
+        $productTypesService->getAllProductTypes();
+
+        $reflection = new ReflectionClass($productTypesService);
+        $prop = $reflection->getProperty('_allProductTypes');
+        $prop->setAccessible(true);
+
+        foreach ($prop->getValue($productTypesService) as $type) {
+            if ($type->id === $typeId) {
+                $type->skuFormat = $skuFormat;
+                return;
+            }
+        }
+    }
 }
