Revert billing-period switch downgrades (PR #889, #893)#896
Conversation
Reverts the changes from PR #889 (GH#888) and follow-up PR #893 that routed billing-period switches (monthly<->yearly) through the scheduled downgrade path. The swap system only updates the local membership record - it does not modify or cancel the Stripe/PayPal subscription. A period change would leave the gateway subscription on the old interval while the local membership thinks it changed, causing payment mismatches at renewal. Restores the original guard that blocks cross-period changes, with an improved error message suggesting plan variations instead of the previous unhelpful 'You already have an active X agreement' text.
📝 WalkthroughWalkthroughThe billing period switch detection logic in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@inc/checkout/class-cart.php`:
- Around line 2706-2712: The code dereferences $this->membership when
get_cart_type() === 'downgrade' (in the block that returns
strtotime($membership->get_date_expiration())) which can NPE if custom code sets
cart_type to 'downgrade' without calling build_from_membership(); add a
defensive null-guard: check that $this->membership is not null (and that methods
used on it like is_active() and get_date_expiration() exist/return expected
values) before dereferencing, and return the safe default (e.g., null or false)
when membership is null; apply the same guard to the other downgrade branch that
references $this->membership so both downgrade paths are safe even for
out-of-tree cart_type modifications, leaving build_from_membership() logic
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1092527b-89b5-45e7-8e35-31f8b882d436
📒 Files selected for processing (2)
inc/checkout/class-cart.phptests/WP_Ultimo/Checkout/Cart_Test.php
💤 Files with no reviewable changes (1)
- tests/WP_Ultimo/Checkout/Cart_Test.php
| if ($this->get_cart_type() === 'downgrade') { | ||
| $membership = $this->membership; | ||
|
|
||
| if ($membership && ($membership->is_active() || $membership->get_status() === Membership_Status::TRIALING)) { | ||
| if ($membership->is_active() || $membership->get_status() === Membership_Status::TRIALING) { | ||
| return strtotime($membership->get_date_expiration()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify there are no other writers of $this->cart_type = 'downgrade' that bypass build_from_membership()
rg -nP --type=php "cart_type\s*=\s*['\"]downgrade['\"]" -C2
# And confirm no public setter exists for cart_type
rg -nP --type=php '\bset_cart_type\b' -C2Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 510
🏁 Script executed:
sed -n '1350,1380p' inc/checkout/class-cart.php | cat -nRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 1728
🏁 Script executed:
sed -n '1300,1375p' inc/checkout/class-cart.php | head -20Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 774
🏁 Script executed:
rg -n "function build_from_membership" inc/checkout/class-cart.php -A 5Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 298
🏁 Script executed:
sed -n '788,822p' inc/checkout/class-cart.php | cat -nRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 1233
🏁 Script executed:
rg -n "^\s*(public|protected|private)\s+function\b" inc/checkout/class-cart.php | awk -F: '{print $1}' | sort -n > /tmp/methods.txt && cat /tmp/methods.txt && echo "---" && sed -n '1371p' inc/checkout/class-cart.phpRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 475
Null-guard revert removes safety check for out-of-tree cart_type modifications.
$this->membership is dereferenced directly in the cart_type === 'downgrade' branches at lines 2706-2712 and 2747-2755. The invariant that $this->membership is assigned before this code executes is maintained in-tree: build_from_membership() assigns the membership at line 822, and the only assignment of cart_type = 'downgrade' occurs within that method at line 1371. No public setter for cart_type exists.
However, custom integrations or filters that set cart_type = 'downgrade' without invoking build_from_membership() will now fatal-error on null dereference instead of returning a safe default. This is a conscious trade-off of the null-guard revert that should be accepted intentionally.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@inc/checkout/class-cart.php` around lines 2706 - 2712, The code dereferences
$this->membership when get_cart_type() === 'downgrade' (in the block that
returns strtotime($membership->get_date_expiration())) which can NPE if custom
code sets cart_type to 'downgrade' without calling build_from_membership(); add
a defensive null-guard: check that $this->membership is not null (and that
methods used on it like is_active() and get_date_expiration() exist/return
expected values) before dereferencing, and return the safe default (e.g., null
or false) when membership is null; apply the same guard to the other downgrade
branch that references $this->membership so both downgrade paths are safe even
for out-of-tree cart_type modifications, leaving build_from_membership() logic
unchanged.
|
Performance Test Results Performance test results for 63e753b are in 🛎️! Note: the numbers in parentheses show the difference to the previous (baseline) test run. Differences below 2% or 0.5 in absolute values are not shown. URL:
|
1 participant
Summary
Reverts PR #889 and follow-up PR #893 which routed billing-period switches (monthly↔yearly) through the scheduled downgrade path.
Why revert
The scheduled swap system (
Membership::swap()) only updates the local membership record — it does not modify or cancel the Stripe/PayPal subscription. A cross-period change (e.g. yearly → monthly) would:The original guard that blocked these changes was correct. The upgrade/downgrade swap path was designed for same-period plan changes where the gateway subscription interval stays the same.
What changed
Cart::calculate_swap()$orig_old_price_per_day/$orig_new_price_per_daycaptures (PR t892: fix period-switch per-day rate comparison and downgrade null-guard test #893)$is_period_switch_to_shorterflag and its OR condition in the downgrade checkget_billing_next_charge_date()(PR GH#888: fix(checkout): allow billing-period switches as scheduled downgrades #889)Verification
All 117 Cart_Test tests pass (the 4 removed tests were specific to the reverted behaviour).
Summary by CodeRabbit
Release Notes