feat(frontend): add filterable popup_id query parameter name#1156
feat(frontend): add filterable popup_id query parameter name#1156danieliser merged 9 commits intodevelopfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a centralized request-parameter API (names, values, sanitization) and replaces hardcoded popup ID/CTA query-parameter usage with configurable, filterable parameter names across JS and PHP, updating script localization and CTA link/redirect handling. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Assets as "Site Assets (PHP)"
participant JS as "URL Tracking JS"
participant Server as "CTA Controller (PHP)"
Client->>Assets: Request page
Assets->>Assets: get_param_names()
Assets->>JS: Localize pum_vars.paramNames
Client->>JS: User click / interaction
JS->>JS: Read paramNames.popup_id (or default)
JS->>Client: Generate URL with dynamic param (e.g. ?pid=123&cta=...)
Client->>Server: Request with dynamic params
Server->>Server: get_param_value('popup_id') / get_param_value('cta')
Server->>Server: sanitize_param_by_type(...)
Server->>Client: Redirect / CTA response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🤖 Fix all issues with AI agents
In @classes/Shortcode/CallToAction.php:
- Around line 198-200: The generated URL currently passes null for a missing
popup id which causes add_query_arg to emit a bare query param; change the
ternary in the call to CallToAction::generate_url (the line using
\PopupMaker\get_param_name('popup_id') => $popup_id ? $popup_id : null) to pass
boolean false when $popup_id is empty (e.g., use $popup_id ?: false) so
add_query_arg will omit the parameter, or alternatively remove the key from the
args array before calling generate_url.
🧹 Nitpick comments (1)
includes/namespaced/utils.php (1)
203-215: Consider sanitizing the filtered parameter name.The filter allows site admins to customize the parameter name, but if a malicious or poorly written filter returns a value with special characters (e.g.,
=,&, spaces), it could break URL construction or cause unexpected behavior.♻️ Proposed sanitization
function get_param_name( $key ) { static $cache = []; if ( ! isset( $cache[ $key ] ) ) { $defaults = [ 'popup_id' => 'pid' ]; - $cache[ $key ] = apply_filters( + $filtered = apply_filters( "popup_maker/param_name/{$key}", $defaults[ $key ] ?? $key ); + // Sanitize to ensure valid URL parameter name. + $cache[ $key ] = sanitize_key( $filtered ); } return $cache[ $key ]; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
assets/js/src/site/plugins/pum-url-tracking.jsclasses/Controllers/CallToActions.phpclasses/Shortcode/CallToAction.phpclasses/Site/Assets.phpincludes/namespaced/utils.php
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use PUM.hooks system for frontend extensibility in JavaScript/TypeScript
Files:
assets/js/src/site/plugins/pum-url-tracking.js
**/*.{js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/pm-best-practices.mdc)
**/*.{js,jsx}: Use thePUM.hooksevent manager for JavaScript actions and filters on any page where Popup Maker scripts are loaded
Use JavaScript hooks likepum.popup.beforeOpen,pum.popup.afterOpen,pum.popup.beforeClose,pum.popup.afterClose, andpum.integration.form.successfor extending Popup Maker functionality
Use the globalPUMobject to interact with popups via JavaScript methods likePUM.open(),PUM.close(), andPUM.getSettings()
DO NOT rely on the older$.fn.popmakemethods directly in JavaScript if aPUM.*equivalent exists, asPUM.*provides a more stable, public-facing API
Fire thepum.integration.form.successJavaScript action after a successful form submission to integrate a new form plugin with Popup Maker's Form Submission trigger
Files:
assets/js/src/site/plugins/pum-url-tracking.js
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/pm-js-api-reference.mdc)
**/*.{js,ts,tsx,jsx}: Use the globalwindow.PUMobject as the primary way to interact with popups on the frontend. Available methods include:PUM.open(),PUM.close(),PUM.getPopup(),PUM.getSettings(),PUM.getSetting(),PUM.getCookie(),PUM.setCookie(),PUM.clearCookie(),PUM.clearCookies(), andPUM.checkConditions()
Use the jQuerypopmake()method directly on popup jQuery objects as an alternative to the globalPUMobject. Available methods include:popmake('open', callback),popmake('close', callback),popmake('getSettings'), andpopmake('getContainer')
Use Popup Maker's hooks system (PUM.hooks) to extend frontend functionality in an event-driven manner. UsePUM.hooks.addAction()for actions andPUM.hooks.addFilter()for filters. Key actions include:pum.popup.beforeOpen,pum.popup.afterOpen,pum.popup.beforeClose,pum.popup.afterClose, andpum.integration.form.success
For form integrations that submit via AJAX, listen for the form's successful submission event and callwindow.PUM.integrations.formSubmission()with the form's jQuery object and an object containingformProviderandformId
Files:
assets/js/src/site/plugins/pum-url-tracking.js
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/pm-js-api-reference.mdc)
To extend the Call to Action editor in
@popup-maker/cta-editor, use the Registry API methods:registerEditorTab()to add new tabs,registerEditorHeaderAction()to add header buttons, andregisterEditorHeaderOption()to add menu items
Files:
assets/js/src/site/plugins/pum-url-tracking.js
**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.php: Usepum_prefixes for all public functions and hooks in PHP
Use custom\PopupMaker\{ExtensionName}\namespace for classes/functions/hooks in PHP
Follow WordPress coding standards enforced by PHPCS
Use dependency injection over global access in PHP
Inline comments in PHP must end in full-stops, exclamation marks, or question marks (Squiz.Commenting.InlineComment.InvalidEndChar)
Never use strict PHP types on methods hooked to third-party actions/filters; use defensive validation instead
All PHP code must be compatible with PHP 7.4+; forbidden features include union types (e.g.,string|int),mixedtype, named arguments, and match expressions
Use%ifor the table name in $wpdb prepared queries
Check for Popup Maker existence before calling functions
Use defensive validation in hook callbacks for graceful handling of third-party plugin parameter changes
**/*.php: Use thepum_orPUM_prefix for your functions, classes, and hooks to maintain consistency with the core plugin's standards
DO NOT use the olderpopmake_prefix. This is for legacy code being phased out. Usepum_equivalent instead
Use thePopupMaker\namespace for new, modern integrations where appropriate, following the structure seen in theclasses/directory
Check for the existence of Popup Maker functions and classes before calling them usingfunction_exists()to prevent fatal errors if the core plugin is not active
DO NOT use deprecated functions or classes in Popup Maker integrations. Refer to the deprecation guide for alternatives
Use thepum_enqueue_scriptandpum_enqueue_stylefunctions for your assets to allow inclusion in Popup Maker's Asset Cache for optimized performance
DO NOT enqueue your own copies of libraries that Popup Maker already includes, such as jQuery or jQuery UI. Use the registered handles as dependencies
Use thepum_registered_triggers,pum_registered_cookies, andpum_registered_conditionsfilters to register custom items so they appea...
Files:
classes/Controllers/CallToActions.phpclasses/Site/Assets.phpclasses/Shortcode/CallToAction.phpincludes/namespaced/utils.php
🧠 Learnings (22)
📓 Common learnings
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `$popup->get_settings()` and `$popup->get_setting('key', 'default')` methods on `PUM_Model_Popup` objects to access popup settings
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `pum_registered_triggers` filter to register new popup triggers with custom configuration
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `$popup->get_triggers()`, `$popup->get_cookies()`, and `$popup->get_conditions()` methods to retrieve popup metadata
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `pum_popup_data_attr` filter to modify the `data-popmake` attribute array before JSON conversion
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-best-practices.mdc:0-0
Timestamp: 2025-12-20T07:31:54.054Z
Learning: Applies to **/*.php : Use `pum_popup_data_attr` filter instead of the deprecated `popmake_get_the_popup_data_attr` filter
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `pum_get_options` and `pum_get_option` filters to modify Popup Maker's global settings before retrieval
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `pum_get_option()` and `pum_update_option()` functions for accessing and modifying Popup Maker settings
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-best-practices.mdc:0-0
Timestamp: 2025-12-20T07:31:54.054Z
Learning: Applies to **/*.{js,jsx} : Use JavaScript hooks like `pum.popup.beforeOpen`, `pum.popup.afterOpen`, `pum.popup.beforeClose`, `pum.popup.afterClose`, and `pum.integration.form.success` for extending Popup Maker functionality
📚 Learning: 2025-12-20T07:31:54.054Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-best-practices.mdc:0-0
Timestamp: 2025-12-20T07:31:54.054Z
Learning: Applies to **/*.{js,jsx} : Use the global `PUM` object to interact with popups via JavaScript methods like `PUM.open()`, `PUM.close()`, and `PUM.getSettings()`
Applied to files:
assets/js/src/site/plugins/pum-url-tracking.js
📚 Learning: 2025-12-20T07:32:04.308Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-js-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:04.308Z
Learning: Applies to **/*.{js,ts,tsx,jsx} : Use the global `window.PUM` object as the primary way to interact with popups on the frontend. Available methods include: `PUM.open()`, `PUM.close()`, `PUM.getPopup()`, `PUM.getSettings()`, `PUM.getSetting()`, `PUM.getCookie()`, `PUM.setCookie()`, `PUM.clearCookie()`, `PUM.clearCookies()`, and `PUM.checkConditions()`
Applied to files:
assets/js/src/site/plugins/pum-url-tracking.js
📚 Learning: 2025-12-20T07:31:54.054Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-best-practices.mdc:0-0
Timestamp: 2025-12-20T07:31:54.054Z
Learning: Applies to **/*.{js,jsx} : DO NOT rely on the older `$.fn.popmake` methods directly in JavaScript if a `PUM.*` equivalent exists, as `PUM.*` provides a more stable, public-facing API
Applied to files:
assets/js/src/site/plugins/pum-url-tracking.js
📚 Learning: 2025-12-20T07:31:54.054Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-best-practices.mdc:0-0
Timestamp: 2025-12-20T07:31:54.054Z
Learning: Applies to **/*.php : DO NOT use deprecated functions or classes in Popup Maker integrations. Refer to the deprecation guide for alternatives
Applied to files:
classes/Controllers/CallToActions.php
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `pum_get_popup()` function to retrieve a single popup object instead of direct post queries
Applied to files:
classes/Controllers/CallToActions.php
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `$popup->get_triggers()`, `$popup->get_cookies()`, and `$popup->get_conditions()` methods to retrieve popup metadata
Applied to files:
classes/Controllers/CallToActions.phpclasses/Site/Assets.phpclasses/Shortcode/CallToAction.php
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `$popup->increase_event_count('open|conversion')` to increment popup analytics counters
Applied to files:
classes/Controllers/CallToActions.php
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use template functions `pum_popup_ID()`, `pum_popup_title()`, `pum_popup_content()`, `pum_popup_classes()`, `pum_popup_data_attr()`, `pum_show_close_button()`, and `pum_popup_close_text()` within popup templates
Applied to files:
classes/Controllers/CallToActions.phpclasses/Shortcode/CallToAction.php
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `pum_popup_settings` filter to modify the settings array for a specific popup
Applied to files:
classes/Site/Assets.php
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `pum_popup_data_attr` filter to modify the `data-popmake` attribute array before JSON conversion
Applied to files:
classes/Site/Assets.php
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `pum_get_options` and `pum_get_option` filters to modify Popup Maker's global settings before retrieval
Applied to files:
classes/Site/Assets.php
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `pum_registered_cookies` filter to register new popup cookies with custom configuration
Applied to files:
classes/Site/Assets.php
📚 Learning: 2025-12-20T07:31:54.054Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-best-practices.mdc:0-0
Timestamp: 2025-12-20T07:31:54.054Z
Learning: Applies to **/*.php : Use the `pum_registered_triggers`, `pum_registered_cookies`, and `pum_registered_conditions` filters to register custom items so they appear correctly in the Popup Editor UI
Applied to files:
classes/Site/Assets.php
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `pum_get_option()` and `pum_update_option()` functions for accessing and modifying Popup Maker settings
Applied to files:
classes/Site/Assets.php
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `$popup->get_settings()` and `$popup->get_setting('key', 'default')` methods on `PUM_Model_Popup` objects to access popup settings
Applied to files:
classes/Site/Assets.php
📚 Learning: 2025-12-20T07:31:54.054Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-best-practices.mdc:0-0
Timestamp: 2025-12-20T07:31:54.054Z
Learning: Applies to **/*.php : Use the `pum_popup_settings_fields` and `pum_theme_settings_fields` filters to add new settings tabs, sections, and fields to the Popup and Theme editors
Applied to files:
classes/Site/Assets.php
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `pum_registered_triggers` filter to register new popup triggers with custom configuration
Applied to files:
classes/Site/Assets.php
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `pum_popup_classes` and element-specific class filters (`pum_popup_container_classes`, `pum_popup_overlay_classes`, etc.) to modify popup CSS classes
Applied to files:
classes/Shortcode/CallToAction.php
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `pum_is_popup()` to validate that a variable is a valid `PUM_Model_Popup` object
Applied to files:
classes/Shortcode/CallToAction.php
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `pum_get_popups()` or `pum_get_all_popups()` functions to retrieve multiple popup objects based on arguments
Applied to files:
classes/Shortcode/CallToAction.php
📚 Learning: 2025-12-20T07:31:54.054Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-best-practices.mdc:0-0
Timestamp: 2025-12-20T07:31:54.054Z
Learning: Applies to **/*.php : Use the `PopupMaker\` namespace for new, modern integrations where appropriate, following the structure seen in the `classes/` directory
Applied to files:
includes/namespaced/utils.php
🧬 Code graph analysis (3)
classes/Controllers/CallToActions.php (1)
includes/namespaced/utils.php (1)
get_param_name(203-215)
classes/Site/Assets.php (1)
includes/namespaced/utils.php (1)
get_param_names(224-226)
classes/Shortcode/CallToAction.php (2)
classes/Models/CallToAction.php (1)
generate_url(221-227)includes/namespaced/utils.php (1)
get_param_name(203-215)
⏰ 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: commitlint
🔇 Additional comments (5)
includes/namespaced/utils.php (1)
224-225: LGTM!Simple wrapper that correctly delegates to
get_param_name(). The implementation is clean and extensible for future parameter types.classes/Site/Assets.php (1)
270-270: LGTM!Correctly exposes the parameter names to JavaScript via the existing
pum_varslocalization pattern. The fully qualified namespace call ensures proper resolution.classes/Controllers/CallToActions.php (1)
37-42: LGTM!The implementation correctly:
- Retrieves the dynamic parameter name once and reuses it
- Includes it in the
popup_maker/cta_valid_url_argsfilter for proper URL cleanup- Safely reads and sanitizes the value from
$_GETusingabsint()assets/js/src/site/plugins/pum-url-tracking.js (2)
64-65: LGTM!The dynamic key assignment pattern is correct and maintains compatibility with the existing
PUM.hooks.applyFilterssystem that allows extensions to modify URL parameters.
59-65: No browser compatibility concern—optional chaining is transpiled during build.The code uses
@wordpress/babel-preset-defaultwith the webpack build pipeline (wp-scripts build), which automatically transpiles optional chaining syntax to older JavaScript. The transpilation includes allassets/js/src/**/*.jsfiles, ensuring compatibility with the browsers specified in the project's.browserslistrcconfiguration.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@includes/namespaced/utils.php`:
- Around line 282-292: get_post_param_value currently assumes $_POST[$key] is a
scalar and can break or bypass validation if an array is submitted; add the same
array-input guard used for GET params by checking if isset($_POST[$key]) and
is_array($_POST[$key]) and returning $fallback in that case before calling
wp_unslash and sanitize_param_by_type; update the function get_post_param_value
to return $fallback when $_POST[$key] is an array so sanitize_param_by_type
continues to receive only scalar values.
- Around line 253-265: get_param_value currently trusts $_GET[$param_name] may
be a string but PHP allows array inputs which will break downstream functions
like sanitize_param_by_type; update get_param_value to early-guard against array
values for $_GET[$param_name] (e.g., treat is_array($_GET[$param_name]) as
invalid and return $fallback or otherwise normalize to a safe string) before
calling wp_unslash and sanitize_param_by_type; reference get_param_value,
get_param_name and sanitize_param_by_type when locating where to add the check.
🧹 Nitpick comments (1)
classes/Controllers/CallToActions.php (1)
137-145: Consider defensive type checks for hook callback parameters.The
track_cta_conversionmethod receives$argsfrom thepopup_maker/cta_conversionaction. Per coding guidelines, hook callbacks should use defensive validation since third-party plugins might alter the filter chain. Consider validating$args['popup_id']and$args['notrack']types.♻️ Optional defensive fix
public function track_cta_conversion( $call_to_action, $args ) { - $popup_id = $args['popup_id']; - $notrack = $args['notrack']; + $popup_id = isset( $args['popup_id'] ) ? absint( $args['popup_id'] ) : 0; + $notrack = isset( $args['notrack'] ) ? (bool) $args['notrack'] : false; if ( ! $notrack && $popup_id && $popup_id > 0 ) { // Triggers pum_analytics_event / pum_analytics_conversion actions. \pum_track_conversion_event( $popup_id, $args ); } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/Controllers/CallToActions.phpincludes/namespaced/utils.php
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.php: Usepum_prefixes for all public functions and hooks in PHP
Use custom\PopupMaker\{ExtensionName}\namespace for classes/functions/hooks in PHP
Follow WordPress coding standards enforced by PHPCS
Use dependency injection over global access in PHP
Inline comments in PHP must end in full-stops, exclamation marks, or question marks (Squiz.Commenting.InlineComment.InvalidEndChar)
Never use strict PHP types on methods hooked to third-party actions/filters; use defensive validation instead
All PHP code must be compatible with PHP 7.4+; forbidden features include union types (e.g.,string|int),mixedtype, named arguments, and match expressions
Use%ifor the table name in $wpdb prepared queries
Check for Popup Maker existence before calling functions
Use defensive validation in hook callbacks for graceful handling of third-party plugin parameter changes
**/*.php: Use thepum_orPUM_prefix for your functions, classes, and hooks to maintain consistency with the core plugin's standards
DO NOT use the olderpopmake_prefix. This is for legacy code being phased out. Usepum_equivalent instead
Use thePopupMaker\namespace for new, modern integrations where appropriate, following the structure seen in theclasses/directory
Check for the existence of Popup Maker functions and classes before calling them usingfunction_exists()to prevent fatal errors if the core plugin is not active
DO NOT use deprecated functions or classes in Popup Maker integrations. Refer to the deprecation guide for alternatives
Use thepum_enqueue_scriptandpum_enqueue_stylefunctions for your assets to allow inclusion in Popup Maker's Asset Cache for optimized performance
DO NOT enqueue your own copies of libraries that Popup Maker already includes, such as jQuery or jQuery UI. Use the registered handles as dependencies
Use thepum_registered_triggers,pum_registered_cookies, andpum_registered_conditionsfilters to register custom items so they appea...
Files:
classes/Controllers/CallToActions.phpincludes/namespaced/utils.php
🧠 Learnings (3)
📚 Learning: 2025-12-20T07:31:54.054Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-best-practices.mdc:0-0
Timestamp: 2025-12-20T07:31:54.054Z
Learning: Applies to **/*.php : DO NOT use deprecated functions or classes in Popup Maker integrations. Refer to the deprecation guide for alternatives
Applied to files:
classes/Controllers/CallToActions.php
📚 Learning: 2025-12-20T07:31:54.054Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-best-practices.mdc:0-0
Timestamp: 2025-12-20T07:31:54.054Z
Learning: Applies to **/*.php : Use the `PopupMaker\` namespace for new, modern integrations where appropriate, following the structure seen in the `classes/` directory
Applied to files:
classes/Controllers/CallToActions.php
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `$popup->increase_event_count('open|conversion')` to increment popup analytics counters
Applied to files:
classes/Controllers/CallToActions.php
🧬 Code graph analysis (1)
classes/Controllers/CallToActions.php (1)
includes/namespaced/utils.php (2)
get_param_name(206-220)get_param_value(253-265)
⏰ 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: commitlint
🔇 Additional comments (5)
includes/namespaced/utils.php (4)
160-168: LGTM!The multi-line reformatting of
wp_parse_argsimproves readability with no functional change.
206-220: LGTM!Clean implementation with static caching for performance and proper filter hook naming convention (
popup_maker/param_name/{$key}). Thesanitize_key()wrapper ensures safe output regardless of filter values.
229-235: LGTM!Clean API surface for exposing all parameter names to JavaScript localization.
307-360: LGTM!Well-structured sanitization with appropriate WordPress functions and helpful debug logging for development. The use of
absint()for integers aligns with WordPress conventions for entity IDs.classes/Controllers/CallToActions.php (1)
37-42: LGTM!Clean migration to the new utility functions with appropriate type specifications. The dynamic
$pid_paramin the filter ensuresremove_query_arg()strips the correct parameter regardless of customization.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@includes/namespaced/utils.php`:
- Around line 206-225: get_param_name currently builds a default/override value
for the parameter but never applies the filter described in the PR; update
get_param_name to run the value through
apply_filters("popup_maker/param_name/{$key}", $default) (after reading the
stored override from get_option('pum_pid_param_override') so the filter can
alter the final name) then sanitize_key() and cache the result; reference
function get_param_name and option pum_pid_param_override when making the
change.
♻️ Duplicate comments (2)
includes/namespaced/utils.php (2)
258-270: Guard against array input to prevent type errors.This issue was already flagged in a previous review. PHP allows array query parameters (e.g.,
?pid[]=malicious), which would pass through tosanitize_param_by_type()where functions likesanitize_key()expect string input.
287-297: Same array input guard needed for POST parameters.This issue was already flagged in a previous review. POST parameters can also be arrays and need the same protection.
🧹 Nitpick comments (2)
includes/namespaced/utils.php (2)
413-462: File scanning is limited and may miss pid usage in subdirectories.The scan only checks main plugin files, but many plugins define their query parameter handling in included files or class files within subdirectories. This could lead to false negatives where conflicts exist but aren't detected.
Consider documenting this limitation or expanding the known plugins list as conflicts are reported, rather than relying solely on pattern scanning.
487-505: Silent auto-fix may surprise users and complicate debugging.When a conflict is detected, the option is silently set without any admin notice or log entry. Users investigating URL tracking issues won't know why their parameter changed from
pidtopum_popup_id.Consider adding a debug log entry or storing metadata about when/why the override was applied.
💡 Proposed enhancement
// Auto-apply override if conflict found. if ( $has_conflict ) { update_option( 'pum_pid_param_override', 'pum_popup_id', false ); + + // Log for debugging purposes. + if ( defined( 'WP_DEBUG' ) && WP_DEBUG ) { + // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_error_log + error_log( 'Popup Maker: Auto-applied pid parameter override to "pum_popup_id" due to detected conflict.' ); + } }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@includes/namespaced/utils.php`:
- Around line 425-473: scan_plugins_for_pid_usage currently only reads
site-specific active plugins via $active_plugins = get_option('active_plugins')
and misses network-activated plugins on multisite; update the function to also
fetch network-wide plugins using get_site_option('active_sitewide_plugins') (or
get_network_option/get_site_option as appropriate), merge them with
$active_plugins (ensuring keys/values are normalized and duplicates removed),
then iterate the combined list when building $plugin_path and scanning file
contents; keep the existing Popup Maker skip (strpos check), file_exists check,
pattern matching loop, and transient caching behavior.
♻️ Duplicate comments (1)
includes/namespaced/utils.php (1)
206-222: Parameter name isn’t actually filterable yet.
The filter described in the PR isn’t applied, so consumers can’t override viaadd_filter.🔧 Proposed fix
- $cache[ $key ] = sanitize_key( $default ); + /** + * Filter the query parameter name. + * + * `@since` 1.22.0 + * + * `@param` string $param_name The parameter name. + * `@param` string $key The parameter key. + */ + $cache[ $key ] = sanitize_key( apply_filters( "popup_maker/param_name/{$key}", $default, $key ) );
🧹 Nitpick comments (1)
includes/namespaced/utils.php (1)
214-218: Prefer pum_get_option()/pum_update_option() for plugin settings.
Keeps option access consistent with Popup Maker’s settings layer.♻️ Proposed refactor
- $override = get_option( 'pum_pid_param_override' ); + $override = \pum_get_option( 'pum_pid_param_override' ); if ( $override ) { $default = $override; }- if ( get_option( 'pum_pid_param_override' ) ) { + if ( \pum_get_option( 'pum_pid_param_override' ) ) { return; } ... - update_option( 'pum_pid_param_override', 'pum_popup_id', false ); + \pum_update_option( 'pum_pid_param_override', 'pum_popup_id', false ); }Based on learnings, prefer
pum_get_option()/pum_update_option()for Popup Maker settings.Also applies to: 501-516
| function scan_plugins_for_pid_usage() { | ||
| // Check cache first. | ||
| $cached = get_transient( 'pum_pid_scan_result' ); | ||
| if ( false !== $cached ) { | ||
| return (bool) $cached; | ||
| } | ||
|
|
||
| $found = false; | ||
| $active_plugins = get_option( 'active_plugins', [] ); | ||
|
|
||
| // Patterns that indicate pid parameter usage. | ||
| $patterns = [ | ||
| '/\$_GET\s*\[\s*[\'"]pid[\'"]\s*\]/', | ||
| '/\$_REQUEST\s*\[\s*[\'"]pid[\'"]\s*\]/', | ||
| '/\$_POST\s*\[\s*[\'"]pid[\'"]\s*\]/', | ||
| '/get_query_var\s*\(\s*[\'"]pid[\'"]\s*\)/', | ||
| ]; | ||
|
|
||
| foreach ( $active_plugins as $plugin_file ) { | ||
| // Skip Popup Maker itself. | ||
| if ( false !== strpos( $plugin_file, 'popup-maker' ) ) { | ||
| continue; | ||
| } | ||
|
|
||
| $plugin_path = WP_PLUGIN_DIR . '/' . $plugin_file; | ||
|
|
||
| // Only check the main plugin file (lightweight). | ||
| if ( ! file_exists( $plugin_path ) ) { | ||
| continue; | ||
| } | ||
|
|
||
| // phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents -- Reading plugin files for pattern matching. | ||
| $content = file_get_contents( $plugin_path ); | ||
| if ( false === $content || '' === $content ) { | ||
| continue; | ||
| } | ||
|
|
||
| foreach ( $patterns as $pattern ) { | ||
| if ( preg_match( $pattern, $content ) ) { | ||
| $found = true; | ||
| break 2; // Exit both loops. | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Cache for 1 week (invalidated on plugin changes). | ||
| set_transient( 'pum_pid_scan_result', $found ? 1 : 0, WEEK_IN_SECONDS ); | ||
|
|
||
| return $found; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if this function is actually used and in what context
rg -A 5 -B 5 "scan_plugins_for_pid_usage" --type phpRepository: PopupMaker/Popup-Maker
Length of output: 1338
🏁 Script executed:
# Search for other multisite handling in the codebase
rg "is_multisite|get_site_option|active_sitewide_plugins" --type phpRepository: PopupMaker/Popup-Maker
Length of output: 2170
🏁 Script executed:
# Check if there are tests or comments about multisite
rg -i "multisite|network" --type php | head -20Repository: PopupMaker/Popup-Maker
Length of output: 1894
🏁 Script executed:
# Verify the exact pattern in Extension/Updater.php for reference
sed -n '1,50p' classes/Extension/Updater.php | grep -A 3 "active_sitewide_plugins"Repository: PopupMaker/Popup-Maker
Length of output: 48
🏁 Script executed:
# Check context around the PID conflict check function
rg -B 10 "scan_plugins_for_pid_usage\(\)" includes/namespaced/utils.php | head -30Repository: PopupMaker/Popup-Maker
Length of output: 686
🏁 Script executed:
# Verify the exact structure of active_sitewide_plugins in WordPress
# Check how it's used in Extension/Updater.php
sed -n '1,100p' classes/Extension/Updater.php | grep -A 5 "active_sitewide_plugins"Repository: PopupMaker/Popup-Maker
Length of output: 48
🏁 Script executed:
# Find and view the relevant section in Extension/Updater.php
rg -A 5 "active_sitewide_plugins" classes/Extension/Updater.phpRepository: PopupMaker/Popup-Maker
Length of output: 237
🏁 Script executed:
# Also check if there's documentation about how WordPress handles this
# Look at the full context where get_site_option is used
rg -B 3 -A 3 "get_site_option.*active_sitewide_plugins" --type phpRepository: PopupMaker/Popup-Maker
Length of output: 555
🏁 Script executed:
# Verify the exact line numbers and context in the file
sed -n '420,475p' includes/namespaced/utils.phpRepository: PopupMaker/Popup-Maker
Length of output: 1506
Missing network-activated plugins in multisite PID conflict detection.
In WordPress multisite, the function only scans site-specific plugins via get_option('active_plugins') and ignores network-wide plugins stored in get_site_option('active_sitewide_plugins'), causing false negatives when a network-activated plugin uses the 'pid' parameter.
🔧 Proposed fix
- $active_plugins = get_option( 'active_plugins', [] );
+ $active_plugins = get_option( 'active_plugins', [] );
+ if ( is_multisite() ) {
+ $network_active = array_keys( get_site_option( 'active_sitewide_plugins', [] ) );
+ $active_plugins = array_unique( array_merge( $active_plugins, $network_active ) );
+ }🤖 Prompt for AI Agents
In `@includes/namespaced/utils.php` around lines 425 - 473,
scan_plugins_for_pid_usage currently only reads site-specific active plugins via
$active_plugins = get_option('active_plugins') and misses network-activated
plugins on multisite; update the function to also fetch network-wide plugins
using get_site_option('active_sitewide_plugins') (or
get_network_option/get_site_option as appropriate), merge them with
$active_plugins (ensuring keys/values are normalized and duplicates removed),
then iterate the combined list when building $plugin_path and scanning file
contents; keep the existing Popup Maker skip (strpos check), file_exists check,
pattern matching loop, and transient caching behavior.
s-acv2
force-pushed
the
feature/filterable-pid-param
branch
from
7072fdb to
c812f32
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Add ability to customize the 'pid' URL query parameter used for
popup link tracking. This resolves conflicts with other plugins
(like WP User Frontend) that also use 'pid' as a query parameter.
The parameter name can be changed via the popup_maker/param_name/popup_id
filter. For example:
add_filter('popup_maker/param_name/popup_id', fn() => 'pum_popup_id');
Changes:
- Add get_param_name() and get_param_names() helpers in utils.php
- Update CallToActions controller to use dynamic param name
- Update CTA shortcode URL generation
- Expose paramNames in pum_vars for JavaScript
- Update pum-url-tracking.js to read param name from config
- Use false instead of null for missing popup_id to prevent bare query params - Add sanitize_key() to filtered param name for security
Add get_param_value() and get_post_param_value() helpers that integrate with the filterable parameter name system. Provides type-safe sanitization and eliminates boilerplate param retrieval code. Features: - get_param_value() for GET params with filterable names - get_post_param_value() for POST params (non-filterable) - sanitize_param_by_type() reusable sanitizer with 6 types - WP_DEBUG logging for type conversion failures - Support for string, int, bool, key, email, url types Updates CallToActions to use new helpers for cleaner code. Extends get_param_names() to include cta and notrack.
Extracts first element when array is passed to scalar type sanitization, preventing unexpected behavior from URL params like ?param[]=value.
Base/CallToAction::safe_redirect() and CallToAction/Link::action_handler() both used hardcoded 'pid' in remove_query_arg() calls via the cta_valid_url_args filter. When the popup_id param is filtered to a different name, these would fail to strip the actual param from the URL.
danieliser
force-pushed
the
feature/filterable-pid-param
branch
from
b152fe9 to
032eb34
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
❌ Commit Validation ErrorYour commits don't pass validation. Common issues: Format RequirementsCommits must follow conventional commit format: Valid types: feat, fix, improve, perf, refactor, docs, style, test, build, ci, chore, revert Examples:
|
- Use get_param_name('cta') in remove_query_arg calls instead of
hardcoded 'cta' string.
- Strip debug logging from sanitize_param_by_type.
- Add 'array' type support to sanitize_param_by_type.
- Simplify array-input guard to work with all non-array types.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
includes/namespaced/utils.php (2)
332-333: Nested array values would fail sanitization.If
$valuecontains nested arrays (e.g.,?param[key][subkey]=val),array_map('sanitize_text_field', $value)will pass arrays tosanitize_text_field(), which expects strings.For URL parameters this is unlikely, but consider using
array_walk_recursiveor rejecting nested arrays for robustness.♻️ Optional defensive fix
case 'array': - return is_array( $value ) ? array_map( 'sanitize_text_field', $value ) : []; + if ( ! is_array( $value ) ) { + return []; + } + // Flatten to single-level and sanitize. + return array_map( + function ( $v ) { + return is_scalar( $v ) ? sanitize_text_field( (string) $v ) : ''; + }, + $value + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@includes/namespaced/utils.php` around lines 332 - 333, The current case 'array' branch uses array_map('sanitize_text_field', $value) which will break when elements are arrays; update the branch to either reject nested arrays or recursively sanitize all scalar elements (use array_walk_recursive to apply sanitize_text_field to every scalar in $value) and return the preserved array structure; reference the case 'array' switch branch and the sanitize_text_field call when making the change.
316-318: Consider supporting negative integers if needed.Using
absint()restricts the'int'type to non-negative values only. If any future use case requires negative integers (e.g., offsets, relative positions), this will silently convert them to their absolute value.If only non-negative integers are expected for URL parameters, this is fine. Otherwise, consider using
intval()with an optional unsigned flag or documenting this behavior explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@includes/namespaced/utils.php` around lines 316 - 318, The current 'int' branch in the switch uses absint($value) which forces non-negative values; update the 'int' case in the function containing the switch to use intval($value) (or optionally accept an unsigned flag and call absint only when unsigned is requested) so negative integers are preserved, and update any docblock/comments for that function to document the chosen behavior; locate the 'int' case and the use of absint() to make the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@includes/namespaced/utils.php`:
- Around line 332-333: The current case 'array' branch uses
array_map('sanitize_text_field', $value) which will break when elements are
arrays; update the branch to either reject nested arrays or recursively sanitize
all scalar elements (use array_walk_recursive to apply sanitize_text_field to
every scalar in $value) and return the preserved array structure; reference the
case 'array' switch branch and the sanitize_text_field call when making the
change.
- Around line 316-318: The current 'int' branch in the switch uses
absint($value) which forces non-negative values; update the 'int' case in the
function containing the switch to use intval($value) (or optionally accept an
unsigned flag and call absint only when unsigned is requested) so negative
integers are preserved, and update any docblock/comments for that function to
document the chosen behavior; locate the 'int' case and the use of absint() to
make the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7da224a1-533a-4cab-a124-205f23332f75
📒 Files selected for processing (7)
assets/js/src/site/plugins/pum-url-tracking.jsclasses/Base/CallToAction.phpclasses/CallToAction/Link.phpclasses/Controllers/CallToActions.phpclasses/Shortcode/CallToAction.phpclasses/Site/Assets.phpincludes/namespaced/utils.php
🚧 Files skipped from review as they are similar to previous changes (3)
- classes/Site/Assets.php
- classes/CallToAction/Link.php
- classes/Controllers/CallToActions.php
2 participants
Summary
Adds a filterable query parameter name system so
pid(and other URL params) can be dynamically renamed to avoid conflicts with third-party plugins like WP User Frontend, Participants Database, etc.Changes
Filterable Parameter Infrastructure (
includes/namespaced/utils.php)get_param_name( $key )— returns the filterable param name for a given key (e.g.,popup_id→pid), with static cache andsanitize_key()outputget_param_names()— returns all param name mappings for JS localizationget_param_value( $key, $fallback, $type )— safe$_GETretrieval using filterable names with type-safe sanitizationget_post_param_value( $key, $fallback, $type )— same for$_POST(no filterable names, direct key)sanitize_param_by_type( $value, $type )— reusable sanitization forstring,int,bool,key,email,urltypesUpdated Consumers
CallToActionscontroller — usesget_param_value()instead of raw$_GETaccess forcta,popup_id, andnotrackCallToActionshortcode — URL generation usesget_param_name( 'popup_id' )instead of hardcoded'pid'Site/Assets.php— exposesparamNamesinpum_varsfor frontend JSpum-url-tracking.js— reads param name frompum_vars.paramNames.popup_idwith'pid'fallbackFiltering Example
Test plan
?pid=123appended?pum_popup_id=123instead?pid=123Summary by CodeRabbit
New Features
Improvements