Fix: All Popups bulk actions fail when categories/tags exist (#1175)#1176
Fix: All Popups bulk actions fail when categories/tags exist (#1175)#1176marklchaves wants to merge 58 commits intomasterfrom
Conversation
- Fixed the issue where the license key was not deactivating properly. - Removed unnecessary checks in the license key handling logic.
- Added support for Pro licenses in the license management system. - Introduced methods to check for active Pro licenses and retrieve Pro license keys and tiers. - Updated settings and templates to reflect Pro license status, improving user experience by providing relevant notices. - Adjusted license activation and deactivation logic to skip actions when a Pro license is active. - Enhanced data structures to include Pro license information in relevant fields. Testing confirmed: ✅ Pro license checks function correctly ✅ User notices display appropriately based on license status
- Changed the `replace_editor` action to a filter to ensure proper value return. - Updated the `replace_editor` method to return the original value when not handling specific post types. - Added entry to CHANGELOG.md for the fix related to the filter usage. Testing confirmed: ✅ Filter now correctly returns the value as expected.
- Renamed methods related to Pro license checks for consistency and clarity, changing `has_active_pro_license` to `has_pro_license`. - Updated references throughout the `License.php` class to reflect the new method names. - Adjusted comments for better understanding of the code's functionality. - Updated CHANGELOG.md to document the fix regarding license key display issues. Testing confirmed: ✅ License management functions correctly with updated method names.
Fixed a critical compatibility issue where the popup editor would fail to load when the block editor was enabled in Divi 4. The classic editor is now automatically enforced for Divi 4 users to resolve the conflict. Changes: - Updated Divi compatibility controller to force classic editor for Divi 4 (versions 4.0.0-4.99.99) - Hook at priority 999 to execute after Divi 4's block editor filter - Disable and auto-check the "Use Classic Editor" setting for Divi 4 users - Add visible warning explaining Divi 4 compatibility requirement - Updated changelog with user-friendly description Fixes the blank popup editor issue that only affects Divi 4 users with block editor enabled.
Adds native support for Newsletter plugin subscription forms: - PHP integration class extending PUM_Abstract_Integration_Form - JavaScript handler for AJAX form submission success events - Registered in core integrations array Follows modern integration pattern (FluentForms, WSForms, MC4WP)
Remove SUCCESS_PATTERNS matching - client-side validation means missing inputs are sufficient evidence of successful submission.
feat: Add Newsletter plugin form integration
…Newsletter plugin integration
- Added improvements to the CHANGELOG.md, highlighting the integration of AJAX submission for the Newsletter plugin. - Refactored block asset loading in the BlockEditor class to ensure block styles are only loaded when necessary, optimizing performance by leveraging WordPress's automatic loading mechanism for block styles.
Configure CodeRabbit to auto-review PRs targeting develop branch. Since develop is automatically merged to master during releases, we need PR reviews before merging to develop.
All PRs target develop; master is updated via automated release merges.
- Add unified CI workflow with PHP, JS/TS, and CSS quality checks - Only lint changed files for faster feedback - Add build validation to catch webpack errors - Add dependency tree validation - Add security scanning with composer/npm audit - Disable PHPUnit tests until test suite is comprehensive - Fix PHPUnit dependency conflicts with --ignore-platform-reqs - Add TypeScript type checking to CI pipeline Jobs: - PHP Quality: PHPCS + PHPStan on changed files (PHP 7.4-8.2) - JS/TS Quality: ESLint + Stylelint + TypeScript checks - Build Validation: webpack build + dependency tree validation - Security: composer audit + npm audit - PHPUnit: Disabled (ready to enable when needed) Files: - New: .github/workflows/ci.yml (315 lines, 5 jobs) - Disabled: phpcs-tests.yml → phpcs-tests.yml.disabled - Disabled: phpunit-tests.yml → phpunit-tests.yml.disabled - Updated: composer.json (added lint:changed script) - Updated: package.json (added lint:js:changed, lint:style:changed)
- Updated Storybook related packages in package.json, package-lock.json, and packages/icons/package.json to version 8.6.14 for improved features and bug fixes. - Ensured consistency across all relevant files for dependency management.
- Upgrade actions/checkout from v4 to v6 - Upgrade actions/setup-node from v4 to v6 - Replace manual composer caching with built-in cache: composer - Replace composer update with composer install for deterministic builds - Add cache: npm to setup-node in security job All actions now on latest stable versions with Node.js 24 runtime support. No breaking changes for existing workflow patterns.
Establish automated release foundation with commit message enforcement and standardized tooling. ## Features - Commitlint + husky for conventional commit enforcement - AI attribution blocking (local hooks + CI validation) - Expanded commit types/scopes aligned with issue labels - Release scripts standardization (version-update, changelog-update, extract-changelog) - Reusable GitHub Actions cache workflows ## Technical Changes - Add @commitlint/cli and @commitlint/config-conventional - Add husky for git hooks management - New commitlint.yml CI workflow - Updated labels.json with release-related labels Closes #1136
Add centralized detect-changes job that gates expensive CI jobs: - php-quality: only runs when PHP files change - js-quality: only runs when JS/CSS files change - build-validation: only runs when JS or npm deps change - npm-security: only runs when package.json/lock change - php-security: only runs when composer.json/lock change This significantly speeds up CI for: - PHP-only PRs (skip npm ci, build, JS linting) - JS-only PRs (skip composer, PHP linting) - Config/docs PRs (skip all expensive checks)
The attribution check is enforced by local git hooks - by the time a PR comment appears, commits are already pushed and can't be undone. Removed redundant/unhelpful notice from the PR comment message.
Allows proper nouns, project names, and acronyms in commit subjects. Only ALL_CAPS is warned against.
* feat(release): Phase 2 - Release Please integration Add automated version calculation and release PR creation: - release-please.yml workflow for weekly release PR creation - Configuration files for semantic versioning from commits - Integration documentation Part of automated release process implementation. * fix(release): use weekly schedule instead of push trigger Release Please now runs on Monday 9am UTC schedule to aggregate all commits into a single weekly release PR, rather than updating on every push to develop. Manual trigger (workflow_dispatch) available for testing. * fix(release): address CodeRabbit review items - Update action to googleapis/release-please-action@v4 (actively maintained) - Fix version output to avoid malformed '..' when no release created - Remove extra-files config, use update-versions.js instead (battle-tested) - Add workflow steps to run npm run version:update on release - Fix WordPress.org capitalization in docs - Update docs to reflect weekly schedule behavior
- request-approval.yml: Interactive approve/reject to #dev-releases - notify-support.yml: Customer changelog to #support-updates - Both support dry-run testing via workflow_dispatch
Enables Popup Maker conversion tracking and trigger actions for Forminator form submissions.
Enables conversion tracking and trigger actions for Elementor Pro form submissions with support for targeting specific forms.
Enables conversion tracking for Bit Form with custom storage handling and form instance identification.
Enables conversion tracking for HTML Forms with lightweight form submission detection.
Adds form submission tracking and conversion analytics for Beaver Builder page builder forms. **Supported form types:** - Contact Form module - Subscribe Form module - Login Form module **Implementation:** - JavaScript tracks AJAX form submissions via `ajaxComplete` event - Detects success responses from `fl_builder_email` and `fl_builder_subscribe_form_submit` actions - Forms tracked by type + node ID pattern (e.g., `contact_any`, `subscribe_any`) Closes #1136
- Use form plugins' own text domains for labels with phpcs:ignore - Convert self:: to $this-> for instance method calls - Use if ($popup_id) pattern for conversion tracking - Form submission always fires regardless of popup context
Reduces vulnerabilities: 26 → 14 (high: 15 → 3)
Co-authored-by: Daniel L. Iser <daniel@code-atlantic.com>
Clarify instructions for the Get Element Selector feature - remove reference to non-existent 'ok' button and use more direct language. Props to @marklchaves.
Allows administrators to sort popups by their enabled/disabled status in the admin list table, making it easier to find and manage active or inactive popups.
Allow admins to enable or disable multiple popups at once from the All Popups list table using the Bulk Actions dropdown. Only published popups can be toggled (matches UI behavior). Displays success notice with count of affected and skipped popups.
Adds comprehensive JSDoc documentation to all functions in the changelog extraction utility script.
- Add lint-staged to run linters on staged files only - Configure pre-commit hook with lint-staged - PHP: composer format + lint - JS/TS/CSS: wp-scripts format + lint --fix
Replaces attribute filters with tag filters for complete script/style ID obfuscation: - Use script_loader_tag, style_loader_tag, wp_inline_script_attributes filters - Add generate_obfuscated_handle() for DRY ID generation - Catches ALL script output: main, -js-extra, -js-before, -js-after, -js-translations - Default obfuscation method changed from 'random' to 'custom' for predictability - Consistent defaults across AssetCache.php, Settings.php, and Analytics.php Closes #1155
feat: add UTM tracking to all upsell upgrade links Created generate_upgrade_url() helper function for consistent UTM parameter application across all upsell touchpoints. Updated notice bar messages to use contextual UTM campaigns (ecommerce-detected, lms-detected, crm-detected, pro-generic-upgrade, free-generic-upgrade). All upgrade links now open in new tab with proper security attributes and route to pricing page with UTM tracking for analytics. Enables measurement of which upsell messages drive traffic and conversions.
Fixes blank tab display when switching from Click Trigger advanced tab to Time Delay trigger. Closes #1109
Strips existing protocols from HTTP_HOST before URL construction to prevent malformed URLs like https://http//site.com in pagination and sorting links. Closes #1092
Adds defensive validation to handle Fluent Forms passing stdClass instead of array during double opt-in submission. Closes #1094
) The detect-changes job uses `files_yaml:` which outputs properties prefixed with the category name (e.g., `php_any_changed`), not just the category name (e.g., `php`). Added documentation comments explaining the two tj-actions/changed-files output patterns used in this workflow: - `files_yaml:` → outputs are `{category}_any_changed` - `files:` → outputs are `any_changed` (no prefix) This was causing all gated jobs to skip because the outputs were always undefined.
* fix(build): improve CI quality check accuracy 1. PHPCS: Only fail on errors, not warnings - Added --warning-severity=0 flag - Warnings still shown but don't block merges 2. Separate JS detection for different purposes: - `js`: All JS/TS files (for ESLint) - `js_src`: Source files only (packages/**/src + assets/js/src) 3. Build/TypeScript only on source changes: - Build Validation: triggers on js_src + npm_deps - TypeScript check: triggers on js_src only - ESLint: triggers on all JS/TS (config, tooling, etc.) * ci: reduce PHP matrix to min+latest, narrow CSS scope to source files * ci: remove no-op push trigger and invalid cache input * chore: fix coderabbit config schema
- js_src: All source JS/TS files (triggers build validation) - ts_src: TypeScript only (triggers tsc type checking) Plain JS files now trigger build but not TypeScript checks.
PHPUnit dev dep uses PHP 8+ union type syntax which causes parse errors when PHP 7.4 tries to load vendor autoloader for PHPCS. Code compatibility is the same regardless of PHP version running PHPCS.
* fix: exclude mailto/tel URLs from PID parameter tracking Non-HTTP protocols like mailto:, tel:, and javascript: cannot handle query parameters. The ?pid= was being appended incorrectly, breaking these special links. Added regex check to skip any URL with a protocol that isn't http(s). * feat: add link click conversion tracking for external links - Fire beacon analytics for mailto, tel, and external link clicks - Track link clicks as conversions with eventData.type: 'link_click' - Add LinkClickTracking service with atomic counter updates - Preserve existing PID tracking for internal links only * refactor: use %i placeholder for table identifiers (WP 6.2+) Modernizes SQL queries to use wpdb::prepare() %i placeholder for table name identifiers instead of PHP string interpolation. * docs: add changelog entries for link click tracking feature * fix: run PID tracking at priority 0 to beat competing plugins Ensures template_redirect fires before other plugins that might intercept/redirect, preventing missed tracking events.
* feat: add form conversion tracking service - Created FormConversionTracking service to track form submissions - Hooks into pum_integrated_form_submission action - Tracks both site-wide and per-popup conversion counts - Stores counts in wp_options (site) and post_meta (popup) - Registered service in DI container and initialized on plugin load - Includes helper methods: get_site_count(), get_popup_count() - Includes reset methods for testing/data cleanup - TODO: Send to Supabase v4 when tracking system is ready This enables: - Milestone-based upsell triggers (e.g., "congrats on first form!") - Future analytics dashboard showing form conversion rates - Per-popup performance metrics for power users Addresses: Quick win for Monday release - form conversion foundation * fix: only track form conversions captured by popups - Add early return when popup_id is empty or invalid - Prevents counting forms submitted outside of popups - Ensures site-wide count only includes popup-captured submissions - Simplifies logic by handling popup_id once instead of twice * docs: update FormConversionTracking version tags and docblock - Change @SInCE tags from 1.21.3 to X.X.X for release versioning - Remove opt-in usage tracking reference from class docblock - Simplify class description to focus on core functionality * chore: add commitlint as dev dependency - Auto-installed during commit message validation - Required for commit message format enforcement * fix: track AJAX form submissions via analytics beacon - Add hook to pum_analytics_conversion for AJAX submissions - Non-AJAX submissions continue via pum_integrated_form_submission - Ensures all form submissions are tracked regardless of method - Resolves issue where AJAX forms weren't being counted * fix: FormConversionTracking now only tracks forms using eventData pattern - Adopt Pro's eventData structure for form metadata - Filter AJAX tracking by eventData.type === 'form_submission' - Prevents counting CTAs and link clicks as form submissions - Maintains Pro compatibility (Pro removes handler entirely) - Non-AJAX forms still tracked via pum_integrated_form_submission * feat: add REST endpoint eventData sanitization and FluentForms integration - Add eventData parameter to Analytics REST endpoint with JSON decode sanitization (matches Pro's Request pattern) - Implement FluentForms integration with defensive handling of double-event firing - Clean FormConversionTracking implementation expecting pre-decoded eventData array - Add TODO in Telemetry for future usage tracking inclusion This completes the form conversion tracking feature with proper architecture matching Pro's proven eventData handling approach. * feat: add popup existence validation before tracking form conversions Prevents orphaned postmeta entries for deleted/invalid popups. Logs validation failures only in WP_DEBUG mode without breaking form submissions. Tracking is non-critical and should fail gracefully. * chore: sync package-lock.json with package.json * fix: address CodeRabbit review feedback - Add defensive validation to track_form_conversion() and track_ajax_conversion() - Fix race condition: use atomic SQL updates for increment operations - Add cache flushing after direct wpdb updates - Set autoload=false for analytical data option - Remove duplicate commitlint dependency (keep @commitlint/cli) * chore: regenerate package-lock.json for CI compatibility * fix: resolve ESLint formatting and console warnings * refactor: apply CodeRabbit suggestions and LinkClickTracking patterns - Replace get_post() with pum_get_popup() + pum_is_popup() for proper validation - Use %i placeholder for table names in SQL queries (modern wpdb pattern) - Add popup_maker/form_conversion_tracked action after tracking (extensibility)
Changed %i (identifier/backticks) to %s (string/quotes) in SHOW TABLES LIKE query. The %i placeholder wraps values in backticks which breaks the SHOW TABLES LIKE syntax that expects a quoted string pattern.
|
Warning Ignoring CodeRabbit configuration file changes. For security, only the configuration from the base branch is applied for open source repositories. 📝 WalkthroughWalkthroughThis pull request introduces comprehensive form submission tracking, analytics enhancements, CI/CD automation, and admin features. Eight form builder integrations (Beaver Builder, Elementor, Forminator, HappyForms, HTML Forms, Kali Forms, Bit Form, Newsletter) enable client-side and server-side form submission tracking. New LinkClickTracking and FormConversionTracking services provide analytics capabilities. Release automation via Release Please and GitHub Actions workflows streamline versioning and code quality checks. Husky hooks and commitlint enforce commit standards. Admin bulk actions, license management improvements, and asset obfuscation enhancements round out the changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User Submits Form
participant Form as Form Plugin<br/>(e.g., Gravity Forms)
participant JS as Client-side<br/>Integration
participant Handler as PUM Integration<br/>Handler
participant Beacon as Analytics<br/>Beacon
User->>Form: Submit Form
Form->>JS: Trigger Success Event
JS->>JS: Extract form ID &<br/>submission data
JS->>Handler: formSubmission(provider,<br/>formId, data)
Handler->>Handler: Store form event<br/>in memory
Handler->>Beacon: Send conversion<br/>beacon payload
Beacon->>Beacon: Increment form<br/>conversion counts
Note over Beacon: Site-wide & Per-popup<br/>conversion tracking
sequenceDiagram
participant User as User Clicks Link
participant Page as Web Page
participant ClickHandler as URL Tracking<br/>Plugin
participant Beacon as Analytics<br/>Beacon
participant Analytics as Analytics<br/>Service
User->>Page: Click Link
Page->>ClickHandler: Link Click Event
alt Internal Link
ClickHandler->>ClickHandler: Append PID param
ClickHandler->>Page: Navigate with PID
else External Link
ClickHandler->>ClickHandler: Categorize link type<br/>(external, mailto, tel, etc.)
ClickHandler->>Beacon: Send click beacon<br/>(URL, type, PID)
Beacon->>Analytics: Track link click<br/>conversion
Analytics->>Analytics: Increment link click<br/>counts
ClickHandler->>Page: Navigate original URL
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
classes/Controllers/WP/Dashboard.php (1)
40-47:⚠️ Potential issue | 🟡 MinorWidget still renders an empty box for users who lack
edit_popupscapability.
register_dashboard_widgetsregisters the widget for everyone, butrender_basic_analytics_widgetreturns early for unauthorized users. This leaves an empty "Popup Analytics" widget on the dashboard. Move the capability check to registration so the widget is not added at all for unauthorized users.Proposed fix
public function register_dashboard_widgets() { + if ( ! current_user_can( $this->container->get_permission( 'edit_popups' ) ) ) { + return; + } + // Basic Analytics Widget (will be moved to free plugin). wp_add_dashboard_widget( 'pum_analytics_basic', __( 'Popup Analytics','popup-maker' ), [ $this, 'render_basic_analytics_widget' ] ); }You can keep the check in
render_basic_analytics_widgetas defense-in-depth, or remove it once registration is gated.Also applies to: 74-78
classes/Admin/Popups.php (1)
1590-1636:⚠️ Potential issue | 🔴 CriticalCritical: The root-cause fix for issue
#1175is missing fromadd_popup_filters().This method contains all four bugs described in the linked issue, and none have been addressed:
- Lines 1612 & 1633:
wp_nonce_field('pum-popup-filter-nonce')omits the second parameter, so it defaults to_wpnonce— overwriting WordPress core's bulk-action nonce and causing the "link expired" error.- Lines 1601 & 1622: Nonce verification is inverted (
! wp_verify_nonce(...)) — filter selections are restored only when the nonce is invalid.- Lines 1601 & 1622: Still checks
$_GET['_wpnonce']instead of the custompum_filter_noncefield.- Duplicate nonce output: Both the category and tag blocks emit their own
wp_nonce_field, producing two hidden fields with the same name.Without these fixes, the new bulk enable/disable actions added in this PR will also fail whenever popup categories or tags exist — the exact scenario from issue
#1175.🐛 Proposed fix
public static function add_popup_filters() { global $typenow; // Checks if the current post type is 'popup' if ( 'popup' === $typenow ) { + $has_filters = false; + if ( get_taxonomy( 'popup_category' ) ) { $terms = get_terms( 'popup_category' ); if ( count( $terms ) > 0 ) { $category = ''; - if ( isset( $_GET['_wpnonce'] ) && ! wp_verify_nonce( sanitize_key( wp_unslash( $_GET['_wpnonce'] ) ), 'pum-popup-filter-nonce' ) ) { + if ( isset( $_GET['pum_filter_nonce'] ) && wp_verify_nonce( sanitize_key( wp_unslash( $_GET['pum_filter_nonce'] ) ), 'pum-popup-filter-nonce' ) ) { $category = isset( $_GET['popup_category'] ) ? sanitize_key( wp_unslash( $_GET['popup_category'] ) ) : ''; } echo "<select name='popup_category' id='popup_category' class='postform'>"; echo "<option value=''>" . esc_html__( 'Show all categories', 'popup-maker' ) . '</option>'; foreach ( $terms as $term ) { $selected = $category === $term->slug ? 'selected="selected"' : ''; echo '<option value="' . esc_attr( $term->slug ) . '" ' . esc_attr( $selected ) . '>' . esc_html( $term->name ) . ' (' . esc_html( $term->count ) . ')</option>'; } echo '</select>'; - wp_nonce_field( 'pum-popup-filter-nonce' ); + $has_filters = true; } } if ( get_taxonomy( 'popup_tag' ) ) { $terms = get_terms( 'popup_tag' ); if ( count( $terms ) > 0 ) { $tag = ''; - if ( isset( $_GET['_wpnonce'] ) && ! wp_verify_nonce( sanitize_key( wp_unslash( $_GET['_wpnonce'] ) ), 'pum-popup-filter-nonce' ) ) { + if ( isset( $_GET['pum_filter_nonce'] ) && wp_verify_nonce( sanitize_key( wp_unslash( $_GET['pum_filter_nonce'] ) ), 'pum-popup-filter-nonce' ) ) { $tag = isset( $_GET['popup_tag'] ) ? sanitize_key( wp_unslash( $_GET['popup_tag'] ) ) : ''; } echo "<select name='popup_tag' id='popup_tag' class='postform'>"; echo "<option value=''>" . esc_html__( 'Show all tags', 'popup-maker' ) . '</option>'; foreach ( $terms as $term ) { $selected = $tag === $term->slug ? 'selected="selected"' : ''; echo '<option value="' . esc_attr( $term->slug ) . '" ' . esc_attr( $selected ) . '>' . esc_html( $term->name ) . ' (' . esc_html( $term->count ) . ')</option>'; } echo '</select>'; - wp_nonce_field( 'pum-popup-filter-nonce' ); + $has_filters = true; } } + + // Output a single nonce field with a unique name to avoid overwriting core's _wpnonce. + if ( $has_filters ) { + wp_nonce_field( 'pum-popup-filter-nonce', 'pum_filter_nonce' ); + } } }classes/Integration/Form/WPForms.php (1)
76-94:⚠️ Potential issue | 🟠 MajorGuard
pum_integrated_form_submissioncall with popup_id check for consistency.The concern is valid:
get_popup_id()returnsfalsewhen the form is not submitted inside a popup, and thisfalsevalue is passed topum_integrated_form_submissionat line 89. However,increase_conversion()at line 83 is correctly guarded withif ( $popup_id ). The function expectsnullas the default forpopup_id, notfalse. For consistency and to avoid unexpected behavior in downstream hooks, wrap thepum_integrated_form_submissioncall with the same guard:if ( $popup_id ) { $this->increase_conversion( $popup_id ); pum_integrated_form_submission( [...] ); }Or move the call inside the existing guard block to ensure the function only processes submissions from forms inside popups.
🤖 Fix all issues with AI agents
In @.github/workflows/notify-support.yml:
- Around line 193-203: The current curl invocation uses a single-quoted inline
expansion of steps.payload.outputs.payload which will break if the payload
contains single quotes; update the "Send to `#support-updates`" step to write the
payload from steps.payload.outputs.payload to a temporary file (e.g.,
payload.json) and call curl with -d `@payload.json` using the SLACK_WEBHOOK_URL
env var, then remove the temp file afterwards; reference the step output name
steps.payload.outputs.payload and the env SLACK_WEBHOOK_URL when making the
change.
- Around line 36-65: The step with id "vars" is vulnerable because it injects
user-controlled values directly into heredocs using ${{
github.event.release.body }} and ${{ inputs.release_body }}; instead, set those
values as step environment variables (e.g., env: RELEASE_BODY: ${{
github.event.release.body }} or RELEASE_BODY: ${{ inputs.release_body }} under
the same step) and write them to /tmp/release_body.txt using a safe shell write
like printf '%s\n' "$RELEASE_BODY" > /tmp/release_body.txt (repeat for the
inputs branch using a different env name), leaving all heredocs removed from the
step to eliminate the EOFBODY injection vector.
In @.github/workflows/request-approval.yml:
- Around line 185-195: The "Send Slack approval request" workflow step injects
the payload string directly into curl using '${{ steps.payload.outputs.payload
}}', which breaks on embedded single quotes; instead, write the payload value
from steps.payload.outputs.payload to a temporary file (e.g., via echo or printf
redirected to a temp file) and call curl with -d `@tempfile` against
SLACK_WEBHOOK_URL, then remove the tempfile; update the step that references the
env SLACK_WEBHOOK_URL and the output variable steps.payload.outputs.payload to
use the temp-file approach to avoid shell quoting issues.
- Around line 85-92: The CHANGELOG assignment is unquoted and can break if the
JSON string contains spaces or shell metacharacters; update the workflow step
that sets VERSION/PLUGIN/ARTIFACT/REPO/DOWNLOAD_URL/CHANGELOG/DRY_RUN to either
export CHANGELOG via the GitHub Actions env context (e.g., add CHANGELOG: ${{
steps.changelog.outputs.changelog }} under env) or wrap the assignment value in
proper shell quoting so that CHANGELOG receives the full JSON string from
steps.changelog.outputs.changelog without word-splitting or globbing; adjust the
same pattern for any other multi-word outputs if needed.
- Around line 29-43: The step "Set variables" (id: vars) is vulnerable because
expressions like ${{ github.event.client_payload.version }} are expanded into
the shell; instead move those expression evaluations into the step's env block
(e.g., VERSION: ${{ github.event.client_payload.version }}, PLUGIN: ${{
github.event.client_payload.plugin || github.event.repository.name }}, ARTIFACT:
${{ github.event.client_payload.artifact }}, DRY_RUN: false) and then in the run
script write safe, already-expanded env vars to $GITHUB_OUTPUT (echo
"version=$VERSION" >> $GITHUB_OUTPUT, etc.); do the same for the
workflow_dispatch branch by exposing inputs via env and referencing those env
vars in run to eliminate direct expression interpolation into the shell.
In @.release-please-config.json:
- Line 12: The .release-please-config.json contains an invalid setting
"changelog-type": "wordpress"; update the changelog-type key (or remove it) so
it uses a supported value ("default" or "github") to satisfy Release Please
schema — locate the "changelog-type" entry and either change its value to
"default" or "github" or delete the "changelog-type" field entirely.
In `@assets/js/src/integration/kaliForms.js`:
- Around line 20-82: The integration listens for undocumented events
(kaliformProcessCompleted, kaliFormSuccess) which may not fire and can cause
double submissions; verify and use the documented event
kaliFormProcessConstructed (or the actual event from your Kali Forms version) in
the document.addEventListener and the submit handler, remove the redundant
listener, and add a per-form guard (e.g., a processedSet keyed by formInstanceId
checked in the handlers) to ensure window.PUM.integrations.formSubmission(…) is
called only once per form submission; update references to the listener function
names (successHandler) and the event names to match the verified event and
remove the secondary listener registration/cleanup to eliminate double-firing
risk while keeping the non-AJAX PHP hook (kaliforms_after_form_process_action)
behavior unchanged.
In `@assets/js/src/integration/newsletter.js`:
- Around line 127-131: The 30s setTimeout that calls observer.disconnect() and
deletes form.dataset.pumNewsletterObserved can prematurely stop tracking; update
the logic in the block using setTimeout to either remove the timeout entirely,
increase the delay to a much larger value (e.g., 300000 ms = 5 minutes), or
implement re-arming when the popup opens again by moving the observer setup into
the popup-open handler and ensuring observer.disconnect() only runs when the
form is truly finished; target the setTimeout call, observer.disconnect, and
manipulation of form.dataset.pumNewsletterObserved when making the change.
In `@assets/js/src/site/plugins/pum-url-tracking.js`:
- Around line 157-165: The code currently places the full href into
eventData.url (in the data object built around pid/event/conversion), which can
leak PII for mailto: and tel: links; update the logic that builds data (the
eventData.url field) — in the block constructing data for conversion events and
where self.getLinkType(href) is used — to sanitize or redact sensitive parts:
for mailto: and tel: links either omit eventData.url entirely or replace it with
a non-PII placeholder (e.g., "mailto:REDACTED" / "tel:REDACTED" or only send
linkType), and ensure any beacon/analytics call uses this sanitized value
instead of the raw href.
- Around line 84-102: The shouldTrackClick function currently only skips empty
and CTA-tagged URLs, so anchor-only links and javascript: links still trigger
conversion beacons; update shouldTrackClick (and reference isInternalUrl usage)
to explicitly return false for URLs that are pure anchors or fragment-only
(e.g., '#' or starting with '#') and for URLs that start with 'javascript:'
(case-insensitive, trim whitespace first) so clicks like in-page anchors or
javascript:void(0) do not fire tracking beacons. Ensure you perform the checks
before the CTA check and any other early returns.
In `@bin/extract-changelog.js`:
- Around line 58-71: The regex in extractVersionContent (and similarly in
extractUnreleasedContent and extractLatestVersion) stops at the first line
because the non-greedy [\s\S]*? combined with the multiline flag makes the
lookahead match end-of-line; replace the regex-based single-pass extraction with
an indexOf/substring approach: locate the header start (e.g., find the "##
v{version}" or "## Unreleased" header using indexOf after escaping/version
formatting), then find the next "## " occurrence (or end of string) with indexOf
and return the substring between them trimmed. Update extractVersionContent,
extractUnreleasedContent, and extractLatestVersion to use this header-indexing
logic instead of the complex regex.
In `@classes/Analytics.php`:
- Around line 179-192: sanitize_event_data currently converts JSON or strings to
arrays but leaves individual values unsanitized; update sanitize_event_data to
recursively sanitize leaf values before returning (use sanitize_text_field for
simple scalar leaves or wp_kses_post for HTML-allowed fields as appropriate).
Locate the sanitize_event_data function and after decoding/accepting an array,
apply a recursive sanitizer (e.g., implement a private helper or use
array_walk_recursive) to transform each scalar value, then return the sanitized
array; ensure handlers like pum_analytics_conversion receive sanitized data.
In `@classes/Integration/Form/BeaverBuilder.php`:
- Around line 47-62: BeaverBuilder's get_forms() uses 'ID'/'post_title' instead
of the standard 'id'/'title' and get_form_selectlist() is duplicated; change
get_forms() to return each form as ['id'=>..., 'title'=>...] to match other
integrations, update get_form() to read the new keys (use 'id' for lookups and
return the same normalized shape), and implement get_form_selectlist() to build
its list by iterating over get_forms() (mapping id=>title) so the select list is
always derived from get_forms(); ensure any code expecting legacy keys is
adjusted or mapped inside these methods to keep behavior consistent.
In `@classes/Integration/Form/FluentForms.php`:
- Around line 102-111: The code in classes/Integration/Form/FluentForms.php is
extracting the form id from the wrong property (attributes->id) so $form_id
stays null; inside the FluentForms integration (the method handling the
fluentform/before_submission_confirmation hook where $form is provided) replace
the defensive check that looks for $form->attributes->id with a check for
$form->id (e.g., if (is_object($form) && isset($form->id)) { $form_id =
$form->id; }) so the actual form ID is captured for conversion tracking.
In `@classes/Services/LinkClickTracking.php`:
- Around line 84-91: Replace the raw post checks using get_post() and
get_post_type() with Popup Maker helpers: call pum_get_popup($popup_id) to
retrieve the popup object and use pum_is_popup($popup) (and/or
$popup->is_valid()) to validate it before tracking; update the variable $popup
to hold the pum_get_popup() result and change the conditional that currently
checks get_post_type() to use pum_is_popup($popup) so the same validation logic
as FormConversionTracking is used.
🟡 Minor comments (8)
classes/Controllers/Compatibility/Builder/Divi.php-130-134 (1)
130-134:⚠️ Potential issue | 🟡 MinorStale docblock: priority described as 5 but registered at 999.
The docblock states "This runs at priority 5 (before PostTypes filter at priority 10)" but the filter is actually registered at priority 999 on line 63. The comment on line 61–62 correctly explains the intent. Update the docblock to match.
Proposed fix
- * This runs at priority 5 (before PostTypes filter at priority 10) - * to ensure Divi 4's conflicting filters can't override our setting. + * This runs at priority 999 (after Divi 4's priority 100) + * to ensure Divi 4's conflicting filters are overridden.classes/Controllers/Compatibility/Builder/Divi.php-170-178 (1)
170-178:⚠️ Potential issue | 🟡 MinorPotential PHP notice if
desckey is missing on the field.If
enable_classic_editorexists but doesn't have adesckey, the.=on line 177 will trigger an undefined index notice. Add a guard or initialize it.Proposed fix
if ( isset( $fields['general']['main']['enable_classic_editor'] ) ) { $field = &$fields['general']['main']['enable_classic_editor']; // Disable the field in UI and set to checked. $field['disabled'] = true; // Append Divi 4 note to existing description. + if ( ! isset( $field['desc'] ) ) { + $field['desc'] = ''; + } $field['desc'] .= '<br/><strong style="color: `#d63638`;">' . esc_html__( 'Divi 4 requires the classic editor for popup editing. This setting is automatically enforced for compatibility.', 'popup-maker' ) . '</strong>'; }classes/Services/FormConversionTracking.php-101-110 (1)
101-110:⚠️ Potential issue | 🟡 Minor
popup_maker/form_conversion_trackedhook fires with inconsistent payload shapes.
track_form_conversion(line 109) passes the full$argsarray (containingpopup_id,form_provider,form_id,tracked, etc.).track_ajax_conversion(line 173) passes only$event_data(containingtype, and whatever the JS beacon sends).Any consumer hooking
popup_maker/form_conversion_trackedwill receive structurally different data depending on the code path. Consider normalizing the second argument to a consistent shape, or using distinct hook names if the payloads are intentionally different.Also applies to: 165-174
classes/AssetCache.php-1308-1332 (1)
1308-1332:⚠️ Potential issue | 🟡 MinorInline script obfuscation only catches
-js-extra; misses-js-before,-js-after, and-js-translations.WordPress generates inline
<script>tags with IDs like{handle}-js-before,{handle}-js-after, and{handle}-js-translations(viawp_add_inline_script()and script translations). All of these pass throughwp_inline_script_attributes, but the current regex only matches-js-extra, leaving the original handle exposed in those IDs.♻️ Broaden the regex to catch all inline script suffixes
- if ( preg_match( '/^(.+)-js-extra$/', $id, $matches ) ) { + if ( preg_match( '/^(.+)-js-(extra|before|after|translations)$/', $id, $matches ) ) { $handle = $matches[1]; if ( self::should_obfuscate_handle( $handle ) ) { $obfuscated = self::generate_obfuscated_handle( $handle ); - $attributes['id'] = $obfuscated . '-js-extra'; + $attributes['id'] = $obfuscated . '-js-' . $matches[2]; } }CHANGELOG.md-24-36 (1)
24-36:⚠️ Potential issue | 🟡 MinorThe fix for issue
#1175(bulk actions failing when categories/tags exist) appears to be missing from the Fixes section.The PR specifically fixes the nonce collision that broke bulk actions on the All Popups list. Consider adding an entry like:
- Fixed All Popups bulk actions failing with "The link you followed has expired" when popup categories or tags exist. Closes
#1175.classes/Integration/Form/HTMLForms.php-90-100 (1)
90-100:⚠️ Potential issue | 🟡 MinorAdd
function_exists()guard forhf_get_form()sinceenabled()only checks forhf_get_forms().The
enabled()method verifieshf_get_forms()(plural) exists, butget_form()callshf_get_form()(singular) without a prior existence check. In PHP 7.4+, calling an undefined function throws anError(notException), so the try-catch will not prevent a fatal error. This should be guarded withfunction_exists( 'hf_get_form' )to follow the defensive validation pattern used elsewhere in the codebase.🛡️ Proposed fix
public function get_form( $id ) { if ( ! $this->enabled() ) { return null; } try { + if ( ! function_exists( 'hf_get_form' ) ) { + return null; + } return hf_get_form( $id ); } catch ( Exception $e ) { return null; } }classes/Upsell.php-198-207 (1)
198-207:⚠️ Potential issue | 🟡 Minor
$has_eddis assigned but never read.The static analysis tool correctly flags this.
$has_eddon line 200 is unused — theelsebranch on line 205 already acts as the implicit EDD fallback. Remove it to avoid confusion.🧹 Proposed fix
// Determine which ecommerce platform to link to. $has_woocommerce = in_array( 'WooCommerce', $platforms, true ); -$has_edd = in_array( 'Easy Digital Downloads', $platforms, true ); // Route to specific integration page (WooCommerce preferred if both exist, though rare). if ( $has_woocommerce ) {assets/js/src/integration/newsletter.js-21-22 (1)
21-22:⚠️ Potential issue | 🟡 MinorOverly broad selector
form[action*="newsletter"]could match unrelated forms.The substring match
action*="newsletter"will match any form whose action URL contains "newsletter" — including forms from other plugins or custom forms. This could cause false-positive submission tracking. Consider tightening this selector or removing it if the class-based selectors (form.tnp-subscription, form.tnp-ajax) are sufficient.
🧹 Nitpick comments (39)
classes/Controllers/PostTypes.php (1)
541-562: Update the docblock to match the new return type and namespace.The method now returns a value, but the docblock still says
@return void. Also,WP_Postshould be fully qualified (\WP_Post) since this class is namespaced.📝 Proposed docblock fix
/** * Replace the editor interface when classic editor is forced. * - * `@param` bool $replace Whether to replace the editor. - * `@param` WP_Post $post The post object. - * `@return` void + * `@param` bool $replace Whether to replace the editor. + * `@param` \WP_Post $post The post object. + * `@return` bool Whether to replace the editor. */classes/Admin/BlockEditor.php (1)
68-79:$hookparameter is unused.PHPMD flags
$hookas unused. Theenqueue_block_assetsaction doesn't pass any arguments, so this parameter will always benull. The same pattern exists inregister_editor_assets(line 47) — consider cleaning up both if you revisit this area.includes/integrations/class-pum-woocommerce-integration.php (1)
102-102: Consider reusing$woocommerce_textincondition_sort_ordertoo.
condition_sort_orderstill has its own inline__( 'WooCommerce', 'woocommerce' )call with a separatephpcs:ignore. Since both methods are static, you could extract the translated string into a shared class constant or a private static helper to fully centralize it. Minor nit — not blocking.classes/Integration/Form/NinjaForms.php (1)
95-113: Consider defensive validation on$form_databefore accessing keys.This callback is hooked to a third-party action (
ninja_forms_after_submission). If Ninja Forms changes the data structure, the unguarded$form_data['form_id']access on Line 110 would emit a PHP notice. A simpleissetcheck would make this more resilient.Suggested guard
public function on_success_v3( $form_data ) { + if ( ! is_array( $form_data ) || ! isset( $form_data['form_id'] ) ) { + return; + } + if ( ! $this->should_process_submission() ) { return; }As per coding guidelines: "Defensive validation in hook callbacks for graceful handling of third-party plugin parameter changes."
classes/Controllers/Compatibility/Builder/Divi.php (2)
102-103: Missing blank line before docblock.WordPress coding standards require a blank line before a docblock that starts a new method/function declaration.
return $is_divi_available; } + /** * Detect if Divi 4 is active (not 5+).
140-140: Avoid strict type hints on WordPress filter callbacks.
divi4_force_classic_editoris hooked to the WordPress core filteruse_block_editor_for_post_type. WordPress may pass unexpected types (e.g.,nullinstead ofbool). Use defensive validation instead of type-hinted parameters to preventTypeErrorin edge cases.Proposed fix
- public function divi4_force_classic_editor( $use_block_editor, $post_type ) { + public function divi4_force_classic_editor( $use_block_editor = true, $post_type = '' ) { // Only handle our post types. - if ( ! in_array( $post_type, [ 'popup', 'popup_theme' ], true ) ) { + if ( ! is_string( $post_type ) || ! in_array( $post_type, [ 'popup', 'popup_theme' ], true ) ) { return $use_block_editor; }As per coding guidelines: "Never use strict PHP types on methods hooked to third-party actions/filters; use defensive validation instead."
classes/Services/LinkClickTracking.php (1)
119-188: Substantial code duplication withFormConversionTracking.
increment_site_count(),increment_popup_count(),get_site_count(),get_popup_count(),reset_site_count(), andreset_popup_count()are structurally identical in both services — only the option/meta key constants differ. This is a DRY violation across ~120 lines.Consider extracting a shared base class or trait (e.g.,
TrackingCounterTraitorBaseTracking extends Service) that parameterizes the storage keys, so both services can inherit the counter logic.classes/AssetCache.php (2)
143-146: Consider conditionally registering obfuscation hooks.These filters fire on every page (admin + frontend) even when
bypass_adblockersis disabled. Each callback bails early, but registering the filters only when bypass is active would avoid the overhead entirely.♻️ Suggested approach
- // Hook into WordPress script/style tag filters for ID obfuscation. - add_filter( 'script_loader_tag', [ __CLASS__, 'obfuscate_script_tag' ], 10, 3 ); - add_filter( 'style_loader_tag', [ __CLASS__, 'obfuscate_style_tag' ], 10, 4 ); - add_filter( 'wp_inline_script_attributes', [ __CLASS__, 'obfuscate_inline_script_id' ], 10, 2 ); + // Hook into WordPress script/style tag filters for ID obfuscation only when needed. + if ( pum_get_option( 'bypass_adblockers', false ) ) { + add_filter( 'script_loader_tag', [ __CLASS__, 'obfuscate_script_tag' ], 10, 3 ); + add_filter( 'style_loader_tag', [ __CLASS__, 'obfuscate_style_tag' ], 10, 4 ); + add_filter( 'wp_inline_script_attributes', [ __CLASS__, 'obfuscate_inline_script_id' ], 10, 2 ); + }Note: The
is_admin()guard in callbacks would still be useful as a safety net, but the outer check avoids running three filter callbacks on every enqueued asset when the feature is off.
1205-1222: Add memoization to avoid repeated DB lookups per asset tag.
generate_obfuscated_handleis called for every PUM script and style tag on the page. Each call hitsget_site_url()andpum_get_option()(×2). Caching results in a static map would eliminate redundant work.♻️ Suggested memoization
public static function generate_obfuscated_handle( $handle ) { + static $cache = []; + if ( isset( $cache[ $handle ] ) ) { + return $cache[ $handle ]; + } + $site_url = get_site_url(); $method = pum_get_option( 'adblock_bypass_url_method', 'custom' ); // Strip popup-maker-/pum- prefix for cleaner custom names. $suffix = preg_replace( '/^(popup-maker-|pum-)/', '', $handle ); switch ( $method ) { case 'random': - return md5( $site_url . $handle ); + $result = md5( $site_url . $handle ); + break; case 'custom': default: $prefix = pum_get_option( 'adblock_bypass_custom_filename', 'pm' ); $prefix = ! empty( $prefix ) ? $prefix : 'pm'; - return sanitize_html_class( $prefix . '-' . $suffix ); + $result = sanitize_html_class( $prefix . '-' . $suffix ); + break; } + + $cache[ $handle ] = $result; + return $result; }classes/Extension/License.php (1)
128-195: Align service access pattern with project convention:\PopupMaker\plugin('license')instead of\PopupMaker\plugin()->get('license').The codebase consistently uses
\PopupMaker\plugin('license')throughout (found in Upsell.php, Services/License.php, Admin/Pages.php, Admin/Settings.php, RestAPI/License.php, and Admin/Templates.php). The new helper methods here uniquely use\PopupMaker\plugin()->get('license'). Align the three methods to match the project convention for consistency.♻️ Suggested diff
private function has_pro_license() { if ( ! function_exists( '\PopupMaker\plugin' ) ) { return false; } try { - $license_service = \PopupMaker\plugin()->get( 'license' ); + $license_service = \PopupMaker\plugin( 'license' ); $license_key = $license_service->get_license_key(); // Check if Pro key exists and is not empty. return ! empty( $license_key ); } catch ( \Exception $e ) { return false; } } private function get_pro_license_key() { if ( ! function_exists( '\PopupMaker\plugin' ) ) { return ''; } try { - $license_service = \PopupMaker\plugin()->get( 'license' ); + $license_service = \PopupMaker\plugin( 'license' ); return $license_service->get_license_key(); } catch ( \Exception $e ) { return ''; } } private function get_pro_license_tier() { if ( ! $this->has_pro_license() ) { return ''; } try { - $license_service = \PopupMaker\plugin()->get( 'license' ); + $license_service = \PopupMaker\plugin( 'license' ); return $license_service->get_license_tier(); } catch ( \Exception $e ) { return ''; } }.husky/pre-commit (1)
1-3: Placeholder is fine, but consider adding a shebang for when commands are added later.Husky hooks typically need
#!/usr/bin/env sh(or#!/bin/sh) at the top to be recognized as executable scripts. Without it, adding commands in a future phase could silently fail depending on the OS/shell..coderabbit.yaml (1)
17-18: Minor: Comment is misleading — the setting actually disables collapsing.The comment says "Collapse file walkthrough" but
collapse_walkthrough: falsemeans the walkthrough will be expanded/visible. Consider updating the comment to reflect the actual behavior.Suggested comment fix
- # Collapse file walkthrough in PR comments - collapse_walkthrough: false + # Show file walkthrough expanded (not collapsed) in PR comments + collapse_walkthrough: falseassets/js/src/integration/forminator.js (1)
9-30: Consider guarding against missingformId.If
event.targetdoesn't carrydata-form-id(e.g., Forminator triggers the event on a wrapper rather than the<form>element),formIdwill beundefinedand the submission call may produce unexpected analytics data. A guard similar to whatbeaverbuilder.jsdoes (early return if key data is missing) would be more defensive.Suggested guard
const formId = $form.attr( 'data-form-id' ); + + if ( ! formId ) { + return; + } + // Extract form instance ID from data-render-id attribute. const formInstanceId = $form.attr( 'data-render-id' );assets/js/src/integration/beaverbuilder.js (1)
68-69: Nit:formIdandformInstanceIdare identical.Both are set to
formType + '_' + nodeId. If Beaver Builder doesn't have a separate instance concept, consider just passing one or documenting why they're the same.classes/Integration/Form/BeaverBuilder.php (1)
102-115: Redundant method overrides —custom_scriptsandcustom_stylesalready have identical pass-through implementations in the abstract base class.These can be safely removed since
PUM_Abstract_Integration_Formalready defines the same default behavior (seeclasses/Abstract/Integration/Form.phplines ~30-42).classes/Integration/Form/KaliForms.php (2)
117-142: Add defensive type check on$argsbefore array access.Per coding guidelines, hook callbacks should use defensive validation since third-party plugins can change hook signatures.
$argsis accessed as an array without verifying it is one. If the hook signature changes, this will produce PHP warnings/errors.🛡️ Proposed fix
public function on_success( $args ) { if ( ! $this->should_process_submission() ) { return; } + // Defensive validation - never assume parameter types. + if ( ! is_array( $args ) ) { + return; + } + // Get form ID from the data array. $form_id = isset( $args['data']['formId'] ) ? $args['data']['formId'] : null;As per coding guidelines: "Never use strict PHP types on methods hooked to third-party actions/filters; use defensive validation instead."
182-195: Redundant:custom_scriptsandcustom_stylesare already defined as pass-throughs in the abstract base class.Same note as BeaverBuilder — these can be removed without behavior change.
bin/extract-changelog.js (1)
47-49:escapeRegexis missing the hyphen character.While
-is only special inside[]character classes and unlikely to cause issues for version strings used in the current patterns, it's good practice to include it for a general-purpose escape function.♻️ Proposed fix
function escapeRegex( str ) { - return str.replace( /[.+*?^$[\](){}|\\]/g, '\\$&' ); + return str.replace( /[-.+*?^$[\](){}|\\]/g, '\\$&' ); }.github/workflows/ci.yml (3)
7-8:WP_VERSION: 6.1.1is defined but never referenced in any active job.The env variable
WP_VERSIONis unused — the phpunit job that would use it is disabled. Consider removing it to avoid confusion, or add a comment that it's reserved for when phpunit is enabled.
365-368:if: falseis flagged by actionlint as a constant condition.This is intentional (placeholder for future PHPUnit enablement), but consider using a more descriptive approach:
♻️ Alternative: use a workflow-level variable or comment the job out
You could comment out the entire job block instead, or use an env-driven flag:
env: PHPUNIT_ENABLED: false # ... phpunit: if: ${{ env.PHPUNIT_ENABLED == 'true' }}This avoids the lint warning while keeping the intent clear.
158-165: File paths with spaces will break thetr ' ' '\n'splitting.Line 163 splits
all_changed_files(space-separated) into newline-separated entries. If any PHP file path contains a space, the path will be split across two lines and PHPCS will fail to find the files. While uncommon in PHP projects, thetj-actions/changed-filesaction also outputs aall_changed_fileswith JSON-safe encoding available. Consider using theseparatorinput or newline-separated output.classes/Upsell.php (1)
209-216: Inconsistent UTM construction: manualadd_query_argvs. centralizedgenerate_upgrade_url().Lines 303 and 326 correctly use
\PopupMaker\generate_upgrade_url(), but these three blocks manually build UTM parameters withadd_query_arg. Since these target external (non-upgrade) base URLs, the centralized helper may not apply directly, but you could still extract a small helper (e.g.,append_utm_args( $base_url, $medium, $campaign )) to eliminate the repeated array construction and keep UTM key naming consistent in one place.Also applies to: 229-236, 260-267
classes/Integration/Form/Forminator.php (2)
1-9: Missing ABSPATH guard.Other integration files in this PR (e.g.,
Newsletter.php) include the standardif ( ! defined( 'ABSPATH' ) ) { exit; }guard. This file omits it.🛡️ Proposed fix
<?php /** * Integration for Forminator * * `@package` PopupMaker * `@copyright` Copyright (c) 2025, Code Atlantic LLC */ +if ( ! defined( 'ABSPATH' ) ) { + exit; +} + class PUM_Integration_Form_Forminator extends PUM_Abstract_Integration_Form {
153-166: Simplifyget_popup_id()— avoid copying the entire$_POSTsuperglobal.
$form_data = $_POSTis unnecessary; you only need to check a single key. This also makes the intent clearer.♻️ Proposed simplification
public function get_popup_id() { // There is no raw nonce passed with this endpoint, so we need to check the raw data. // phpcs:ignore WordPress.Security.NonceVerification if ( isset( $_POST['forminator-form-id'] ) ) { - // phpcs:ignore WordPress.Security.NonceVerification - $form_data = $_POST; - - if ( isset( $form_data['pum_form_popup_id'] ) ) { - return absint( $form_data['pum_form_popup_id'] ); + // phpcs:ignore WordPress.Security.NonceVerification + if ( isset( $_POST['pum_form_popup_id'] ) ) { + return absint( $_POST['pum_form_popup_id'] ); } } return parent::get_popup_id(); }classes/Integration/Form/Elementor.php (2)
1-9: Missing ABSPATH guard (same as Forminator).Add the standard direct-access guard for consistency with
Newsletter.phpand WordPress conventions.🛡️ Proposed fix
<?php /** * Integration for Elementor Pro Forms * * `@package` PopupMaker * `@copyright` Copyright (c) 2024, Code Atlantic LLC */ +if ( ! defined( 'ABSPATH' ) ) { + exit; +} + class PUM_Integration_Form_Elementor extends PUM_Abstract_Integration_Form {
174-195: Add defensive check on$ajax_handler->get_current_form().Per coding guidelines, hook callbacks should use defensive validation for third-party plugin parameters. If Elementor Pro changes this API,
get_current_form()could return an unexpected type or not exist.🛡️ Proposed fix
public function on_success( $record, $ajax_handler ) { if ( ! $this->should_process_submission() ) { return; } // Get element_id to match form selector configuration. - $current_form = $ajax_handler->get_current_form(); - $element_id = isset( $current_form['id'] ) ? $current_form['id'] : null; + $element_id = null; + if ( is_object( $ajax_handler ) && method_exists( $ajax_handler, 'get_current_form' ) ) { + $current_form = $ajax_handler->get_current_form(); + $element_id = isset( $current_form['id'] ) ? $current_form['id'] : null; + } $popup_id = $this->get_popup_id();As per coding guidelines: "Never use strict PHP types on methods hooked to third-party actions/filters; use defensive validation instead" and "Use defensive validation in hook callbacks for graceful handling of third-party plugin parameter changes".
classes/Integration/Form/HappyForms.php (1)
1-9: Missing ABSPATH guard (same pattern as Forminator and Elementor).Add the standard direct-access guard for consistency.
🛡️ Proposed fix
<?php /** * Integration for HappyForms * * `@package` PopupMaker * `@copyright` Copyright (c) 2024, Code Atlantic LLC */ +if ( ! defined( 'ABSPATH' ) ) { + exit; +} + class PUM_Integration_Form_HappyForms extends PUM_Abstract_Integration_Form {assets/js/src/integration/elementor.js (1)
12-25:$formnaming is misleading — it holds a raw DOM element, not a jQuery object.
$( this )[ 0 ]unwraps to a DOM element, but the$prefix conventionally signals a jQuery object. Consider renaming toformorformElfor clarity, or pass the jQuery object directly if the API accepts it.♻️ Suggested rename
- const $form = $( this )[ 0 ]; + const formEl = $( this )[ 0 ]; // Get element_id from the widget container. // Elementor form widgets are inside a .elementor-element-{id} container. @@ -22,7 +22,7 @@ const elementId = $widget.length ? $widget.attr( 'data-id' ) : 'unknown'; - window.PUM.integrations.formSubmission( $form, { + window.PUM.integrations.formSubmission( formEl, { formProvider, formId: elementId, } );classes/Integrations.php (1)
54-54: Inconsistent array key casing:kaliFormsvs all-lowercase convention.All other integration keys use lowercase (
ninjaforms,gravityforms,happyforms,bitform, etc.), butkaliFormsuses camelCase. This inconsistency could cause issues if any code expects normalized lowercase keys..github/workflows/release-please.yml (2)
42-47: Minor: potential script injection intoJSONoutput.Using
${{ toJSON(steps.release.outputs) }}directly in arunblock is a known GitHub Actions anti-pattern. If any output contained a single quote, it could break out of the shell string. Consider using an environment variable instead.♻️ Safer logging approach
- name: Show Release Please outputs - run: | - echo "Release created: ${{ steps.release.outputs.release_created }}" - echo "Tag: ${{ steps.release.outputs.tag_name }}" - echo "PR: ${{ steps.release.outputs.pr }}" - echo "All outputs:" - echo '${{ toJSON(steps.release.outputs) }}' + env: + RELEASE_OUTPUTS: ${{ toJSON(steps.release.outputs) }} + run: | + echo "Release created: ${{ steps.release.outputs.release_created }}" + echo "Tag: ${{ steps.release.outputs.tag_name }}" + echo "PR: ${{ steps.release.outputs.pr }}" + echo "All outputs:" + echo "$RELEASE_OUTPUTS"
69-76: Consider scopinggit addto expected files only.
git add -Astages all changes in the working tree. Ifnpm run version:updateornpm cileaves unexpected artifacts, they'd be committed. Scoping to known version files would be safer.assets/js/src/integration/kaliForms.js (1)
43-82: Event listener accumulation on repeated submits.Each
submitevent adds a newkaliFormSuccesslistener (line 76). If the user submits the form multiple times beforekaliFormSuccessfires or the 30s timeout elapses, multiple listeners stack up. Each matchingkaliFormSuccessevent would then triggerformSubmissionfor every accumulated listener.Consider using
{ once: true }on theaddEventListenercall to auto-remove after first invocation, which also simplifies the manualremoveEventListenerlogic:♻️ Simplify with `once: true`
- document.addEventListener( 'kaliFormSuccess', successHandler ); + document.addEventListener( 'kaliFormSuccess', successHandler, { once: true } ); // Cleanup listener after 30 seconds if not fired. setTimeout( function () { document.removeEventListener( 'kaliFormSuccess', successHandler ); }, 30000 );Note: With
{ once: true }, the manualremoveEventListenerinsidesuccessHandler(lines 69-72) becomes unnecessary and can also be removed..github/workflows/commitlint.yml (2)
26-54: GitHub Actions script injection risk inrun:blocks.Lines 30 and 60 interpolate
${{ github.event.pull_request.base.sha }}and${{ github.event.pull_request.head.sha }}directly into shell scripts. While SHAs are hex-only and practically safe, the recommended pattern is to pass GitHub context values through environment variables to avoid script injection as a general habit.Proposed fix
- name: Check for AI attribution + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} run: | # Check commits for AI attribution if [[ "${{ github.event_name }}" == "pull_request" ]]; then - COMMITS=$(git log --format=%H ${{ github.event.pull_request.base.sha }}..${{ github.event.pull_request.head.sha }}) + COMMITS=$(git log --format=%H "$BASE_SHA".."$HEAD_SHA") else COMMITS=$(git log --format=%H HEAD~1..HEAD) fiThe same pattern should be applied to the "Validate commits" step on lines 56-64.
66-113: Failure comment is only created, never updated.If a contributor pushes multiple failing commits, the existing comment check (Line 84) prevents duplicates, which is good. However, the comment content becomes stale — it won't reflect the latest failure details. Consider updating the existing comment instead of skipping, so the developer always sees current guidance.
Also, the indentation of the template literal (Lines 89-112) will produce leading whitespace in the rendered Markdown comment. This may cause unintended code blocks or formatting issues in the PR comment.
assets/js/src/integration/newsletter.js (2)
63-65: Redundantwindow.PUMguard.These integration files are bundled with
window.PUM, so checking for its existence is unnecessary and creates dead branches. The same applies to the guard at Line 178.Proposed fix
const handleSuccess = ( container, popupId ) => { // Prevent duplicate handling. if ( container.dataset.pumNewsletterHandled ) { return; } container.dataset.pumNewsletterHandled = 'true'; - if ( ! window.PUM || ! window.PUM.integrations ) { - return; - } - window.PUM.integrations.formSubmission( $( container ), {Based on learnings: "In assets/js/src/integration/**/*.js, guards that check for window.PUM (e.g., conditional blocks that run only if window.PUM exists) are redundant because these integration files are bundled with window.PUM."
93-111: Observer sees its owndatasetmutation, risking an extra callback cycle.The observer watches the form with
subtree: true. Inside the callback (Line 105),delete form.dataset.pumNewsletterObservedmutates a child attribute of the form, which can trigger the observer again beforedisconnect()fully takes effect (sincedisconnectis called synchronously but pending mutation records may already be queued). This is unlikely to cause a visible bug due to thepumNewsletterHandledguard inhandleSuccess, but it's worth being aware of. Movingdisconnect()before the dataset mutation would be cleaner.commitlint.config.js (1)
39-73: Scope'build'and'docs'appear in bothtype-enumandscope-enum.Having
buildanddocsas both a type and a scope is intentional (e.g.,build(deps): bump lodashvsci(build): fix workflow), but it may confuse contributors. A brief note in the contributing guide would help clarify when to use each.classes/Integration/Form/BitForm.php (2)
165-168: Negative cache withnullmay not behave as intended withget_transient.On Line 150, you check
false !== $cachedto detect a cache hit. Storingnullviaset_transient(Line 167) will returnnullfromget_transienton hit, which passes thefalse !== $cachedcheck and returnsnull— this works correctly.However, if WordPress object caching drops the value (some backends treat
nullas a delete), the negative cache will silently fail, falling through to the DB query every time. Consider using a sentinel value like an empty array instead.Proposed fix
if ( ! $form ) { // Cache negative result for 1 hour to avoid repeated failed queries. - set_transient( $cache_key, null, HOUR_IN_SECONDS ); + set_transient( $cache_key, 'not_found', HOUR_IN_SECONDS ); return null; }Then update the cache check:
if ( false !== $cached ) { + return 'not_found' === $cached ? null : $cached; - return $cached; }
89-113:get_forms()hits the database on every call without caching.Unlike
get_form()which uses transient caching,get_forms()queries the database every time. If this is called from admin UI select lists or during page loads, it could add unnecessary DB queries. Consider adding transient caching here as well, similar to the pattern inget_form().
| - name: Set variables | ||
| id: vars | ||
| run: | | ||
| # Use release event or workflow_dispatch inputs | ||
| if [ "${{ github.event_name }}" = "release" ]; then | ||
| echo "version=${{ github.event.release.tag_name }}" >> $GITHUB_OUTPUT | ||
| echo "release_url=${{ github.event.release.html_url }}" >> $GITHUB_OUTPUT | ||
| echo "zip_url=${{ github.event.release.zipball_url }}" >> $GITHUB_OUTPUT | ||
| echo "dry_run=false" >> $GITHUB_OUTPUT | ||
|
|
||
| # Store release body for changelog extraction | ||
| cat << 'EOFBODY' > /tmp/release_body.txt | ||
| ${{ github.event.release.body }} | ||
| EOFBODY | ||
| else | ||
| VERSION="${{ inputs.version }}" | ||
| # Add prerelease suffix if checked | ||
| if [ "${{ inputs.prerelease }}" = "true" ]; then | ||
| VERSION="${VERSION}-beta.1" | ||
| fi | ||
| echo "version=$VERSION" >> $GITHUB_OUTPUT | ||
| echo "release_url=${{ github.server_url }}/${{ github.repository }}/releases/tag/$VERSION" >> $GITHUB_OUTPUT | ||
| echo "zip_url=${{ github.server_url }}/${{ github.repository }}/archive/refs/tags/$VERSION.zip" >> $GITHUB_OUTPUT | ||
| echo "dry_run=${{ inputs.dry_run }}" >> $GITHUB_OUTPUT | ||
|
|
||
| # Use input body | ||
| cat << 'EOFBODY' > /tmp/release_body.txt | ||
| ${{ inputs.release_body }} | ||
| EOFBODY | ||
| fi |
There was a problem hiding this comment.
Script injection risk: user-controlled release.body is interpolated directly into shell commands.
${{ github.event.release.body }} and ${{ inputs.release_body }} are expanded by GitHub Actions before the shell runs. If the content contains EOFBODY on its own line, it terminates the heredoc early and the remaining text executes as shell commands. This is a known GitHub Actions injection vector.
Use an environment variable instead of direct interpolation:
🔒 Proposed fix: use environment variables to avoid injection
- name: Set variables
id: vars
+ env:
+ RELEASE_BODY: ${{ github.event.release.body }}
+ INPUT_BODY: ${{ inputs.release_body }}
run: |
if [ "${{ github.event_name }}" = "release" ]; then
echo "version=${{ github.event.release.tag_name }}" >> $GITHUB_OUTPUT
echo "release_url=${{ github.event.release.html_url }}" >> $GITHUB_OUTPUT
echo "zip_url=${{ github.event.release.zipball_url }}" >> $GITHUB_OUTPUT
echo "dry_run=false" >> $GITHUB_OUTPUT
- cat << 'EOFBODY' > /tmp/release_body.txt
- ${{ github.event.release.body }}
- EOFBODY
+ printf '%s\n' "$RELEASE_BODY" > /tmp/release_body.txt
else
VERSION="${{ inputs.version }}"
if [ "${{ inputs.prerelease }}" = "true" ]; then
VERSION="${VERSION}-beta.1"
fi
echo "version=$VERSION" >> $GITHUB_OUTPUT
echo "release_url=${{ github.server_url }}/${{ github.repository }}/releases/tag/$VERSION" >> $GITHUB_OUTPUT
echo "zip_url=${{ github.server_url }}/${{ github.repository }}/archive/refs/tags/$VERSION.zip" >> $GITHUB_OUTPUT
echo "dry_run=${{ inputs.dry_run }}" >> $GITHUB_OUTPUT
- cat << 'EOFBODY' > /tmp/release_body.txt
- ${{ inputs.release_body }}
- EOFBODY
+ printf '%s\n' "$INPUT_BODY" > /tmp/release_body.txt
fi🤖 Prompt for AI Agents
In @.github/workflows/notify-support.yml around lines 36 - 65, The step with id
"vars" is vulnerable because it injects user-controlled values directly into
heredocs using ${{ github.event.release.body }} and ${{ inputs.release_body }};
instead, set those values as step environment variables (e.g., env:
RELEASE_BODY: ${{ github.event.release.body }} or RELEASE_BODY: ${{
inputs.release_body }} under the same step) and write them to
/tmp/release_body.txt using a safe shell write like printf '%s\n'
"$RELEASE_BODY" > /tmp/release_body.txt (repeat for the inputs branch using a
different env name), leaving all heredocs removed from the step to eliminate the
EOFBODY injection vector.
| - name: Send to #support-updates | ||
| if: steps.vars.outputs.dry_run != 'true' | ||
| env: | ||
| SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_SUPPORT }} | ||
| run: | | ||
| echo "📤 Sending support notification to Slack..." | ||
| curl -X POST "$SLACK_WEBHOOK_URL" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '${{ steps.payload.outputs.payload }}' | ||
| echo "" | ||
| echo "✅ Slack message sent to #support-updates!" |
There was a problem hiding this comment.
Slack curl payload may break on single quotes in the output.
'${{ steps.payload.outputs.payload }}' is single-quoted in shell, but ${{ }} is expanded by Actions before the shell. If the payload contains a ', the shell command breaks, potentially causing a partial send or error.
Safer approach: write the payload to a file and use curl -d @file``.
🔧 Proposed fix
- name: Send to `#support-updates`
if: steps.vars.outputs.dry_run != 'true'
env:
SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_SUPPORT }}
+ SLACK_PAYLOAD: ${{ steps.payload.outputs.payload }}
run: |
echo "📤 Sending support notification to Slack..."
+ printf '%s' "$SLACK_PAYLOAD" > /tmp/slack_payload.json
curl -X POST "$SLACK_WEBHOOK_URL" \
-H "Content-Type: application/json" \
- -d '${{ steps.payload.outputs.payload }}'
+ -d `@/tmp/slack_payload.json`
echo ""
echo "✅ Slack message sent to `#support-updates`!"📝 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.
| - name: Send to #support-updates | |
| if: steps.vars.outputs.dry_run != 'true' | |
| env: | |
| SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_SUPPORT }} | |
| run: | | |
| echo "📤 Sending support notification to Slack..." | |
| curl -X POST "$SLACK_WEBHOOK_URL" \ | |
| -H "Content-Type: application/json" \ | |
| -d '${{ steps.payload.outputs.payload }}' | |
| echo "" | |
| echo "✅ Slack message sent to #support-updates!" | |
| - name: Send to `#support-updates` | |
| if: steps.vars.outputs.dry_run != 'true' | |
| env: | |
| SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_SUPPORT }} | |
| SLACK_PAYLOAD: ${{ steps.payload.outputs.payload }} | |
| run: | | |
| echo "📤 Sending support notification to Slack..." | |
| printf '%s' "$SLACK_PAYLOAD" > /tmp/slack_payload.json | |
| curl -X POST "$SLACK_WEBHOOK_URL" \ | |
| -H "Content-Type: application/json" \ | |
| -d `@/tmp/slack_payload.json` | |
| echo "" | |
| echo "✅ Slack message sent to `#support-updates`!" |
🤖 Prompt for AI Agents
In @.github/workflows/notify-support.yml around lines 193 - 203, The current
curl invocation uses a single-quoted inline expansion of
steps.payload.outputs.payload which will break if the payload contains single
quotes; update the "Send to `#support-updates`" step to write the payload from
steps.payload.outputs.payload to a temporary file (e.g., payload.json) and call
curl with -d `@payload.json` using the SLACK_WEBHOOK_URL env var, then remove the
temp file afterwards; reference the step output name
steps.payload.outputs.payload and the env SLACK_WEBHOOK_URL when making the
change.
| - name: Set variables | ||
| id: vars | ||
| run: | | ||
| # Use dispatch payload or workflow_dispatch inputs | ||
| if [ "${{ github.event_name }}" = "repository_dispatch" ]; then | ||
| echo "version=${{ github.event.client_payload.version }}" >> $GITHUB_OUTPUT | ||
| echo "plugin=${{ github.event.client_payload.plugin || github.event.repository.name }}" >> $GITHUB_OUTPUT | ||
| echo "artifact=${{ github.event.client_payload.artifact }}" >> $GITHUB_OUTPUT | ||
| echo "dry_run=false" >> $GITHUB_OUTPUT | ||
| else | ||
| echo "version=${{ inputs.version }}" >> $GITHUB_OUTPUT | ||
| echo "plugin=${{ inputs.plugin }}" >> $GITHUB_OUTPUT | ||
| echo "artifact=test-artifact-${{ inputs.version }}" >> $GITHUB_OUTPUT | ||
| echo "dry_run=${{ inputs.dry_run }}" >> $GITHUB_OUTPUT | ||
| fi |
There was a problem hiding this comment.
Script injection risk: client_payload values are interpolated directly into shell.
${{ github.event.client_payload.version }}, .plugin, and .artifact are expanded by Actions before the shell runs. Anyone who can trigger repository_dispatch (e.g., via a compromised PAT) can inject arbitrary shell commands. Use environment variables instead.
🔒 Proposed fix
- name: Set variables
id: vars
+ env:
+ DISPATCH_VERSION: ${{ github.event.client_payload.version }}
+ DISPATCH_PLUGIN: ${{ github.event.client_payload.plugin || github.event.repository.name }}
+ DISPATCH_ARTIFACT: ${{ github.event.client_payload.artifact }}
run: |
if [ "${{ github.event_name }}" = "repository_dispatch" ]; then
- echo "version=${{ github.event.client_payload.version }}" >> $GITHUB_OUTPUT
- echo "plugin=${{ github.event.client_payload.plugin || github.event.repository.name }}" >> $GITHUB_OUTPUT
- echo "artifact=${{ github.event.client_payload.artifact }}" >> $GITHUB_OUTPUT
+ echo "version=$DISPATCH_VERSION" >> $GITHUB_OUTPUT
+ echo "plugin=$DISPATCH_PLUGIN" >> $GITHUB_OUTPUT
+ echo "artifact=$DISPATCH_ARTIFACT" >> $GITHUB_OUTPUT
echo "dry_run=false" >> $GITHUB_OUTPUT
else📝 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.
| - name: Set variables | |
| id: vars | |
| run: | | |
| # Use dispatch payload or workflow_dispatch inputs | |
| if [ "${{ github.event_name }}" = "repository_dispatch" ]; then | |
| echo "version=${{ github.event.client_payload.version }}" >> $GITHUB_OUTPUT | |
| echo "plugin=${{ github.event.client_payload.plugin || github.event.repository.name }}" >> $GITHUB_OUTPUT | |
| echo "artifact=${{ github.event.client_payload.artifact }}" >> $GITHUB_OUTPUT | |
| echo "dry_run=false" >> $GITHUB_OUTPUT | |
| else | |
| echo "version=${{ inputs.version }}" >> $GITHUB_OUTPUT | |
| echo "plugin=${{ inputs.plugin }}" >> $GITHUB_OUTPUT | |
| echo "artifact=test-artifact-${{ inputs.version }}" >> $GITHUB_OUTPUT | |
| echo "dry_run=${{ inputs.dry_run }}" >> $GITHUB_OUTPUT | |
| fi | |
| - name: Set variables | |
| id: vars | |
| env: | |
| DISPATCH_VERSION: ${{ github.event.client_payload.version }} | |
| DISPATCH_PLUGIN: ${{ github.event.client_payload.plugin || github.event.repository.name }} | |
| DISPATCH_ARTIFACT: ${{ github.event.client_payload.artifact }} | |
| run: | | |
| # Use dispatch payload or workflow_dispatch inputs | |
| if [ "${{ github.event_name }}" = "repository_dispatch" ]; then | |
| echo "version=$DISPATCH_VERSION" >> $GITHUB_OUTPUT | |
| echo "plugin=$DISPATCH_PLUGIN" >> $GITHUB_OUTPUT | |
| echo "artifact=$DISPATCH_ARTIFACT" >> $GITHUB_OUTPUT | |
| echo "dry_run=false" >> $GITHUB_OUTPUT | |
| else | |
| echo "version=${{ inputs.version }}" >> $GITHUB_OUTPUT | |
| echo "plugin=${{ inputs.plugin }}" >> $GITHUB_OUTPUT | |
| echo "artifact=test-artifact-${{ inputs.version }}" >> $GITHUB_OUTPUT | |
| echo "dry_run=${{ inputs.dry_run }}" >> $GITHUB_OUTPUT | |
| fi |
🤖 Prompt for AI Agents
In @.github/workflows/request-approval.yml around lines 29 - 43, The step "Set
variables" (id: vars) is vulnerable because expressions like ${{
github.event.client_payload.version }} are expanded into the shell; instead move
those expression evaluations into the step's env block (e.g., VERSION: ${{
github.event.client_payload.version }}, PLUGIN: ${{
github.event.client_payload.plugin || github.event.repository.name }}, ARTIFACT:
${{ github.event.client_payload.artifact }}, DRY_RUN: false) and then in the run
script write safe, already-expanded env vars to $GITHUB_OUTPUT (echo
"version=$VERSION" >> $GITHUB_OUTPUT, etc.); do the same for the
workflow_dispatch branch by exposing inputs via env and referencing those env
vars in run to eliminate direct expression interpolation into the shell.
| run: | | ||
| VERSION="${{ steps.vars.outputs.version }}" | ||
| PLUGIN="${{ steps.vars.outputs.plugin }}" | ||
| ARTIFACT="${{ steps.vars.outputs.artifact }}" | ||
| REPO="${{ github.event.repository.name }}" | ||
| DOWNLOAD_URL="${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}" | ||
| CHANGELOG=${{ steps.changelog.outputs.changelog }} | ||
| DRY_RUN="${{ steps.vars.outputs.dry_run }}" |
There was a problem hiding this comment.
Unquoted variable assignment will break on whitespace or special characters.
Line 91: CHANGELOG=${{ steps.changelog.outputs.changelog }} — this is expanded by Actions before the shell. Since the changelog output is a JSON-encoded string (with wrapping quotes from jq -Rs .), if it contains spaces or shell metacharacters outside the wrapping quotes, the assignment will be corrupted.
🔧 Proposed fix: use env variable or proper quoting
- name: Build Slack payload
id: payload
+ env:
+ CHANGELOG_JSON: ${{ steps.changelog.outputs.changelog }}
run: |
VERSION="${{ steps.vars.outputs.version }}"
PLUGIN="${{ steps.vars.outputs.plugin }}"
ARTIFACT="${{ steps.vars.outputs.artifact }}"
REPO="${{ github.event.repository.name }}"
DOWNLOAD_URL="${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}"
- CHANGELOG=${{ steps.changelog.outputs.changelog }}
+ CHANGELOG="$CHANGELOG_JSON"
DRY_RUN="${{ steps.vars.outputs.dry_run }}"🤖 Prompt for AI Agents
In @.github/workflows/request-approval.yml around lines 85 - 92, The CHANGELOG
assignment is unquoted and can break if the JSON string contains spaces or shell
metacharacters; update the workflow step that sets
VERSION/PLUGIN/ARTIFACT/REPO/DOWNLOAD_URL/CHANGELOG/DRY_RUN to either export
CHANGELOG via the GitHub Actions env context (e.g., add CHANGELOG: ${{
steps.changelog.outputs.changelog }} under env) or wrap the assignment value in
proper shell quoting so that CHANGELOG receives the full JSON string from
steps.changelog.outputs.changelog without word-splitting or globbing; adjust the
same pattern for any other multi-word outputs if needed.
| - name: Send Slack approval request | ||
| if: steps.vars.outputs.dry_run != 'true' | ||
| env: | ||
| SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_DEV }} | ||
| run: | | ||
| echo "📤 Sending approval request to Slack..." | ||
| curl -X POST "$SLACK_WEBHOOK_URL" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '${{ steps.payload.outputs.payload }}' | ||
| echo "" | ||
| echo "✅ Slack message sent!" |
There was a problem hiding this comment.
Same curl payload injection risk as notify-support.yml.
'${{ steps.payload.outputs.payload }}' will break if the payload contains single quotes. Write to a temp file and use curl -d @file`` instead, as recommended for the other workflow.
🤖 Prompt for AI Agents
In @.github/workflows/request-approval.yml around lines 185 - 195, The "Send
Slack approval request" workflow step injects the payload string directly into
curl using '${{ steps.payload.outputs.payload }}', which breaks on embedded
single quotes; instead, write the payload value from
steps.payload.outputs.payload to a temporary file (e.g., via echo or printf
redirected to a temp file) and call curl with -d `@tempfile` against
SLACK_WEBHOOK_URL, then remove the tempfile; update the step that references the
env SLACK_WEBHOOK_URL and the output variable steps.payload.outputs.payload to
use the temp-file approach to avoid shell quoting issues.
| function extractVersionContent( content, version ) { | ||
| // Escape version string to handle metacharacters like dots and plus signs | ||
| const escapedVersion = escapeRegex( version ); | ||
|
|
||
| // Support multiple version formats: ## v1.0.3, ## 1.0.3, etc. | ||
| // Handles both LF and CRLF line endings | ||
| const versionPattern = new RegExp( | ||
| `^## (?:v)?${ escapedVersion }(?:\\s*-\\s*[0-9]{4}-[0-9]{2}-[0-9]{2})?\\s*\\r?\\n([\\s\\S]*?)(?=\\r?\\n## |\\r?\\n?$)`, | ||
| 'm' | ||
| ); | ||
|
|
||
| const match = content.match( versionPattern ); | ||
| return match ? match[ 1 ].trim() : null; | ||
| } |
There was a problem hiding this comment.
Regex bug: non-greedy [\s\S]*? with multiline $ in the lookahead captures only the first line.
The lookahead (?=\r?\n## |\r?\n?$) with the m flag causes $ to match at the end of every line (not just end of string). Combined with the non-greedy [\s\S]*?, the regex stops at the first line boundary. This means extractVersionContent returns only the first line of a version's changelog instead of the entire section.
The same bug affects extractUnreleasedContent (line 83) and extractLatestVersion (line 114).
A simpler, correct approach is to use string splitting instead of a complex single-pass regex:
🐛 Proposed fix: use indexOf-based extraction
function extractVersionContent( content, version ) {
const escapedVersion = escapeRegex( version );
- const versionPattern = new RegExp(
- `^## (?:v)?${ escapedVersion }(?:\\s*-\\s*[0-9]{4}-[0-9]{2}-[0-9]{2})?\\s*\\r?\\n([\\s\\S]*?)(?=\\r?\\n## |\\r?\\n?$)`,
- 'm'
- );
-
- const match = content.match( versionPattern );
- return match ? match[ 1 ].trim() : null;
+ // Find the version heading
+ const headingPattern = new RegExp(
+ `^## (?:v)?${ escapedVersion }(?:\\s*-\\s*[0-9]{4}-[0-9]{2}-[0-9]{2})?\\s*$`,
+ 'm'
+ );
+ const headingMatch = headingPattern.exec( content );
+ if ( ! headingMatch ) {
+ return null;
+ }
+
+ // Find the start of content (after the heading line)
+ const startIdx = headingMatch.index + headingMatch[ 0 ].length;
+ // Advance past the newline
+ const contentStart = content.indexOf( '\n', startIdx );
+ if ( contentStart === -1 ) {
+ return null;
+ }
+
+ // Find the next ## heading
+ const nextHeading = content.indexOf( '\n## ', contentStart );
+ const sectionContent = nextHeading === -1
+ ? content.slice( contentStart + 1 )
+ : content.slice( contentStart + 1, nextHeading );
+
+ return sectionContent.trim() || null;
}Apply the same pattern to extractUnreleasedContent and extractLatestVersion.
🤖 Prompt for AI Agents
In `@bin/extract-changelog.js` around lines 58 - 71, The regex in
extractVersionContent (and similarly in extractUnreleasedContent and
extractLatestVersion) stops at the first line because the non-greedy [\s\S]*?
combined with the multiline flag makes the lookahead match end-of-line; replace
the regex-based single-pass extraction with an indexOf/substring approach:
locate the header start (e.g., find the "## v{version}" or "## Unreleased"
header using indexOf after escaping/version formatting), then find the next "##
" occurrence (or end of string) with indexOf and return the substring between
them trimmed. Update extractVersionContent, extractUnreleasedContent, and
extractLatestVersion to use this header-indexing logic instead of the complex
regex.
| public static function sanitize_event_data( $value ) { | ||
| // If already an array, return as-is. | ||
| if ( is_array( $value ) ) { | ||
| return $value; | ||
| } | ||
|
|
||
| // Decode JSON string to array (matches Pro's Request::parse_tracking_data). | ||
| if ( is_string( $value ) ) { | ||
| $decoded = json_decode( $value, true ); | ||
| return is_array( $decoded ) ? $decoded : []; | ||
| } | ||
|
|
||
| return []; | ||
| } |
There was a problem hiding this comment.
sanitize_event_data normalizes structure but doesn't sanitize values.
This method converts the input to an array but doesn't sanitize the individual values within it. If any downstream consumer (e.g., a pum_analytics_conversion action handler) stores or renders these values, they could contain unsanitized user input. Consider recursively applying sanitize_text_field to leaf values.
🛡️ Suggested hardening
public static function sanitize_event_data( $value ) {
// If already an array, return as-is.
if ( is_array( $value ) ) {
- return $value;
+ return array_map( 'sanitize_text_field', $value );
}
// Decode JSON string to array (matches Pro's Request::parse_tracking_data).
if ( is_string( $value ) ) {
$decoded = json_decode( $value, true );
- return is_array( $decoded ) ? $decoded : [];
+ return is_array( $decoded ) ? array_map( 'sanitize_text_field', $decoded ) : [];
}
return [];
}Note: If nested arrays are expected in eventData, use wp_kses_post or a recursive sanitization helper instead of a flat array_map.
📝 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.
| public static function sanitize_event_data( $value ) { | |
| // If already an array, return as-is. | |
| if ( is_array( $value ) ) { | |
| return $value; | |
| } | |
| // Decode JSON string to array (matches Pro's Request::parse_tracking_data). | |
| if ( is_string( $value ) ) { | |
| $decoded = json_decode( $value, true ); | |
| return is_array( $decoded ) ? $decoded : []; | |
| } | |
| return []; | |
| } | |
| public static function sanitize_event_data( $value ) { | |
| // If already an array, return as-is. | |
| if ( is_array( $value ) ) { | |
| return array_map( 'sanitize_text_field', $value ); | |
| } | |
| // Decode JSON string to array (matches Pro's Request::parse_tracking_data). | |
| if ( is_string( $value ) ) { | |
| $decoded = json_decode( $value, true ); | |
| return is_array( $decoded ) ? array_map( 'sanitize_text_field', $decoded ) : []; | |
| } | |
| return []; | |
| } |
🤖 Prompt for AI Agents
In `@classes/Analytics.php` around lines 179 - 192, sanitize_event_data currently
converts JSON or strings to arrays but leaves individual values unsanitized;
update sanitize_event_data to recursively sanitize leaf values before returning
(use sanitize_text_field for simple scalar leaves or wp_kses_post for
HTML-allowed fields as appropriate). Locate the sanitize_event_data function and
after decoding/accepting an array, apply a recursive sanitizer (e.g., implement
a private helper or use array_walk_recursive) to transform each scalar value,
then return the sanitized array; ensure handlers like pum_analytics_conversion
receive sanitized data.
| public function get_forms() { | ||
| return [ | ||
| [ | ||
| 'ID' => 'contact_any', | ||
| 'post_title' => __( 'Any Contact Form', 'popup-maker' ), | ||
| ], | ||
| [ | ||
| 'ID' => 'subscribe_any', | ||
| 'post_title' => __( 'Any Subscribe Form', 'popup-maker' ), | ||
| ], | ||
| [ | ||
| 'ID' => 'login_any', | ||
| 'post_title' => __( 'Any Login Form', 'popup-maker' ), | ||
| ], | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate the BeaverBuilder integration file and examine its content
find . -name "BeaverBuilder.php" -type f 2>/dev/null | head -5Repository: PopupMaker/Popup-Maker
Length of output: 110
🏁 Script executed:
# Look at the BeaverBuilder.php file to verify the actual implementation
cat -n classes/Integration/Form/BeaverBuilder.php | head -100Repository: PopupMaker/Popup-Maker
Length of output: 2776
🏁 Script executed:
# Check HTMLForms integration for comparison
cat -n classes/Integration/Form/HTMLForms.php | head -100Repository: PopupMaker/Popup-Maker
Length of output: 2906
🏁 Script executed:
# Check if there's a KaliForms integration
find . -name "KaliForms.php" -type f 2>/dev/nullRepository: PopupMaker/Popup-Maker
Length of output: 106
🏁 Script executed:
# Search for how get_forms() is used across the codebase
rg -n 'get_forms\(\)' --type=php -B2 -A5Repository: PopupMaker/Popup-Maker
Length of output: 21973
🏁 Script executed:
# Check for abstract Form class that might define the interface
find . -name "*.php" -type f | xargs grep -l "abstract class.*Form" 2>/dev/null | head -5Repository: PopupMaker/Popup-Maker
Length of output: 105
🏁 Script executed:
# Look at the abstract Form class to understand expected interface
rg -n "class Form" --type=php -A20 | head -100Repository: PopupMaker/Popup-Maker
Length of output: 1556
🏁 Script executed:
# Check the abstract Form class to understand if there's an interface contract
cat -n classes/Abstract/Integration/Form.phpRepository: PopupMaker/Popup-Maker
Length of output: 2637
🏁 Script executed:
# Verify how the integrations are actually used - check if there's a unified consumer of get_forms()
rg -n "get_form_selectlist\(\)" --type=php -B2 -A2 | head -60Repository: PopupMaker/Popup-Maker
Length of output: 3154
🏁 Script executed:
# Check if get_form() is used elsewhere and how it accesses the returned form
rg -n "get_form\(" --type=php -B2 -A2 | head -80Repository: PopupMaker/Popup-Maker
Length of output: 4395
🏁 Script executed:
# Look at how forms are used in the integration system - check Integrations.php around line 148
sed -n '140,160p' classes/Integrations.phpRepository: PopupMaker/Popup-Maker
Length of output: 626
Align get_forms() key format with other integrations and derive get_form_selectlist() from get_forms() to follow DRY principle.
BeaverBuilder returns ['ID' => ..., 'post_title' => ...] while HTMLForms, GravityForms, and BitForm return ['id' => ..., 'title' => ...]. This inconsistency makes the API unpredictable across integrations. Additionally, get_form_selectlist() is hardcoded separately from get_forms(), creating a maintenance burden if the form list changes.
🔧 Proposed fixes
public function get_forms() {
return [
[
- 'ID' => 'contact_any',
- 'post_title' => __( 'Any Contact Form', 'popup-maker' ),
+ 'id' => 'contact_any',
+ 'title' => __( 'Any Contact Form', 'popup-maker' ),
],
[
- 'ID' => 'subscribe_any',
- 'post_title' => __( 'Any Subscribe Form', 'popup-maker' ),
+ 'id' => 'subscribe_any',
+ 'title' => __( 'Any Subscribe Form', 'popup-maker' ),
],
[
- 'ID' => 'login_any',
- 'post_title' => __( 'Any Login Form', 'popup-maker' ),
+ 'id' => 'login_any',
+ 'title' => __( 'Any Login Form', 'popup-maker' ),
],
];
}Then update get_form() and derive get_form_selectlist():
public function get_form( $id ) {
$forms = $this->get_forms();
foreach ( $forms as $form ) {
- if ( $form['ID'] === $id ) {
+ if ( $form['id'] === $id ) {
return $form;
}
}
return null;
}
public function get_form_selectlist() {
- return [
- 'contact_any' => __( 'Any Contact Form', 'popup-maker' ),
- 'subscribe_any' => __( 'Any Subscribe Form', 'popup-maker' ),
- 'login_any' => __( 'Any Login Form', 'popup-maker' ),
- ];
+ $form_selectlist = [];
+ foreach ( $this->get_forms() as $form ) {
+ $form_selectlist[ $form['id'] ] = $form['title'];
+ }
+ return $form_selectlist;
}🤖 Prompt for AI Agents
In `@classes/Integration/Form/BeaverBuilder.php` around lines 47 - 62,
BeaverBuilder's get_forms() uses 'ID'/'post_title' instead of the standard
'id'/'title' and get_form_selectlist() is duplicated; change get_forms() to
return each form as ['id'=>..., 'title'=>...] to match other integrations,
update get_form() to read the new keys (use 'id' for lookups and return the same
normalized shape), and implement get_form_selectlist() to build its list by
iterating over get_forms() (mapping id=>title) so the select list is always
derived from get_forms(); ensure any code expecting legacy keys is adjusted or
mapped inside these methods to keep behavior consistent.
|
|
||
| if ( $popup_id ) { | ||
| $this->increase_conversion( $popup_id ); | ||
| } | ||
|
|
||
| // Defensive validation - $form is stdClass, not array. | ||
| $form_id = null; | ||
| if ( is_object( $form ) && isset( $form->attributes ) && is_object( $form->attributes ) && isset( $form->attributes->id ) ) { | ||
| $form_id = $form->attributes->id; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check the FluentForms.php file to understand context and existing form usage
echo "=== Checking FluentForms.php context ==="
cat -n classes/Integration/Form/FluentForms.php | head -120
# Search for any references to form properties in this file
echo -e "\n=== Searching for form property access patterns ==="
rg -n "form->" classes/Integration/Form/FluentForms.php
# Check if there are any comments or documentation about the form structure
echo -e "\n=== Searching for form-related comments ==="
rg -n -i "form|fluent" classes/Integration/Form/FluentForms.php | head -30Repository: PopupMaker/Popup-Maker
Length of output: 5569
🏁 Script executed:
#!/bin/bash
# Check how other form integrations handle form_id extraction in hook callbacks
echo "=== HappyForms.php ==="
rg -n -A5 "on_success|form.*id" classes/Integration/Form/HappyForms.php | head -40
echo -e "\n=== CalderaForms.php ==="
rg -n -A5 "on_success|form.*id" classes/Integration/Form/CalderaForms.php | head -40
# Check if there are any other references to Fluent Forms form structure
echo -e "\n=== Other form integration patterns ==="
fd -e php classes/Integration/Form/ --exec basename {} \;Repository: PopupMaker/Popup-Maker
Length of output: 2032
🏁 Script executed:
#!/bin/bash
# Search for any references or documentation about form structure
echo "=== Searching for form structure clues in codebase ==="
rg -i "attributes" classes/Integration/Form/ -A2 -B2
# Check if there are any tests or documentation files
echo -e "\n=== Looking for test or documentation files ==="
fd -e php -p test classes/Integration/Form/ 2>/dev/null || echo "No test files found"
# Search for any inline documentation about Fluent Forms
echo -e "\n=== Searching for Fluent Forms references ==="
rg -i "fluent" --include="*.php" -A3 -B3 | head -100Repository: PopupMaker/Popup-Maker
Length of output: 1168
🌐 Web query:
Fluent Forms fluentform/before_submission_confirmation hook documentation form parameter structure
💡 Result:
fluentform/before_submission_confirmation fires after Fluent Forms finishes processing integrations/notifications and is about to return the final AJAX confirmation response. The action is triggered like: do_action('fluentform/before_submission_confirmation', $insertId, $this->formData, $this->form); and your callback receives 3 params: ($entryId, $formData, $form). [1]
$form parameter (structure)
In this hook, $form is the Fluent Forms “Form object” (the form record + settings), with commonly-used properties like: [2]
$form->id(form ID)$form->title$form->status$form->form_fields(raw JSON string of the form field definitions)$form->has_payment$form->type$form->conditions$form->created_by,$form->created_at,$form->updated_at$form->settings(array), which includes keys such as:confirmation(e.g.redirectTo,messageToShow,samePageFormBehavior, etc.)restrictionslayout
(Example JSON for the object is shown in the Form Object doc.) [2]
Quick way to inspect the exact shape on your site
add_action('fluentform/before_submission_confirmation', function($entryId, $formData, $form) {
error_log(print_r($form, true)); // inspect full object
$fields = json_decode($form->form_fields, true); // field definitions array
}, 20, 3);Sources: Fluent Forms hook doc for fluentform/before_submission_confirmation [1], Fluent Forms “$form Object” data definition (properties + example JSON) [2].
[1] (fluentforms.com)
[2] (fluentforms.com)
Citations:
- 1: https://fluentforms.com/docs/fluentform_before_submission_confirmation/?utm_source=openai
- 2: https://fluentforms.com/docs/form-object/
Incorrect form ID extraction path — fix to use $form->id instead of $form->attributes->id.
The defensive validation structure is sound, but it checks the wrong property. According to Fluent Forms' official documentation for the fluentform/before_submission_confirmation hook, the form object exposes the ID directly at $form->id, not $form->attributes->id. The code at line 83 already demonstrates this pattern ($form->id and $form->title).
With the current implementation, $form_id will silently remain null even when a valid form ID exists, causing form submission tracking to fail silently.
Replace line 109–110:
if ( is_object( $form ) && isset( $form->id ) ) {
$form_id = $form->id;
}🤖 Prompt for AI Agents
In `@classes/Integration/Form/FluentForms.php` around lines 102 - 111, The code in
classes/Integration/Form/FluentForms.php is extracting the form id from the
wrong property (attributes->id) so $form_id stays null; inside the FluentForms
integration (the method handling the fluentform/before_submission_confirmation
hook where $form is provided) replace the defensive check that looks for
$form->attributes->id with a check for $form->id (e.g., if (is_object($form) &&
isset($form->id)) { $form_id = $form->id; }) so the actual form ID is captured
for conversion tracking.
| // Verify popup exists before tracking (prevents orphaned meta). | ||
| $popup = get_post( $popup_id ); | ||
| if ( ! $popup || 'popup' !== get_post_type( $popup ) ) { | ||
| if ( defined( 'WP_DEBUG' ) && WP_DEBUG ) { | ||
| error_log( sprintf( '[Popup Maker] Skipping link click tracking for invalid popup ID: %d', $popup_id ) ); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use pum_get_popup() / pum_is_popup() instead of get_post() / get_post_type() for popup validation.
FormConversionTracking (lines 86-87) uses pum_get_popup() + pum_is_popup(), but this service uses raw get_post() + get_post_type(). This is inconsistent and bypasses Popup Maker's popup validation API, which also checks $popup->is_valid().
♻️ Proposed fix
- // Verify popup exists before tracking (prevents orphaned meta).
- $popup = get_post( $popup_id );
- if ( ! $popup || 'popup' !== get_post_type( $popup ) ) {
+ // Verify popup exists before tracking (prevents orphaned meta).
+ $popup = pum_get_popup( $popup_id );
+ if ( ! pum_is_popup( $popup ) ) {As per coding guidelines: "Use the pum_get_popup($popup_id) … functions to get popup and theme objects rather than directly querying post meta."
🤖 Prompt for AI Agents
In `@classes/Services/LinkClickTracking.php` around lines 84 - 91, Replace the raw
post checks using get_post() and get_post_type() with Popup Maker helpers: call
pum_get_popup($popup_id) to retrieve the popup object and use
pum_is_popup($popup) (and/or $popup->is_valid()) to validate it before tracking;
update the variable $popup to hold the pum_get_popup() result and change the
conditional that currently checks get_post_type() to use pum_is_popup($popup) so
the same validation logic as FormConversionTracking is used.
|
Closing — this PR is |
3 participants
Description
Related Issue: https://github.com//issues/1175
Fixes: Fixes #1175
Types of changes
What I changed
[classes/Admin/Popups.php
add_popup_filters()method (lines ~1590-1636)]Tests
new unit tests that assert:
Types of changes
✅ Bug fix (non-breaking change)
Screenshots
https://jam.dev/c/34f02751-a401-4081-9a9b-7b999be17cb7
This has been tested in the following browsers
Merge Checklist