-
Notifications
You must be signed in to change notification settings - Fork 7
fix(woo-memberships): prevent fatal when membership plan has been deleted #313
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -80,7 +80,13 @@ public static function membership_status_changed( $user_membership, $old_status, | |
| return; | ||
| } | ||
| $user_email = $user->user_email; | ||
| $plan_id = $user_membership->get_plan()->get_id(); | ||
|
|
||
| // Plan post may have been deleted; skip rather than fatal on ->get_id(). | ||
| $plan = $user_membership->get_plan(); | ||
| if ( ! $plan ) { | ||
| return; | ||
| } | ||
| $plan_id = $plan->get_id(); | ||
|
|
||
| $plan_network_id = get_post_meta( $plan_id, Admin::NETWORK_ID_META_KEY, true ); | ||
| if ( ! $plan_network_id ) { | ||
|
|
@@ -113,7 +119,13 @@ public static function membership_deleted( $user_membership ) { | |
| return; | ||
| } | ||
| $user_email = $user->user_email; | ||
| $plan_id = $user_membership->get_plan()->get_id(); | ||
|
|
||
| // Plan post may have been deleted; skip rather than fatal on ->get_id(). | ||
| $plan = $user_membership->get_plan(); | ||
| if ( ! $plan ) { | ||
| return; | ||
| } | ||
| $plan_id = $plan->get_id(); | ||
|
Comment on lines
+123
to
+128
|
||
|
|
||
| $plan_network_id = get_post_meta( $plan_id, Admin::NETWORK_ID_META_KEY, true ); | ||
| if ( ! $plan_network_id ) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,149 @@ | ||||||||||
| <?php | ||||||||||
| /** | ||||||||||
| * Test that Events listeners handle memberships with deleted plans gracefully. | ||||||||||
| * | ||||||||||
| * Regression test for NPPM-2742: a membership whose plan post was deleted | ||||||||||
| * caused a PHP fatal in Events::membership_deleted() and | ||||||||||
| * Events::membership_transferred() when calling get_plan()->get_id() on null. | ||||||||||
|
||||||||||
| * Events::membership_transferred() when calling get_plan()->get_id() on null. | |
| * Events::membership_status_changed() when calling get_plan()->get_id() on null. |
Copilot
AI
Apr 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The real failure mode described in the PR is get_plan() returning false when the plan post is deleted, but this test simulates a missing plan with null and the mock documents get_plan() as object|null. Consider using false (and updating the mock/docblocks) so the test matches WooCommerce Memberships behavior more closely and continues to validate the guard even if the production code changes to strict checks.
Copilot
AI
Apr 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this test, $plan and the assigned $plan->get_id closure are unused (the test uses $mock_plan instead). Removing the dead code will reduce confusion about how the mock plan is supposed to behave.
| $plan = (object) [ 'id' => $plan_id ]; | |
| $plan->get_id = function() use ( $plan_id ) { | |
| return $plan_id; | |
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR adds a new early-return path in
membership_status_changed()when the plan is missing, but the new unit test file only exercisesmembership_deleted(). Please add a regression test formembership_status_changed()with a membership whoseget_plan()returns falsey to ensure this listener also avoids the fatal and returns early as intended.