Merge pull request #4278 from craftcms/nathaniel/com-550-5x-disabled-variant-makes-it-impossible-to-change-sorting-of

[5.x] Disabled variants causing issues saving sort order when reordering

Author: nfourtythree

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## Unreleased
 
 - Fixed a PHP error that occurred when marking an inventory transfer as pending. ([#4267](https://github.com/craftcms/commerce/issues/4267))
+- 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
 

src/elements/Product.php

@@ -27,6 +27,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;
@@ -1166,13 +1167,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 +1213,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));
     }
 

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,
+            ],
+        ];
+    }
 }