[5.x] Fix applying default variant to provisional drafts

[lukeholder]

Summary

Fixes #4361 — when a variant's default status was changed via Set default variant while the product had an open provisional draft, the change didn't stick after the draft was applied, and the underlying commerce_variants.isDefault column could end up with two variants flagged true for the same product.

This PR also deprecates the use of isDefault in the database on a variant. VariantQuery now derives commerce_products.defaultVariantId always.

[Copilot]

🟡 Changes recommended

Cleanup can leave stale legacy flags, and provisional-draft application lacks regression coverage.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes provisional-draft default variant persistence and makes defaultVariantId authoritative.

Changes:

• Resolves draft variant IDs and synchronizes legacy flags.
• Derives isDefault queries from product data.
• Adds tests, compatibility documentation, and changelog updates.

File summaries

File	Summary and final review notes
tests/unit/elements/variant/VariantQueryTest.php	Tests query/display consistency. Nit (1 vote): add provisional-draft application coverage.
src/elements/Variant.php	Documents retained legacy flag compatibility.
src/elements/Product.php	Resolves defaults and repairs flags. Moderate (2 votes): clear stale flags when no valid default exists. Nit (2 votes): add integration coverage for provisional-draft application and canonical-row repair.
src/elements/db/VariantQuery.php	Derives default filtering from defaultVariantId.
src/elements/actions/SetDefaultVariant.php	Documents compatibility writes.
CHANGELOG.md	Records the fix.

Review details

Suppressed comments (1)

tests/unit/elements/variant/VariantQueryTest.php:577

• This test only exercises the query/display result after manually flipping commerce_variants.isDefault; it does not cover the reported provisional-draft apply path that this PR changes in Product::getDefaultVariant() and Product::afterSave(). Add a regression test that creates a provisional draft, changes the default, applies it, and asserts the resulting defaultVariantId, variant order, and single raw isDefault flag, so the main fix is actually protected.

    public function testIsDefaultDerivesFromDefaultVariantId(): void

• Files reviewed: 6/6 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[nfourtythree]

I've had a look through this one. The PR tests all pass for me locally, and the changes (making defaultVariantId the source of truth) makes sense.

I did find a couple of things with getDefaultVariant() though.

Extra query when the default variant is disabled

If the default variant is disabled, $variants->firstWhere('id', $this->defaultVariantId) won't find it, so the new fallback query runs. Nothing gets cached, so it runs again on every call. I wrote a test that calls getDefaultVariant() 5 times on a product with a disabled default, and it ran 5 queries. On 5.x it runs none. Anything looping over products and hitting product.defaultVariant will pick up an extra query per product.

Also, if canonicalId is null, the ?? $canonicalVariantId['id'] part just searches for the same ID that already wasn't found, so that bit can probably go.

Maybe we only do the lookup when the product isn't canonical, and/or cache the result?

Only covers one direction

The fallback handles defaultVariantId pointing at a derivative when the list has the canonical variant. It doesn't cover the opposite case, where a draft has a 'derivative B' in its variants and defaultVariantId still points at canonical B. Then it falls back to the first variant. If the draft gets saved like that, the first variant gets stored as the default and the apply carries it over to the live product.

To be fair, this isn't new. It fails the same way on 5.x, and the normal CP flow seems fine because saving the variant updates the draft's defaultVariantId. I could only reproduce it when the derivative was created another way (e.g. duplicateElement()).

Since we're already in here though, something like this would cover it without hitting the DB:

  $defaultVariant = $variants->firstWhere('id', $this->defaultVariantId)                                                                                            
      ?? $variants->first(fn(Variant $v) => $v->getCanonicalId() === $this->defaultVariantId);

I've got a test file covering both of these locally (not committed). Want me to look into a fix and push it up with the tests, or would you rather take it from here?

[lukeholder]

Thanks

Yeah the extra query is there, I got 5 queries over 5 calls with a disabled default. So yeah the fallback lookup isn't needed. During applyDraft the canonical product's variants are loaded from the draft, so a derivative defaultVariantId matches directly and afterSave() maps it to the canonical ID. The lookup only ever fired in the disabled-default case, where it found nothing. So I've removed it rather than caching it, and getDefaultVariant() is back to being query-free.

For the other direction, I agree, but since it behaves the same on 5.x and doesn't happen through the normal CP flow, I'm going to leave it out of this PR.