Conversation
17868ba to
db0466f
Compare
…ayment-method flow WooCommerce Subscriptions hijacks the pay-for-order page with its own handler, bypassing WC Core's `woocommerce_before_pay_action` hook. This adds a compat layer that hooks the Subscriptions action fired after nonce verification to verify the session and block on BLOCK decisions via redirect + exit, preventing both `update_payment_method()` and `process_payment()` from running.
db0466f to
292aa61
Compare
leonardola
left a comment
There was a problem hiding this comment.
The changes work as expected. But AI has flagged a few things.
AI aided review
- Feature flag bypass — woocommerce-fraud-protection.php:95-97
- Description: SubscriptionsChangePaymentCompat::register() is called directly in the bootstrap closure, bypassing FraudProtectionController::on_init() and its feature_is_enabled() gate. All other Protectors register through
the controller. The existing Compat/ classes (Stripe, Square) also register directly, but those are passive payment-data filters that only fire when the fraud pipeline is already active. This class actively verifies sessions
and blocks users. - Suggestion: Wire through FraudProtectionController: add a typed property, add it as an init() parameter, call $this->subscriptions_change_payment_compat->register() in on_init().
- Naming/location inconsistency — src/Compat/SubscriptionsChangePaymentCompat.php
- Description: Per CLAUDE.md, the Protector pattern is: "*Protector classes share the same shape — they take SessionVerifier, BlockedSessionNotice via init(), hook an action in register(), and call verify_and_block()." This
class follows that exact shape but lives in Compat/ with a Compat suffix. The existing Compat/ classes are passive filter-based data resolvers with no blocking logic. - Suggestion: Either rename to SubscriptionsChangePaymentProtector in src/, or document in the class docblock why it lives in Compat/ (e.g., because it targets a third-party extension's hook).
- Mock stub conflict in tests — SubscriptionsChangePaymentCompatTest.php:53-55 vs 162-166
- Description: setUp() stubs get_message_html with ->method() (blanket). Then test_verify_blocks_on_block_decision re-configures with ->expects($this->once()). The blanket stub may interfere depending on PHPUnit version.
- Suggestion: Remove the default get_message_html stub from setUp() and configure it per-test.
- Missing notice deduplication test — SubscriptionsChangePaymentCompatTest.php
- Description: The wc_has_notice guard at line 120 is untested. PayForOrderProtectorTest has an explicit test_verify_deduplicates_blocked_notice() for the same pattern.
- Suggestion: Add a test that pre-adds the notice, calls verify_and_block() with BLOCK, and asserts wc_notice_count('error') === 1.
- Trait docblock outdated — ClassicFormDataExtractionTrait.php:15
- Description: Says "Used by ShortcodeCheckoutProtector and AddPaymentMethodProtector" but now also used by PayForOrderProtector and SubscriptionsChangePaymentCompat.
- Suggestion: Update to a generic statement like "Used by classic form protectors and compat layers that read from $_POST."
- Local debug filter (not in PR) — woocommerce-fraud-protection.php:129-131
- Description: Unstaged add_filter('woocommerce_fraud_protection_decision', fn() => 'block') in the working tree. Not part of the PR diff, but flagging to ensure it doesn't get accidentally committed.
Remove the hardcoded list of classes using the trait in favor of a generic description, since the list was already outdated.
It targets a third-party extension hook and is only relevant when WooCommerce Subscriptions is active, unlike top-level Protectors which integrate with WC Core flows.
The blanket stub was only needed by the BLOCK test. Configuring it per-test makes the test intent clearer.
Test the wc_has_notice guard that prevents duplicate error notices when verify_and_block is called with a notice already present.
Compat layers register outside FraudProtectionController to keep the boundary between first-party components and third-party integrations clean. Making feature_is_enabled() static lets them check the feature flag without being wired into the controller.
|
Thanks for the review!
I'm not a fan of mixing the compat code with our logic, as these are for third-party extensions and should move to the appropriate repos eventually. The relationship is: compats are consumers of our APIs, not the other way around. I've updated
This is not a fraud protection protector but rather a third-party compatibility layer. I've added a comment clarifying this in 1f62894.
Removed the blanket stub in 98f205a.
I had initially added a test for that, but it seemed overkill. Added back in 119f373.
Fixed in 6dea3ea.
I think this was pasted mistakenly. It seems to come from the local files (due to testing the block scenario). I've triple checked, and this change does not exist in the PR. |
…erce-subscriptions
PayPalCompat is a third-party compat layer, not a first-party component. Like Stripe, Square, and Subscriptions compats, it should register itself in the bootstrap closure and guard with the static feature_is_enabled() check rather than being wired through the controller.
leonardola
left a comment
There was a problem hiding this comment.
The changes look mostly good. But I noticed the FraudProtectionController::feature_is_enabled() now always returns false for me.
The guard prevented compat layers from registering hooks when called during woocommerce_loaded (before init). It was inherited from WC core to avoid FeaturesController translation notices, but this is a standalone MU-plugin with no FeaturesController. Also adds the feature_is_enabled() check to Stripe and Square compat layers for consistency with PayPal and Subscriptions.
leonardola
left a comment
There was a problem hiding this comment.
Changes look good to me and work as expected now.
AI Aided review
wc_add_notice() persistence across redirect+exit
- File: src/Compat/SubscriptionsChangePaymentCompat.php:127-137
- Severity: warning
- Description: wc_add_notice() stores the notice in the WC session, then exit terminates the request. PHP's exit does invoke register_shutdown_function callbacks, which is how WC's session handler saves — so this should work.
But it's fragile and depends on PHP internals. If the notice doesn't persist, the user lands on the view-subscription page with no explanation. - Suggestion: Verify this works in manual testing (Test 2 in the PR description should confirm). If notices are lost, explicitly call WC()->session->save_data() before the redirect.
Missing @internal annotation on register()
- File: src/Compat/SubscriptionsChangePaymentCompat.php:89
- Severity: nit
- Description: PayPalCompat::register() and all protector register() methods have @internal. This one doesn't.
- Suggestion: Add @internal to the register() docblock.
Compat registration pattern diverges from documented convention
- File: woocommerce-fraud-protection.php:90-102
- Severity: nit
- Description: AGENTS.md says "Hook registration: Via FraudProtectionController::on_init()". Compat classes now self-register during woocommerce_loaded, with their own feature_is_enabled() guard. This is a reasonable
architectural decision (compats target third-party extensions), but the AGENTS.md checklist should be updated to reflect this pattern. - Suggestion: Update the PR review checklist in AGENTS.md to note that compat layers self-register.
…erce-subscriptions
…erce-subscriptions
Compat layers self-register with a feature_is_enabled() guard rather than going through FraudProtectionController::on_init().
Changes proposed in this Pull Request:
WooCommerce Subscriptions hijacks the pay-for-order page with its own request handler (
wp_loadedpriority 20), so WC Core'swoocommerce_before_pay_actionnever fires andPayForOrderProtector's server-side hook is bypassed. The JS side already works —pay-for-order.jsenqueues on the page (same#order_reviewform) and injects the session ID — but nothing consumed it server-side. (more details on the docs repo)This adds a compat layer (
SubscriptionsChangePaymentCompat) that hookswoocommerce_subscription_change_payment_method_via_pay_shortcode, which fires after nonce verification but before any mutations. On BLOCK, the request is stopped entirely viawp_safe_redirect()+exit, preventing bothupdate_payment_method()(which saves to DB and may trigger gateway cancellation) andprocess_payment()from running. On BLOCK, the user is redirected to the view-subscription page (rather than back to the change-payment form, which would be unusable sinceSessionBlockingHandlerremoves payment gateways in a blocked state).No new JS file is needed —
PayForOrderProtectoralready enqueuespay-for-order.json this page.Closes WOOSUBS-1474
subscriptions-change-payment-method-demo.mov
How to test the changes in this Pull Request:
Prerequisites:
Test 1: ALLOW path (normal change succeeds)
woocommerce-fraud-protectionlogs under WooCommerce > Status > Logs for a verify entry with sourcesubscriptions_change_payment_method.Test 2: BLOCK path (change is stopped)
woocommerce-fraud-protectionlogs under WooCommerce > Status > Logs for a verify entry with sourcesubscriptions_change_payment_method.