Cross sell#2538
Conversation
WalkthroughRegisters an admin_menu hook for FrmSalesApi::menu and extends FrmSalesApi to parse, validate, store, and expose cross-sell text/link pairs; adds logic to select an index and redirect via admin_init when the submenu slug is requested. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant WP as WordPress
participant HC as FrmHooksController
participant SA as FrmSalesApi
WP->>HC: load_admin_hooks()
HC->>WP: add_action('admin_menu', SA::menu, 1000)
Note over SA: submenu only added if cross-sell data present
WP->>SA: SA::menu()
alt cross-sell data present
SA->>WP: add_submenu_page(... 'frm-sales-api-cross-sell' ...)
SA->>WP: add_action('admin_init', redirect handler)
WP->>WP: admin_init triggers handler when page slug matches
SA->>SA: determine_cross_sell_index()
SA->>WP: wp_redirect(cross_sell_link[index])
WP-->>User: Redirect to cross-sell URL
else no cross-sell data
SA-->>WP: no submenu registered
end
sequenceDiagram
autonumber
participant Payload as Sales Payload
participant SA as FrmSalesApi
Payload-->>SA: set_sales($sales)
alt contains cross_sell_text/link
SA->>SA: cross_sell_is_valid()
alt valid
SA->>SA: determine_cross_sell_index()
SA->>SA: set_cross_sell(text[], link[])
else invalid
SA-->>SA: ignore cross-sell fields
end
else no cross-sell fields
SA-->>SA: proceed with normal sale processing
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
classes/models/FrmSalesApi.php (1)
32-45: Docblocks: replace placeholder version and consider explicit null defaultsUpdate @SInCE from x.x to the real release version. Optional: initialize to null explicitly for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/controllers/FrmHooksController.php(1 hunks)classes/models/FrmSalesApi.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
classes/models/FrmSalesApi.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4617)simple_get(703-712)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Run PHP Syntax inspection (8.3)
- GitHub Check: Cypress
🔇 Additional comments (1)
classes/controllers/FrmHooksController.php (1)
202-204: Hook registration looks goodLate priority ensures ordering under the parent menu. No issues.
WalkthroughRegisters a new admin menu hook to FrmSalesApi::menu. Expands FrmSalesApi with cross-sell text/link handling parsed from sales data, adds a redirecting submenu page when cross-sell data is available, and wires an admin_init redirect for that page. No public API removals; existing sales parsing augmented. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Admin User
participant WP as WordPress Admin
participant Hooks as FrmHooksController
participant API as FrmSalesApi
Admin->>WP: Load wp-admin
WP->>Hooks: do_action('admin_menu')
Hooks->>API: FrmSalesApi::menu()
API->>API: Initialize + ensure sales data loaded
alt Cross-sell text/link present
API->>WP: add_submenu_page(slug=frm-cross-sell)
WP-->>Admin: Submenu item rendered
Admin->>WP: Click submenu (frm-cross-sell)
WP->>WP: do_action('admin_init')
WP->>API: Redirect handler
API-->>Admin: wp_redirect(cross_sell_link)
else No cross-sell data
API-->>WP: No submenu registered
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/controllers/FrmHooksController.php(1 hunks)classes/models/FrmSalesApi.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
classes/models/FrmSalesApi.php (2)
classes/controllers/FrmAppController.php (1)
menu(11-24)classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4617)simple_get(703-712)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (1)
classes/controllers/FrmHooksController.php (1)
202-204: LGTM: hook placement is appropriate.Registering FrmSalesApi::menu at priority 1000 is fine and runs after the top‑level Formidable menu is added.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
classes/models/FrmSalesApi.php (3)
85-92: Critical: Static call to non-static method.Line 89 calls
self::set_cross_sale( $sale )butset_cross_saleis a private instance method (line 100). This issue was flagged in previous reviews and remains unresolved.Apply this diff:
- if ( is_array( $sale ) && isset( $sale['cross_sell_text'] ) ) { - self::set_cross_sale( $sale ); - } + if ( is_array( $sale ) && isset( $sale['cross_sell_text'] ) ) { + $this->set_cross_sale( $sale ); + }
94-108: Major: Sanitize API data before storing.The cross-sell values come from a remote API and are used in admin UI/redirects without sanitization. This issue was flagged in previous reviews and remains unresolved.
Apply this diff:
private function set_cross_sale( $data ) { if ( ! empty( $data['cross_sell_text'] ) ) { - self::$cross_sell_text = $data['cross_sell_text']; + self::$cross_sell_text = sanitize_text_field( wp_strip_all_tags( $data['cross_sell_text'] ) ); } if ( ! empty( $data['cross_sell_link'] ) ) { - self::$cross_sell_link = $data['cross_sell_link']; + $url = esc_url_raw( $data['cross_sell_link'], array( 'http', 'https' ) ); + if ( $url && wp_http_validate_url( $url ) ) { + self::$cross_sell_link = $url; + } } }
421-450: Critical: Multiple redirect implementation issues.This implementation has several critical problems flagged in previous reviews that remain unresolved:
- The
admin_inithook (line 442) is added inside a function called duringadmin_menu. Sinceadmin_initfires beforeadmin_menu, this hook never runs on initial page load, causing a blank page.wp_redirect()(line 445) should bewp_safe_redirect()for external URLs.- No capability check in the redirect callback.
- The external host isn't added to
allowed_redirect_hosts.self::$cross_sell_textis used unsanitized in menu titles (lines 432-433).Apply this diff:
- add_submenu_page( + $menu_text = esc_html( self::$cross_sell_text ); + $hook = add_submenu_page( 'formidable', - self::$cross_sell_text . ' | Formidable', - self::$cross_sell_text, + $menu_text . ' | Formidable', + $menu_text, 'activate_plugins', 'frm-sales-api-cross-sell', - function () { - // There is no page. The redirect logic is handled below, before this callback is triggered. - } + '__return_null' ); - add_action( - 'admin_init', - function () { - if ( 'frm-sales-api-cross-sell' === FrmAppHelper::simple_get( 'page' ) && ! empty( self::$cross_sell_link ) ) { - wp_redirect( self::$cross_sell_link ); - exit; - } - } - ); + add_action( "load-$hook", array( __CLASS__, 'redirect_cross_sell' ) );Add this new method:
public static function redirect_cross_sell() { if ( ! current_user_can( 'activate_plugins' ) ) { wp_die( esc_html__( 'You do not have sufficient permissions to access this page.', 'formidable' ) ); } if ( empty( self::$cross_sell_link ) ) { wp_die( esc_html__( 'Nothing to see here.', 'formidable' ) ); } $host = wp_parse_url( self::$cross_sell_link, PHP_URL_HOST ); if ( $host ) { add_filter( 'allowed_redirect_hosts', static function( $hosts ) use ( $host ) { $hosts[] = $host; return array_unique( $hosts ); } ); } wp_safe_redirect( self::$cross_sell_link ); exit; }
🧹 Nitpick comments (1)
classes/models/FrmSalesApi.php (1)
32-44: Update version placeholder in @SInCE tags.Replace "x.x" with the actual release version before merging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/models/FrmSalesApi.php(3 hunks)tests/cypress/e2e/Forms/formPageDataValidation.cy.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
classes/models/FrmSalesApi.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4617)simple_get(703-712)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (1)
tests/cypress/e2e/Forms/formPageDataValidation.cy.js (1)
33-33: LGTM! Test coverage extended for cross-sell heading.The new valid H1 heading appropriately extends the test to accommodate the cross-sell feature's landing page.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
classes/models/FrmSalesApi.php (1)
421-454: Critical: admin_init hook runs too early; use load-$hook and wp_safe_redirect.The capability check and exit are good improvements, but two critical issues remain:
- Hook timing:
admin_initfires on every admin page load before the menu is registered, so the redirect won't work reliably. Useload-$hookto run only when the submenu page loads.- Unsafe redirect:
wp_redirect()to external URLs requires adding the host toallowed_redirect_hosts. Usewp_safe_redirect()instead.Apply this diff:
- add_submenu_page( + $hook = add_submenu_page( 'formidable', esc_html( self::$cross_sell_text ) . ' | Formidable', esc_html( self::$cross_sell_text ), 'activate_plugins', 'frm-sales-api-cross-sell', - function () { - // There is no page. The redirect logic is handled below, before this callback is triggered. - } + '__return_null' ); add_action( - 'admin_init', + "load-$hook", function () { if ( ! current_user_can( 'activate_plugins' ) ) { return; } if ( 'frm-sales-api-cross-sell' === FrmAppHelper::simple_get( 'page' ) && ! empty( self::$cross_sell_link ) ) { + $host = wp_parse_url( self::$cross_sell_link, PHP_URL_HOST ); + if ( $host ) { + add_filter( 'allowed_redirect_hosts', static function ( $hosts ) use ( $host ) { + $hosts[] = $host; + return array_unique( $hosts ); + } ); + } + - wp_redirect( self::$cross_sell_link ); + wp_safe_redirect( self::$cross_sell_link ); exit; } } );
🧹 Nitpick comments (2)
classes/models/FrmSalesApi.php (2)
32-44: Update @SInCE version placeholders.Replace
x.xwith the actual version number for these new properties.
94-108: Consider additional URL validation.The basic sanitization with
sanitize_text_field()andesc_url_raw()is good. For added security, consider validating the URL withwp_http_validate_url()and optionally restricting allowed hosts.Example enhancement:
private function set_cross_sale( $data ) { if ( ! empty( $data['cross_sell_text'] ) ) { self::$cross_sell_text = sanitize_text_field( $data['cross_sell_text'] ); } if ( ! empty( $data['cross_sell_link'] ) ) { - self::$cross_sell_link = esc_url_raw( $data['cross_sell_link'] ); + $url = esc_url_raw( $data['cross_sell_link'], array( 'http', 'https' ) ); + if ( $url && wp_http_validate_url( $url ) ) { + self::$cross_sell_link = $url; + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/models/FrmSalesApi.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
classes/models/FrmSalesApi.php (2)
classes/controllers/FrmAppController.php (1)
menu(11-24)classes/helpers/FrmAppHelper.php (3)
current_user_can(2078-2100)FrmAppHelper(6-4617)simple_get(703-712)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (1)
classes/models/FrmSalesApi.php (1)
87-92: LGTM!The cross-sell data extraction is properly integrated into the sales initialization flow, and the instance method call is correct.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
classes/models/FrmSalesApi.php (1)
421-454: Critical: Redirect timing bug prevents cross-sell from working.The
admin_inithook is added insidemenu(), which is called during theadmin_menuhook. However,admin_initfires beforeadmin_menu, so this hook won't execute on the same request. On first page load, users will see a blank page instead of being redirected.Additionally:
wp_redirect()should bewp_safe_redirect()for external URLs- The external host must be added to
allowed_redirect_hostsApply this diff to fix the redirect timing and security:
- add_submenu_page( + $hook = add_submenu_page( 'formidable', esc_html( self::$cross_sell_text ) . ' | Formidable', esc_html( self::$cross_sell_text ), 'activate_plugins', 'frm-sales-api-cross-sell', - function () { - // There is no page. The redirect logic is handled below, before this callback is triggered. - } + '__return_null' ); add_action( - 'admin_init', - function () { - if ( ! current_user_can( 'activate_plugins' ) ) { - return; - } - - if ( 'frm-sales-api-cross-sell' === FrmAppHelper::simple_get( 'page' ) && ! empty( self::$cross_sell_link ) ) { - wp_redirect( self::$cross_sell_link ); - exit; - } - } + "load-$hook", + array( __CLASS__, 'redirect_cross_sell' ) ); + } + + /** + * Redirect to cross-sell URL. + * + * @since x.x + * @return void + */ + public static function redirect_cross_sell() { + if ( ! current_user_can( 'activate_plugins' ) ) { + wp_die( esc_html__( 'You do not have sufficient permissions to access this page.', 'formidable' ) ); + } + + if ( empty( self::$cross_sell_link ) ) { + wp_die( esc_html__( 'Invalid redirect.', 'formidable' ) ); + } + + $host = wp_parse_url( self::$cross_sell_link, PHP_URL_HOST ); + if ( $host ) { + add_filter( + 'allowed_redirect_hosts', + function ( $hosts ) use ( $host ) { + $hosts[] = $host; + return array_unique( $hosts ); + } + ); + } + + wp_safe_redirect( self::$cross_sell_link ); + exit; - }Based on learnings from past reviews.
🧹 Nitpick comments (1)
classes/models/FrmSalesApi.php (1)
32-44: Update the @SInCE version placeholders.The
@since x.xtags should be replaced with the actual version number before release.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/models/FrmSalesApi.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
classes/models/FrmSalesApi.php (2)
classes/controllers/FrmAppController.php (1)
menu(11-24)classes/helpers/FrmAppHelper.php (3)
current_user_can(2078-2100)FrmAppHelper(6-4617)simple_get(703-712)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (2)
classes/models/FrmSalesApi.php (2)
87-92: Verify that cross-sell should apply to inactive sales.The cross-sell data is set for any sale that has
cross_sell_text, but this happens before thesale_is_active()check inadd_sale(). This means cross-sell data could come from an inactive sale.If cross-sell should only come from active sales, apply this diff:
foreach ( $api as $sale ) { - $this->add_sale( $sale ); - if ( is_array( $sale ) && isset( $sale['cross_sell_text'] ) ) { - $this->set_cross_sale( $sale ); + if ( $this->sale_is_active( $sale ) ) { + $this->set_cross_sale( $sale ); + } } + + $this->add_sale( $sale ); }Alternatively, if any sale can provide cross-sell regardless of active status, the current implementation is correct.
100-108: LGTM! Sanitization is properly applied.The cross-sell data is now sanitized using
sanitize_text_field()for text andesc_url_raw()for the URL, which addresses previous security concerns.Note: Update the
@since x.xplaceholder before release.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
classes/models/FrmSalesApi.php (1)
460-493: Critical: Redirect fires too late and lacks safe-redirect safeguards.The redirect logic has two critical flaws:
Timing issue:
admin_initfires beforeadmin_menu. Sincemenu()is called on theadmin_menuhook, theadmin_initcallback is registered too late and won't execute on the initial page load, resulting in a blank page instead of a redirect.Security: Using
wp_redirect()to an external URL requireswp_safe_redirect()and adding the host toallowed_redirect_hosts.Apply this diff:
- add_submenu_page( + $hook = add_submenu_page( 'formidable', esc_html( self::$cross_sell_text ) . ' | Formidable', esc_html( self::$cross_sell_text ), 'activate_plugins', 'frm-sales-api-cross-sell', - function () { - // There is no page. The redirect logic is handled below, before this callback is triggered. - } + '__return_null' ); add_action( - 'admin_init', - function () { - if ( ! current_user_can( 'activate_plugins' ) ) { - return; - } - - if ( 'frm-sales-api-cross-sell' === FrmAppHelper::simple_get( 'page' ) && ! empty( self::$cross_sell_link ) ) { - wp_redirect( self::$cross_sell_link ); - exit; - } - } + "load-$hook", + array( __CLASS__, 'redirect_cross_sell' ) ); + } + + /** + * Redirect to cross-sell URL when menu item is clicked. + * + * @since x.x + * + * @return void + */ + public static function redirect_cross_sell() { + if ( ! current_user_can( 'activate_plugins' ) ) { + wp_die( esc_html__( 'You do not have sufficient permissions to access this page.', 'formidable' ) ); + } + + if ( empty( self::$cross_sell_link ) ) { + wp_die( esc_html__( 'Invalid redirect URL.', 'formidable' ) ); + } + + $host = wp_parse_url( self::$cross_sell_link, PHP_URL_HOST ); + if ( $host ) { + add_filter( + 'allowed_redirect_hosts', + function ( $hosts ) use ( $host ) { + $hosts[] = $host; + return array_unique( $hosts ); + } + ); + } + + wp_safe_redirect( self::$cross_sell_link ); + exit; } -}This fix:
- Uses
load-$hookso the redirect fires at the correct time (when the submenu page is actually loaded)- Moves redirect logic to a dedicated method for clarity
- Uses
wp_safe_redirect()with proper host allowlisting- Adds proper error handling with
wp_die()for unauthorized access or missing URL
🧹 Nitpick comments (2)
classes/models/FrmSalesApi.php (2)
32-44: Update @SInCE version placeholders.The
@since x.xplaceholders should be replaced with the actual version number before merging.
139-147: Document stub implementation.The method currently returns a hardcoded
0with a TODO. Consider adding a docblock note explaining this is a stub and what the future implementation will do. This will help clarify why the$cross_sell_textparameter is currently unused (which triggers a PHPMD warning).Example:
/** + * Determine which cross-sell option to display. + * Currently stubbed to always return the first option. + * TODO: Implement logic to select based on user preferences or A/B testing. + * * @param array $cross_sell_text * * @return int */ private static function determine_cross_sell_index( $cross_sell_text ) { - // TODO return 0; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/models/FrmSalesApi.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
classes/models/FrmSalesApi.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4617)simple_get(703-712)
🪛 PHPMD (2.15.0)
classes/models/FrmSalesApi.php
144-144: Avoid unused parameters such as '$cross_sell_text'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Run PHP Syntax inspection (8.3)
- GitHub Check: PHP 8 tests in WP trunk
🔇 Additional comments (2)
classes/models/FrmSalesApi.php (2)
88-93: LGTM!The cross-sell detection logic correctly identifies sales with cross-sell data and delegates to the instance method.
119-137: LGTM!The validation properly ensures both cross-sell arrays exist, are non-empty, and have matching lengths.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
classes/models/FrmSalesApi.php (3)
69-70: Restore production API URL before merge.The API endpoint is still pointing to the local development URL, which will be unreachable in production. This must be reverted to the production URL.
Apply this diff:
- return 'https://dev-site.local/wp-json/s11-sales/v1/list/'; - // return 'https://plapi.formidableforms.com/sales/'; + return 'https://plapi.formidableforms.com/sales/';
106-117: Add bounds check before array access.Lines 115-116 access
$cross_sell_text[$index]and$cross_sell_links[$index]without verifying the index is valid. Ifdetermine_cross_sell_index()returns an out-of-bounds index, this triggers undefined array key warnings.Apply this diff:
private function set_cross_sell( $data ) { if ( ! self::cross_sell_is_valid( $data ) ) { return; } $cross_sell_text = $data['cross_sell_text']; $cross_sell_links = $data['cross_sell_link']; $index = self::determine_cross_sell_index( $cross_sell_text ); + if ( ! isset( $cross_sell_text[ $index ] ) || ! isset( $cross_sell_links[ $index ] ) ) { + return; + } + self::$cross_sell_text = sanitize_text_field( $cross_sell_text[ $index ] ); self::$cross_sell_link = esc_url_raw( $cross_sell_links[ $index ] ); }
489-522: Redirect implementation has critical timing and security issues.The redirect logic has multiple problems:
Timing issue:
admin_initfires beforeadmin_menu, so registering the redirect callback insidemenu()means it won't execute until the next page load, causing a blank page on first click.Open redirect risk: Uses
wp_redirect()with an external URL without proper safeguards. Should usewp_safe_redirect()and add the host toallowed_redirect_hosts.Capability alignment: While the capability check exists, using
load-$hookwould be more robust.Apply this diff:
- add_submenu_page( + $hook = add_submenu_page( 'formidable', esc_html( self::$cross_sell_text ) . ' | Formidable', esc_html( self::$cross_sell_text ), 'activate_plugins', 'frm-sales-api-cross-sell', - function () { - // There is no page. The redirect logic is handled below, before this callback is triggered. - } + '__return_null' ); - add_action( - 'admin_init', - function () { - if ( ! current_user_can( 'activate_plugins' ) ) { - return; - } - - if ( 'frm-sales-api-cross-sell' === FrmAppHelper::simple_get( 'page' ) && ! empty( self::$cross_sell_link ) ) { - wp_redirect( self::$cross_sell_link ); - exit; - } - } - ); + add_action( "load-$hook", array( __CLASS__, 'redirect_cross_sell' ) );Add this method to the class:
public static function redirect_cross_sell() { if ( ! current_user_can( 'activate_plugins' ) ) { wp_die( esc_html__( 'You do not have sufficient permissions to access this page.', 'formidable' ) ); } if ( empty( self::$cross_sell_link ) ) { wp_die( esc_html__( 'Nothing to see here.', 'formidable' ) ); } $host = wp_parse_url( self::$cross_sell_link, PHP_URL_HOST ); if ( $host ) { add_filter( 'allowed_redirect_hosts', static function( $hosts ) use ( $host ) { $hosts[] = $host; return array_unique( $hosts ); } ); } wp_safe_redirect( self::$cross_sell_link ); exit; }
🧹 Nitpick comments (1)
classes/models/FrmSalesApi.php (1)
32-44: Update the@sinceversion placeholder.The
@since x.xplaceholder should be replaced with the actual version number before merging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/models/FrmSalesApi.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
classes/models/FrmSalesApi.php (2)
classes/controllers/FrmAppController.php (1)
menu(11-24)classes/helpers/FrmAppHelper.php (3)
current_user_can(2078-2100)FrmAppHelper(6-4617)simple_get(703-712)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Run PHP Syntax inspection (8.3)
🔇 Additional comments (3)
classes/models/FrmSalesApi.php (3)
88-93: LGTM!The cross-sell detection logic correctly identifies sales containing cross-sell data and processes them via instance method call.
119-137: LGTM!The validation logic properly ensures both cross-sell arrays exist, are the correct type, and have matching lengths before proceeding.
139-176: LGTM!The cross-sell rotation logic correctly tracks and rotates through options based on 30-day intervals. The use of persistent options and safe fallback to index 0 ensures reliable behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
classes/models/FrmSalesApi.php (2)
105-116: Add bounds check before array access.Lines 114-115 access
$cross_sell_text[$index]and$cross_sell_links[$index]without verifying$indexis valid. Ifdetermine_cross_sell_index()returns an out-of-bounds index, this triggers undefined array key warnings.Apply this diff:
private function set_cross_sell( $data ) { if ( ! self::cross_sell_is_valid( $data ) ) { return; } $cross_sell_text = $data['cross_sell_text']; $cross_sell_links = $data['cross_sell_link']; $index = self::determine_cross_sell_index( $cross_sell_text ); + if ( ! isset( $cross_sell_text[ $index ] ) || ! isset( $cross_sell_links[ $index ] ) ) { + return; + } + self::$cross_sell_text = sanitize_text_field( $cross_sell_text[ $index ] ); self::$cross_sell_link = esc_url_raw( $cross_sell_links[ $index ] ); }
488-521: Critical: Fix redirect timing, use safe redirect, and scope to page load.The current implementation has multiple critical issues:
- Timing issue:
admin_initfires beforeadmin_menu, so adding the hook insideadmin_menumeans the redirect won't execute on the initial request, resulting in a blank page.- Security risk:
wp_redirect()to an external URL without validation is an open-redirect vulnerability.- Incorrect scope: The redirect checks the page parameter globally in
admin_initrather than being scoped to this specific submenu load.Apply this diff:
public static function menu() { if ( false === self::$sales ) { new self(); } if ( ! self::$cross_sell_text || ! self::$cross_sell_link ) { return; } - add_submenu_page( + $hook = add_submenu_page( 'formidable', esc_html( self::$cross_sell_text ) . ' | Formidable', esc_html( self::$cross_sell_text ), 'activate_plugins', 'frm-sales-api-cross-sell', - function () { - // There is no page. The redirect logic is handled below, before this callback is triggered. - } + '__return_null' ); add_action( - 'admin_init', - function () { + "load-$hook", + function() { if ( ! current_user_can( 'activate_plugins' ) ) { - return; + wp_die( esc_html__( 'You do not have sufficient permissions.', 'formidable' ) ); } - if ( 'frm-sales-api-cross-sell' === FrmAppHelper::simple_get( 'page' ) && ! empty( self::$cross_sell_link ) ) { - wp_redirect( self::$cross_sell_link ); - exit; + if ( empty( self::$cross_sell_link ) ) { + wp_die( esc_html__( 'Nothing to see here.', 'formidable' ) ); } + + $host = wp_parse_url( self::$cross_sell_link, PHP_URL_HOST ); + if ( $host ) { + add_filter( + 'allowed_redirect_hosts', + function( $hosts ) use ( $host ) { + $hosts[] = $host; + return array_unique( $hosts ); + } + ); + } + + wp_safe_redirect( self::$cross_sell_link ); + exit; } ); }This fix:
- Uses
load-$hookto scope the redirect to only when this submenu page is loaded- Uses
wp_safe_redirect()for external URLs- Adds the external host to
allowed_redirect_hosts- Provides proper error messages for permission/data issues
- Removes the unnecessary page parameter check since
load-$hookhandles scoping
🧹 Nitpick comments (1)
classes/models/FrmSalesApi.php (1)
32-44: Update @SInCE version placeholders.The
@since x.xtags should be replaced with the actual version number before merging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/models/FrmSalesApi.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
classes/models/FrmSalesApi.php (2)
classes/controllers/FrmAppController.php (1)
menu(11-24)classes/helpers/FrmAppHelper.php (3)
current_user_can(2078-2100)FrmAppHelper(6-4617)simple_get(703-712)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cypress
🔇 Additional comments (3)
classes/models/FrmSalesApi.php (3)
85-92: LGTM!The cross-sell detection and invocation logic is correct. The instance method call is properly used.
138-175: LGTM!The time-based rotation logic correctly tracks and cycles through cross-sell items every 30 days, with appropriate fallback to index 0 when all options expire.
126-136: Validate arrays are non-empty.
cross_sell_is_valid()checks array types and matching lengths but allows empty arrays. If both arrays are empty,count([]) === count([])returns true, but subsequent index access will fail.Apply this diff:
private function cross_sell_is_valid( $data ) { if ( empty( $data['cross_sell_text'] ) || empty( $data['cross_sell_link'] ) ) { return false; } if ( ! is_array( $data['cross_sell_text'] ) || ! is_array( $data['cross_sell_link'] ) ) { return false; } - return count( $data['cross_sell_link'] ) === count( $data['cross_sell_text'] ); + $text_count = count( $data['cross_sell_text'] ); + return $text_count > 0 && $text_count === count( $data['cross_sell_link'] ); }Likely an incorrect or invalid review comment.
1 participant
No description provided.