[5.x] Fix applying default variant to provisional drafts - #4363
Open
lukeholder wants to merge 3 commits into
Open
lukeholder wants to merge 3 commits into
lukeholder wants to merge 3 commits into
Conversation
lukeholder
marked this pull request as ready for review
September 15, 2026 02:49
lukeholder
changed the title
Contributor
There was a problem hiding this comment.
🟡 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
isDefaultqueries 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 inProduct::getDefaultVariant()andProduct::afterSave(). Add a regression test that creates a provisional draft, changes the default, applies it, and asserts the resultingdefaultVariantId, variant order, and single rawisDefaultflag, 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.
Comment on lines
+1712
to
+1713
| if ($this->getIsCanonical() && $defaultVariant?->id) { | ||
| // Make sure exactly one canonical variant is flagged as the default. This is normally kept in |
Comment on lines
+1712
to
+1716
| if ($this->getIsCanonical() && $defaultVariant?->id) { | ||
| // Make sure exactly one canonical variant is flagged as the default. This is normally kept in | ||
| // sync by `SetDefaultVariant`/`Variant::afterSave()`, but that update is deferred while a | ||
| // variant's default status is changed from within a provisional draft (so as to not affect the | ||
| // canonical product before the draft is applied), and can otherwise be missed when the draft |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
VariantQuerynow derivescommerce_products.defaultVariantIdalways.