fix: expose sovereign tenant extension hooks#1278
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR systematically replaces hardcoded sovereign-tenant conditional guards throughout the checkout, UI, and compatibility layers with externally-controllable filter hooks, removing the centralized ChangesSovereign-tenant guard refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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: |
|
Performance Test Results Performance test results for bc6b737 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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
inc/ui/class-template-switching-element.php (1)
433-433: ⚡ Quick winDocument the new filter contract inline.
Since this is now a primary extension point, add a short hook docblock (
@param/expected boolean return) immediately abovewu_template_switching_skip_outputto make integration safer and clearer for addon authors.Proposed patch
+ /** + * Filters whether the Template Switching element output should be skipped. + * + * `@param` bool $skip Whether to skip rendering. + * `@param` array $atts Block/shortcode attributes. + * `@param` string|null $content Block/shortcode inner content. + * `@param` Template_Switching_Element $this Current element instance. + */ if (apply_filters('wu_template_switching_skip_output', false, $atts, $content, $this)) { return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@inc/ui/class-template-switching-element.php` at line 433, Add an inline hook docblock immediately above the apply_filters call for 'wu_template_switching_skip_output' documenting the filter contract: state that the filter expects and returns a boolean skip flag, list the parameters ($skip default false, $atts array, $content string|null, $this the current instance/object), and include an `@return` bool line; place this short PHPDoc block directly above the line containing apply_filters('wu_template_switching_skip_output', ...) in class-template-switching-element.php so addon authors can see the expected types and return value.tests/WP_Ultimo/UI/Sovereign_Mode_Elements_Test.php (1)
30-39: ⚡ Quick winAdd checkout element skip hook to this coverage map.
Line 30-39 sets up skip-output hooks for UI elements, but
wu_checkout_skip_outputis missing, leaving one migrated element guard untested.♻️ Proposed patch
$this->elements = [ 'wu_account_skip_output' => Account_Summary_Element::get_instance(), 'wu_billing_info_skip_output' => Billing_Info_Element::get_instance(), + 'wu_checkout_skip_output' => Checkout_Element::get_instance(), 'wu_invoices_skip_output' => Invoices_Element::get_instance(), 'wu_my_sites_skip_output' => My_Sites_Element::get_instance(), 'wu_current_membership_skip_output' => Current_Membership_Element::get_instance(), 'wu_current_site_skip_output' => Current_Site_Element::get_instance(), 'wu_template_switching_skip_output' => Template_Switching_Element::get_instance(), 'wu_domain_mapping_skip_output' => Domain_Mapping_Element::get_instance(), ];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/WP_Ultimo/UI/Sovereign_Mode_Elements_Test.php` around lines 30 - 39, Add the missing checkout skip-output hook to the elements coverage map by inserting the key 'wu_checkout_skip_output' mapped to Checkout_Element::get_instance() into the $this->elements array alongside the other entries (e.g., Account_Summary_Element::get_instance(), Billing_Info_Element::get_instance(), etc.) so the migrated checkout element guard is included in the test coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@inc/checkout/class-cart.php`:
- Around line 354-356: The constructor early-return when
apply_filters('wu_cart_skip_initialization', ...) leaves $this->errors empty so
Cart::is_valid() still returns true; instead, when the filter returns true
populate $this->errors with a WP_Error (e.g. 'cart_skipped' code and a short
message) in the same block before returning so downstream callers like
Checkout::process_order() can detect the failure via is_wp_error()/is_valid
checks; modify the block around the apply_filters call in the Cart class
constructor (same scope as the existing apply_filters line) to set a WP_Error on
$this->errors and then return.
In `@inc/checkout/class-checkout-pages.php`:
- Around line 134-135: filter_checkout_urls() currently bails unless the second
argument is a \WP_Post, but the page_link hook passes an int ID; update
filter_checkout_urls() to accept either a \WP_Post or an int by detecting when
$post (or the second parameter name) is numeric and calling get_post($post) to
retrieve the WP_Post object before proceeding, then continue using
get_signup_pages() as before so the wu_checkout_pages_checkout_url filter runs
for normal page permalinks too.
---
Nitpick comments:
In `@inc/ui/class-template-switching-element.php`:
- Line 433: Add an inline hook docblock immediately above the apply_filters call
for 'wu_template_switching_skip_output' documenting the filter contract: state
that the filter expects and returns a boolean skip flag, list the parameters
($skip default false, $atts array, $content string|null, $this the current
instance/object), and include an `@return` bool line; place this short PHPDoc
block directly above the line containing
apply_filters('wu_template_switching_skip_output', ...) in
class-template-switching-element.php so addon authors can see the expected types
and return value.
In `@tests/WP_Ultimo/UI/Sovereign_Mode_Elements_Test.php`:
- Around line 30-39: Add the missing checkout skip-output hook to the elements
coverage map by inserting the key 'wu_checkout_skip_output' mapped to
Checkout_Element::get_instance() into the $this->elements array alongside the
other entries (e.g., Account_Summary_Element::get_instance(),
Billing_Info_Element::get_instance(), etc.) so the migrated checkout element
guard is included in the test coverage.
🪄 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: bea9141b-3e77-41ca-9fe1-c1086090f31b
📒 Files selected for processing (26)
inc/admin-pages/customer-panel/class-account-admin-page.phpinc/admin-pages/customer-panel/class-add-new-site-admin-page.phpinc/admin-pages/customer-panel/class-checkout-admin-page.phpinc/admin-pages/customer-panel/class-my-sites-admin-page.phpinc/admin-pages/customer-panel/class-template-switching-admin-page.phpinc/checkout/class-cart.phpinc/checkout/class-checkout-pages.phpinc/checkout/class-checkout.phpinc/class-wp-ultimo.phpinc/compat/class-auto-delete-users-compat.phpinc/compat/class-edit-users-compat.phpinc/compat/class-multiple-accounts-compat.phpinc/functions/sovereign.phpinc/managers/class-gateway-manager.phpinc/ui/class-account-summary-element.phpinc/ui/class-billing-info-element.phpinc/ui/class-checkout-element.phpinc/ui/class-current-membership-element.phpinc/ui/class-current-site-element.phpinc/ui/class-domain-mapping-element.phpinc/ui/class-invoices-element.phpinc/ui/class-my-sites-element.phpinc/ui/class-template-switching-element.phptests/WP_Ultimo/Checkout/Checkout_Test.phptests/WP_Ultimo/UI/Sovereign_Mode_Elements_Test.phpviews/elements/sovereign-redirect.php
💤 Files with no reviewable changes (3)
- views/elements/sovereign-redirect.php
- inc/functions/sovereign.php
- inc/class-wp-ultimo.php
Comment on lines
+354
to
+356
| if (apply_filters('wu_cart_skip_initialization', false, $args, $this)) { | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Skipped carts still validate as usable.
If this filter returns true, the constructor exits with $this->errors empty and no cart state built. Cart::is_valid() will still return true, so Checkout::process_order() can keep going with a zero-item cart instead of stopping the flow.
💡 Suggested fix
- if (apply_filters('wu_cart_skip_initialization', false, $args, $this)) {
+ $skip_initialization = apply_filters('wu_cart_skip_initialization', false, $args, $this);
+
+ if ($skip_initialization) {
+ if ($skip_initialization instanceof \WP_Error) {
+ $this->errors->merge_from($skip_initialization);
+ } elseif ( ! $this->errors->has_errors()) {
+ $this->errors->add('cart_initialization_skipped', __('This checkout is not available.', 'ultimate-multisite'));
+ }
+
return;
}As per coding guidelines, "Use WP_Error for validation/operation failures instead of exceptions; check return values with is_wp_error()".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (apply_filters('wu_cart_skip_initialization', false, $args, $this)) { | |
| return; | |
| } | |
| $skip_initialization = apply_filters('wu_cart_skip_initialization', false, $args, $this); | |
| if ($skip_initialization) { | |
| if ($skip_initialization instanceof \WP_Error) { | |
| $this->errors->merge_from($skip_initialization); | |
| } elseif ( ! $this->errors->has_errors()) { | |
| $this->errors->add('cart_initialization_skipped', __('This checkout is not available.', 'ultimate-multisite')); | |
| } | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@inc/checkout/class-cart.php` around lines 354 - 356, The constructor
early-return when apply_filters('wu_cart_skip_initialization', ...) leaves
$this->errors empty so Cart::is_valid() still returns true; instead, when the
filter returns true populate $this->errors with a WP_Error (e.g. 'cart_skipped'
code and a short message) in the same block before returning so downstream
callers like Checkout::process_order() can detect the failure via
is_wp_error()/is_valid checks; modify the block around the apply_filters call in
the Cart class constructor (same scope as the existing apply_filters line) to
set a WP_Error on $this->errors and then return.
Comment on lines
+134
to
+135
| add_filter('post_type_link', [$this, 'filter_checkout_urls'], 10, 2); | ||
| add_filter('page_link', [$this, 'filter_checkout_urls'], 10, 2); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
WordPress hook reference: what arguments do the page_linkandpost_type_linkfilters pass to callbacks, and doespage_linkpass a page ID or aWP_Post object as its second parameter?
💡 Result:
- page_link filter: arguments passed to callbacks WordPress applies the page permalink with apply_filters( 'page_link', $link, $post->ID, $sample). That means your callback receives 3 arguments: (1) $link (string) the page’s permalink, (2) $post_id (int) the ID of the page, and (3) $sample (bool) whether it’s a sample permalink. [1][2] 2) post_type_link filter: arguments passed to callbacks WordPress applies the custom post type permalink with apply_filters( 'post_type_link', $post_link, $post, $leavename, $sample). That means your callback receives 4 arguments: (1) $post_link (string) the post’s permalink, (2) $post (WP_Post) the post object, (3) $leavename (bool), and (4) $sample (bool). [3][4] Does page_link pass a page ID or a WP_Post object as its second parameter? It passes the page ID (an int) as the second parameter ($post_id), not a WP_Post object. This is explicit in both the hook reference for page_link and in get_page_link where the filter is invoked with $post->ID as the second argument. [1][2]
Citations:
- 1: https://developer.wordpress.org/reference/hooks/page_link/
- 2: https://developer.wordpress.org/reference/functions/get_page_link/
- 3: https://developer.wordpress.org/reference/hooks/post_type_link/?output_format=md
- 4: https://developer.wordpress.org/reference/hooks/post_type_link/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="inc/checkout/class-checkout-pages.php"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "==> $FILE (lines 110-190)"
nl -ba "$FILE" | sed -n '110,190p'Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="inc/checkout/class-checkout-pages.php"
awk 'NR>=110 && NR<=190 {printf "%d\t%s\n", NR, $0}' "$FILE"Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 2541
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="inc/checkout/class-checkout-pages.php"
rg -n "function\s+get_signup_pages\s*\(" "$FILE"Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 123
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="inc/checkout/class-checkout-pages.php"
awk 'NR>=960 && NR<=1040 {printf "%d\t%s\n", NR, $0}' "$FILE"Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 2502
Fix filter_checkout_urls() to support page_link’s callback argument type
page_link passes the page ID (int) as the 2nd argument, but filter_checkout_urls() immediately returns unless the 2nd argument is a \WP_Post. Since get_signup_pages() returns page IDs for signup/login/update/etc., the wu_checkout_pages_checkout_url filter never runs on the normal page permalink path.
💡 Suggested fix
public function filter_checkout_urls($permalink, $post) {
+ if (is_numeric($post)) {
+ $post = get_post((int) $post);
+ }
if (! is_a($post, '\WP_Post')) {
return $permalink;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@inc/checkout/class-checkout-pages.php` around lines 134 - 135,
filter_checkout_urls() currently bails unless the second argument is a \WP_Post,
but the page_link hook passes an int ID; update filter_checkout_urls() to accept
either a \WP_Post or an int by detecting when $post (or the second parameter
name) is numeric and calling get_post($post) to retrieve the WP_Post object
before proceeding, then continue using get_signup_pages() as before so the
wu_checkout_pages_checkout_url filter runs for normal page permalinks too.
Collaborator
Author
Summary
Verification
Companion
Merged via PR #1278 to main. aidevops.sh v3.19.5 spent 13m on this as a headless bash routine. |
Collaborator
Author
Summary
Verification
Companion
Merged via PR #1278 to main. aidevops.sh v3.19.5 spent 32s on this as a headless bash routine. |
superdav42
added
the
review-feedback-scanned
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
1 participant
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
Verification
rg -n "sovereign|get_main_site_checkout_url|wu_is_sovereign_tenant|wu_mt_main_site_account_url|WU_MT_SOVEREIGN_TENANT|sovereign-redirect|functions/sovereign|sovereign\.php" inc tests viewsreturned no matches.vendor/bin/phpcson changed production files and UI test passed.vendor/bin/phpunit --filter 'Sovereign_Mode_Elements_Test|test_(create_order|maybe_handle_order_submission|check_user_exists|handle_inline_login)_returns_early_when_skip_filter_is_enabled|test_cart_constructor_sets_error_when_skip_filter_responds'passed: 6 tests, 22 assertions.Companion
ultimate-multisite-multi-tenancyto implement the sovereign callbacks for these hooks.Summary by CodeRabbit
Refactor
Tests