You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.
Use the following commands to manage reviews:
@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.
Use the checkboxes below for quick actions:
▶️ Resume reviews
🔍 Trigger review
📝 Walkthrough
Walkthrough
Adds a gated-content form action with token generation/storage/validation, DB table and migrations, admin UI/JS and styles, shortcode rendering, query hooks to unlock private/passworded posts, hook wiring, and unit/integration tests.
Changes
Gated Content Form Action Feature
Layer / File(s)
Summary
Core Data Models and Action classes/models/FrmGatedItem.php, classes/models/FrmGatedToken.php, classes/models/FrmGatedContentAction.php
Value objects for gated items and tokens, plus FrmGatedContentAction implementing the gated_content form action, post selectors, shortcodes, and settings sanitization.
Token Generation & Validation System classes/helpers/FrmGatedTokenHelper.php
Generates 32-character raw tokens, hashes and persists rows, manages per-request/transient caches, validates tokens via URL→cookie→user→filter chain, handles cookies and membership caching, and provides cleanup.
Adds frm_gated_tokens table and bumps plugin DB version to 106; uninstall drops the table.
Post Access Control & Query Filtering classes/controllers/FrmGatedContentController.php
Includes private posts in main singular queries when a token validates, unlocks password-protected posts for a validated token (bypasses password check only for that post), strips access_code via redirect, and forces 404 for unauthorized private posts; clears caches on action updates/deletes.
Admin UI for configuring gated items, item row template, shortcode reference, autocomplete for large post lists, and styles for layout and copy controls.
Client-side Admin Logic js/src/admin/admin.js
Manages dynamic gated-content item rows: type activation, reindexing, deterministic per-row IDs, file-field filtering, and shortcode copy UX with fallback.
Tests that action triggers generate tokens and respect configured events (e.g., payment-success).
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested labels
run tests
Suggested reviewers
garretlaxton
Crabcyborg
Poem
🐰 I hopped through tokens, keys in paw,
I tucked a shortcode by the door,
Private posts now find a way,
Links and tokens lead the day,
A rabbit cheers the code once more.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name
Status
Explanation
Resolution
Title check
❓ Inconclusive
The title 'Rock gated content' is vague and generic, using unclear phrasing that does not convey specific information about the changeset despite implementing substantial gated content functionality.
Consider revising the title to be more descriptive, such as 'Add gated content feature with token validation' or 'Implement gated content access control system' to better communicate the main changes.
✅ Passed checks (4 passed)
Check name
Status
Explanation
Description Check
✅ Passed
Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage
✅ Passed
Docstring coverage is 95.51% which is sufficient. The required threshold is 80.00%.
Linked Issues check
✅ Passed
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
✅ Passed
Check skipped because no linked issues were found for this pull request.
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches🧪 Generate unit tests (beta)
Create PR with unit tests
Commit unit tests in branch rock-gated-content
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.
We reviewed changes in c9a068f...2979f16 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.
Some issues found as part of this review are outside of the diff in this pull request and aren't shown in the inline review comments due to GitHub's API limitations. You can see those issues on the DeepSource dashboard.
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
The reason will be displayed to describe this comment to others. Learn more.
`shortcode` has a cyclomatic complexity of 19 with "High" risk
A function with high cyclomatic complexity can be hard to understand and
maintain. Cyclomatic complexity is a software metric that measures the number of
independent paths through a function. A higher cyclomatic complexity indicates
that the function has more decision points and is more complex.
Check the new table before clearing the install warning.
On upgrades, $this->forms already exists before create_tables() runs, so Line 154 returns early even if frm_gated_tokens failed to create. That leaves gated content broken without surfacing the inbox error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@classes/models/FrmMigrate.php` around lines 149 - 158, The
check_that_tables_exist() function currently only verifies $this->forms and
returns early, which can hide failures creating the new frm_gated_tokens table;
update this function so it verifies both the existing forms table and the new
frm_gated_tokens table (or calls create_tables() / a helper that confirms the
new table exists) before dismissing the inbox warning. Specifically, in
check_that_tables_exist() ensure you query for the frm_gated_tokens table name
in addition to $this->forms (or use a helper like create_tables()’s post-check)
and only call (new FrmInbox())->dismiss('failed-to-create-tables') after both
tables are confirmed present; if the gated tokens table is missing, do not
dismiss and surface the error instead.
Tighten negative-path assertions to expected return values.
At Line 143, Line 161, Line 187, Line 197, Line 212, and Line 247, assertNotInstanceOf is too broad and can pass on wrong return types. For these tests, assert null/false explicitly to enforce the method contract.
Also applies to: 161-164, 187-190, 197-200, 212-212, 247-247
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/phpunit/helpers/test_FrmGatedTokenHelper.php` around lines 143 - 146,
The tests currently use assertNotInstanceOf(\FrmGatedToken::class,
FrmGatedTokenHelper::validate_access_code(...)), which is too broad; update each
negative-path assertion that calls FrmGatedTokenHelper::validate_access_code to
assert the exact expected falsey return (use assertNull(...) or assertFalse(...)
according to the method contract) instead of assertNotInstanceOf, and do the
same for any other places in this test file asserting negative paths against
\FrmGatedToken::class so the test enforces the precise return value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@classes/controllers/FrmGatedContentController.php`:
- Around line 239-240: The trigger() method currently passes whatever $action is
directly to FrmGatedTokenHelper::generate(), but generate() expects a WP_Post;
update trigger() to validate and tighten the type of $action before calling
FrmGatedTokenHelper::generate(): check that $action is an instance of WP_Post
(using instanceof) and only call FrmGatedTokenHelper::generate($action, $entry,
$event) when that check passes (otherwise return early or bail out and
optionally log/debug). Ensure you reference the trigger() method and
FrmGatedTokenHelper::generate() when making the change so the validation is
colocated with the call.
In `@classes/helpers/FrmGatedTokenHelper.php`:
- Around line 110-118: The code currently caches and returns $raw_token even if
$wpdb->insert( $wpdb->prefix . 'frm_gated_tokens', $data, $format ) fails;
change the logic to check the insert result (e.g. $inserted =
$wpdb->insert(...)) and only execute self::$generated_tokens[ $action_id ] =
$raw_token, set_transient( self::get_token_transient_key( $action_id ),
$raw_token, 5 * MINUTE_IN_SECONDS ), and return $raw_token when the insert
succeeded; if the insert failed, return an error indicator (WP_Error or false)
or throw so callers can detect the failure instead of receiving an unusable
token.
- Around line 61-67: The code always persists the visitor IP (uses
FrmAppHelper::get_ip_address() when building $data) and always sets a browser
cookie later in FrmGatedTokenHelper, which bypasses the plugin's privacy
opt-outs; wrap both the IP assignment and the cookie-setting block in
conditional checks against the plugin's existing privacy flags (use whatever
existing helpers on FrmAppHelper expose the "no IPs" and "no GDPR cookies"
settings) so that you only add 'ip_address' to $data and call setcookie() when
the respective privacy setting allows it; update the token-creation path that
builds $data (the token_hash/action_id/entry_id/ip_address/created_at array) and
the later cookie-setting code in FrmGatedTokenHelper to respect those flags.
- Around line 381-390: The setcookie() call in FrmGatedTokenHelper uses the PHP
7.3+ array-options overload which breaks PHP 7.0–7.2; replace it with the legacy
setcookie signature setcookie($cookie_name, $raw_token, $expiry, $path, $domain,
$secure, $httponly) (use '/' for $path, determine $domain as needed or ''), and
then add the SameSite=Lax attribute by sending a Set-Cookie header (e.g. build
the cookie string including SameSite=Lax) so that $cookie_name, $raw_token,
$expiry and is_ssl() are used and behavior remains identical on PHP 7.0–7.2.
In `@classes/models/FrmGatedContentAction.php`:
- Around line 284-289: After calling
apply_filters('frm_gated_content_sanitize_item', $item, compact('raw_item')),
don't unconditionally drop entries with an empty $item['id']; change the guard
so only items that genuinely require an ID are skipped. Replace the
unconditional if ( empty($item['id']) ) continue; with a check that preserves
sanitized, non-ID item types by allowing items that provide a type-specific
payload (e.g., a non-empty $item['type'] or other payload keys set by the
sanitizer) and only continuing when the item both lacks an id and lacks any
type-specific data; reference the apply_filters call, the $item/$raw_item
variables and the empty($item['id']) check to locate and update the logic.
- Around line 195-219: The filtered query args from
apply_filters('frm_gated_content_posts_query', $query_args) can be changed to
return IDs (fields => 'ids'), causing get_posts($query_args) to return ints and
the closure in array_filter (which dereferences $p->post_status) to fatally
fail; after the filter but before calling get_posts, enforce WP_Post results by
forcing/normalizing the fields param back to return full objects (e.g. set
'fields' => 'all' or remove any 'fields' override) or validate the results from
get_posts() and filter only WP_Post instances before accessing post_status
(reference $query_args, get_posts, and the static closure). Ensure the change is
made in FrmGatedContentAction around the apply_filters -> get_posts ->
array_filter sequence.
In `@js/src/admin/admin.js`:
- Around line 11524-11535: showCopied currently reads/writes only the plain
"href" attribute so buttons that use the xlink namespace lose their original
icon on restore; update showCopied to use the existing getSVGHref helper to read
the full namespaced href and restore it with setAttributeNS using the same
namespace and localName (so you capture namespaceURI and localName from
getSVGHref and call setAttributeNS(namespaceURI, localName, originalHref) when
swapping back), and likewise set the temporary checkmark using setAttributeNS
with the proper namespace/localName.
- Around line 11559-11574: The navigator.clipboard.writeText call in the click
handler for copy buttons (identify via copyBtn and dataset.frmCopy) lacks a
rejection handler; update the code around navigator.clipboard.writeText( text
).then( () => showCopied( copyBtn ) ) to add a .catch() that performs the same
fallback copy logic used in the else branch (create hidden textarea, set value,
append, select, document.execCommand('copy'), remove) and then calls
showCopied(copyBtn); optionally log the error before falling back to aid
debugging. Ensure the fallback code is reused (extractable) so both the .catch
and the existing else branch invoke the same operation.
In `@tests/phpunit/helpers/test_FrmGatedTokenHelper.php`:
- Around line 296-307: The test is wiping all callbacks for
'frm_obtain_gated_token' which can break other tests; instead capture the
specific closure you register (assign the static function to a $callback
variable and pass that to add_filter) and remove only that callback with
remove_filter('frm_obtain_gated_token', $callback, 10) in a finally block so
cleanup always runs; keep the surrounding calls to
FrmGatedTokenHelper::get_valid_token and FrmGatedItem::make and call
$this->reset_helper_caches() as needed.
In `@tests/phpunit/misc/test_FrmGatedContentController.php`:
- Around line 105-128: The test creates the entry before inserting the form
action which can prime an empty frm_actions cache and make
FrmFormActionsController::trigger_actions('payment-success', $form_id,
$entry_id) skip the action; fix by inserting the action (wp_insert_post(...)
that creates the 'frm_form_actions' post) before creating the entry
($this->factory->entry->create(...)) or, if you must keep the order, explicitly
clear/refresh the frm_actions cache for that form (so trigger_actions sees the
newly inserted action) between wp_insert_post(...) and
FrmFormActionsController::trigger_actions(...).
---
Outside diff comments:
In `@classes/models/FrmMigrate.php`:
- Around line 149-158: The check_that_tables_exist() function currently only
verifies $this->forms and returns early, which can hide failures creating the
new frm_gated_tokens table; update this function so it verifies both the
existing forms table and the new frm_gated_tokens table (or calls
create_tables() / a helper that confirms the new table exists) before dismissing
the inbox warning. Specifically, in check_that_tables_exist() ensure you query
for the frm_gated_tokens table name in addition to $this->forms (or use a helper
like create_tables()’s post-check) and only call (new
FrmInbox())->dismiss('failed-to-create-tables') after both tables are confirmed
present; if the gated tokens table is missing, do not dismiss and surface the
error instead.
---
Nitpick comments:
In `@tests/phpunit/helpers/test_FrmGatedTokenHelper.php`:
- Around line 143-146: The tests currently use
assertNotInstanceOf(\FrmGatedToken::class,
FrmGatedTokenHelper::validate_access_code(...)), which is too broad; update each
negative-path assertion that calls FrmGatedTokenHelper::validate_access_code to
assert the exact expected falsey return (use assertNull(...) or assertFalse(...)
according to the method contract) instead of assertNotInstanceOf, and do the
same for any other places in this test file asserting negative paths against
\FrmGatedToken::class so the test enforces the precise return value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b266ded2-0b95-4afe-aab6-a6516f582f30
📥 Commits
Reviewing files that changed from the base of the PR and between f42ed06 and 0ee2321.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Tighten trigger() action type before calling token generation.
FrmGatedTokenHelper::generate() expects a WP_Post, but trigger() forwards any object. This is currently tripping static analysis and can break when non-WP_Post payloads are dispatched.
Suggested fix
- * `@param` object $action Form action post object (post_excerpt = 'gated_content').+ * `@param` WP_Post $action Form action post object (post_excerpt = 'gated_content').
@@
public static function trigger( $action, $entry, $form, $event ) {
+ if ( ! $action instanceof WP_Post ) {+ return;+ }
FrmGatedTokenHelper::generate( $action, $entry, $event );
}
[warning] 239-239: Avoid unused parameters such as '$form'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@classes/controllers/FrmGatedContentController.php` around lines 239 - 240,
The trigger() method currently passes whatever $action is directly to
FrmGatedTokenHelper::generate(), but generate() expects a WP_Post; update
trigger() to validate and tighten the type of $action before calling
FrmGatedTokenHelper::generate(): check that $action is an instance of WP_Post
(using instanceof) and only call FrmGatedTokenHelper::generate($action, $entry,
$event) when that check passes (otherwise return early or bail out and
optionally log/debug). Ensure you reference the trigger() method and
FrmGatedTokenHelper::generate() when making the change so the validation is
colocated with the call.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Gate both IP and cookie persistence behind the existing privacy settings.
Line 65 always stores an IP address, and Lines 381-390 always set a browser cookie. That bypasses the plugin's existing no-IPs / no-GDPR-cookies settings and introduces a privacy regression for sites that already opted out of those behaviors.
Also applies to: 377-391
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@classes/helpers/FrmGatedTokenHelper.php` around lines 61 - 67, The code
always persists the visitor IP (uses FrmAppHelper::get_ip_address() when
building $data) and always sets a browser cookie later in FrmGatedTokenHelper,
which bypasses the plugin's privacy opt-outs; wrap both the IP assignment and
the cookie-setting block in conditional checks against the plugin's existing
privacy flags (use whatever existing helpers on FrmAppHelper expose the "no IPs"
and "no GDPR cookies" settings) so that you only add 'ip_address' to $data and
call setcookie() when the respective privacy setting allows it; update the
token-creation path that builds $data (the
token_hash/action_id/entry_id/ip_address/created_at array) and the later
cookie-setting code in FrmGatedTokenHelper to respect those flags.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Don't publish a token unless the insert succeeded.
If Line 110 fails, Lines 113-116 still cache and return the raw token. That produces access links which can never validate and hides the DB failure from the caller.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@classes/helpers/FrmGatedTokenHelper.php` around lines 110 - 118, The code
currently caches and returns $raw_token even if $wpdb->insert( $wpdb->prefix .
'frm_gated_tokens', $data, $format ) fails; change the logic to check the insert
result (e.g. $inserted = $wpdb->insert(...)) and only execute
self::$generated_tokens[ $action_id ] = $raw_token, set_transient(
self::get_token_transient_key( $action_id ), $raw_token, 5 * MINUTE_IN_SECONDS
), and return $raw_token when the insert succeeded; if the insert failed, return
an error indicator (WP_Error or false) or throw so callers can detect the
failure instead of receiving an unusable token.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In which PHP version was the setcookie($name, $value, array $options) signature introduced?
💡 Result:
The alternative signature for the setcookie function, which supports an options array (setcookie(string $name, string $value = "", array $options = [])), was introduced in PHP 7.3.0 [1][2]. This signature was added to provide a more flexible way to set cookie parameters, including support for the SameSite cookie attribute [1][2].
# Check for composer.json to see PHP version requirements
find . -name "composer.json" -type f | head -5
Repository: Strategy11/formidable-forms
Length of output: 87
🏁 Script executed:
# Also search for PHP minimum version in common places
grep -r "requires.*php\|php.*require". --include="*.json" --include="*.txt" --include="*.md" --include="*.php" -i | grep -i "version\|require"| head -20
Repository: Strategy11/formidable-forms
Length of output: 3226
🏁 Script executed:
cat composer.json
Repository: Strategy11/formidable-forms
Length of output: 1446
🏁 Script executed:
# Check the plugin file header and nearby lines for PHP version info
head -50 classes/helpers/FrmGatedTokenHelper.php
Repository: Strategy11/formidable-forms
Length of output: 1408
🏁 Script executed:
# Find the main plugin file
find . -maxdepth 2 -name "*.php" -type f | grep -E "formidable|index"| head -10
Repository: Strategy11/formidable-forms
Length of output: 88
🏁 Script executed:
# Check for readme files
find . -maxdepth 2 -name "readme.*" -type f
Repository: Strategy11/formidable-forms
Length of output: 96
🏁 Script executed:
head -50 formidable.php
Repository: Strategy11/formidable-forms
Length of output: 1433
🏁 Script executed:
grep -A5 -B5 "php\|require\|version" readme.txt | head -60
# Also check the beginning of readme.txt for standard header fields
head -100 readme.txt | grep -i "requires\|php\|version"
Repository: Strategy11/formidable-forms
Length of output: 2136
This setcookie() signature is incompatible with the plugin's PHP 7.0 minimum requirement.
The array-options overload was added in PHP 7.3.0, but the plugin declares PHP 7.0+ as supported. On PHP 7.0–7.2, this call will fail silently or raise a fatal error, breaking gated access persistence on supported runtimes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@classes/helpers/FrmGatedTokenHelper.php` around lines 381 - 390, The
setcookie() call in FrmGatedTokenHelper uses the PHP 7.3+ array-options overload
which breaks PHP 7.0–7.2; replace it with the legacy setcookie signature
setcookie($cookie_name, $raw_token, $expiry, $path, $domain, $secure, $httponly)
(use '/' for $path, determine $domain as needed or ''), and then add the
SameSite=Lax attribute by sending a Set-Cookie header (e.g. build the cookie
string including SameSite=Lax) so that $cookie_name, $raw_token, $expiry and
is_ssl() are used and behavior remains identical on PHP 7.0–7.2.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Don't drop non-ID item types after the sanitize filter.
The extension point at Line 284 exists so add-ons can persist type-specific item payloads, but Line 287 still discards anything with an empty id. That makes non-post item types impossible to save even after they sanitize themselves.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@classes/models/FrmGatedContentAction.php` around lines 284 - 289, After
calling apply_filters('frm_gated_content_sanitize_item', $item,
compact('raw_item')), don't unconditionally drop entries with an empty
$item['id']; change the guard so only items that genuinely require an ID are
skipped. Replace the unconditional if ( empty($item['id']) ) continue; with a
check that preserves sanitized, non-ID item types by allowing items that provide
a type-specific payload (e.g., a non-empty $item['type'] or other payload keys
set by the sanitizer) and only continuing when the item both lacks an id and
lacks any type-specific data; reference the apply_filters call, the
$item/$raw_item variables and the empty($item['id']) check to locate and update
the logic.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file and examine the showCopied functioncd /tmp/repo 2>/dev/null ||cd /home/user/repo 2>/dev/null ||pwd# Get the actual code at lines 11524-11535
sed -n '11524,11535p' js/src/admin/admin.js
Repository: Strategy11/formidable-forms
Length of output: 532
🏁 Script executed:
# Check if getSVGHref function exists and what it does
sed -n '8528,8530p' js/src/admin/admin.js
Repository: Strategy11/formidable-forms
Length of output: 217
🏁 Script executed:
# Search for xlink:href usage in the file to see if it's actually used
rg 'xlink:href' js/src/admin/admin.js -A 2 -B 2
Repository: Strategy11/formidable-forms
Length of output: 188
🏁 Script executed:
# Also search for both href and xlink:href patterns to understand current usage
rg 'getAttribute.*href|setAttributeNS.*href|xlink' js/src/admin/admin.js | head -30
Repository: Strategy11/formidable-forms
Length of output: 1372
Preserve the original SVG href namespace when swapping the copy icon.
This helper only reads/writes href attributes. The codebase already uses xlink:href in multiple places (see other SVG icon handling in the file), so buttons rendered with xlink:href will fail to restore their original icon after the timeout—the attribute will revert to null instead.
Use the existing getSVGHref helper and setAttributeNS to handle both namespaces:
‼️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.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/src/admin/admin.js` around lines 11524 - 11535, showCopied currently
reads/writes only the plain "href" attribute so buttons that use the xlink
namespace lose their original icon on restore; update showCopied to use the
existing getSVGHref helper to read the full namespaced href and restore it with
setAttributeNS using the same namespace and localName (so you capture
namespaceURI and localName from getSVGHref and call setAttributeNS(namespaceURI,
localName, originalHref) when swapping back), and likewise set the temporary
checkmark using setAttributeNS with the proper namespace/localName.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the specific file to verify the code matches
sed -n '11559,11574p' js/src/admin/admin.js | cat -n
Repository: Strategy11/formidable-forms
Length of output: 793
🏁 Script executed:
# Search for promise error handling patterns in the admin.js file
rg '\.catch\(' js/src/admin/admin.js | head -20
Repository: Strategy11/formidable-forms
Length of output: 53
🏁 Script executed:
# Also check for .then patterns to see common style
rg '\.then\(' js/src/admin/admin.js -A 2 | head -40
Repository: Strategy11/formidable-forms
Length of output: 398
🏁 Script executed:
# Check if there are other promise-based operations without error handling
rg 'writeText|fetch|Promise' js/src/admin/admin.js -B 2 -A 2
Repository: Strategy11/formidable-forms
Length of output: 467
🏁 Script executed:
# Look for error handling patterns more broadly
rg '\.catch|\.then.*catch|try.*catch' js/src/admin/admin.js | head -20
Repository: Strategy11/formidable-forms
Length of output: 53
🏁 Script executed:
# Let's see broader context around the clipboard code
sed -n '11550,11580p' js/src/admin/admin.js | cat -n
Repository: Strategy11/formidable-forms
Length of output: 1312
🏁 Script executed:
# Check for any error/promise handling documentation or patterns in the codebase
find . -name "*.md" -o -name "*.txt"| xargs grep -l "error\|promise\|catch"2>/dev/null | head -10
Repository: Strategy11/formidable-forms
Length of output: 532
🏁 Script executed:
# Let's also check if there's a showCopied function to understand the complete flow
rg 'function showCopied|const showCopied' js/src/admin/admin.js -B 2 -A 5
Repository: Strategy11/formidable-forms
Length of output: 385
🏁 Script executed:
# Let's check the JavaScript coding guidelines mentioned in the windsurf rules
cat ./.windsurf/rules/formidable/frm-javascript.md | head -100
Repository: Strategy11/formidable-forms
Length of output: 2168
🏁 Script executed:
# Continue reading the JavaScript guidelines for promise/error handling
sed -n '100,200p' ./.windsurf/rules/formidable/frm-javascript.md
Repository: Strategy11/formidable-forms
Length of output: 1914
🏁 Script executed:
# Search for promise, async, error handling in the JavaScript guidelines
grep -n -i "promise\|async\|error\|catch\|then\|reject" ./.windsurf/rules/formidable/frm-javascript.md
Repository: Strategy11/formidable-forms
Length of output: 2249
🏁 Script executed:
# Get the full error handling section
sed -n '604,660p' ./.windsurf/rules/formidable/frm-javascript.md
Repository: Strategy11/formidable-forms
Length of output: 1438
🏁 Script executed:
# Let's also verify the Async/Await section to see if that's the preferred pattern
sed -n '561,585p' ./.windsurf/rules/formidable/frm-javascript.md
Repository: Strategy11/formidable-forms
Length of output: 580
🏁 Script executed:
# Let's check if there are other clipboard operations in the codebase that might handle this differently
rg 'clipboard|writeText' js/src/ -B 2 -A 5
Repository: Strategy11/formidable-forms
Length of output: 2917
🏁 Script executed:
# Let's see more context around the clipboard code in admin.js to understand the full event listener
sed -n '11540,11590p' js/src/admin/admin.js | cat -n
Repository: Strategy11/formidable-forms
Length of output: 2074
Add error handling for Clipboard API rejection.
The navigator.clipboard.writeText() call lacks a .catch() handler. If the promise rejects due to denied permissions or non-secure context, the code fails silently without falling back to the textarea copy. Add promise rejection handling to attempt the fallback copy method.
‼️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.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/src/admin/admin.js` around lines 11559 - 11574, The
navigator.clipboard.writeText call in the click handler for copy buttons
(identify via copyBtn and dataset.frmCopy) lacks a rejection handler; update the
code around navigator.clipboard.writeText( text ).then( () => showCopied(
copyBtn ) ) to add a .catch() that performs the same fallback copy logic used in
the else branch (create hidden textarea, set value, append, select,
document.execCommand('copy'), remove) and then calls showCopied(copyBtn);
optionally log the error before falling back to aid debugging. Ensure the
fallback code is reused (extractable) so both the .catch and the existing else
branch invoke the same operation.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Avoid global hook wiping in test cleanup.
Line 307 uses remove_all_filters( 'frm_obtain_gated_token' ), which can remove callbacks not added by this test and create cross-test flakiness. Remove only the callback registered here, and ensure cleanup runs in a finally.
‼️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.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/phpunit/helpers/test_FrmGatedTokenHelper.php` around lines 296 - 307,
The test is wiping all callbacks for 'frm_obtain_gated_token' which can break
other tests; instead capture the specific closure you register (assign the
static function to a $callback variable and pass that to add_filter) and remove
only that callback with remove_filter('frm_obtain_gated_token', $callback, 10)
in a finally block so cleanup always runs; keep the surrounding calls to
FrmGatedTokenHelper::get_valid_token and FrmGatedItem::make and call
$this->reset_helper_caches() as needed.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
This test can pass for the wrong reason because of action-cache priming.
You create the entry before inserting the action, which can leave frm_actions cached empty for that form. Then trigger_actions('payment-success', ...) may skip the action entirely, so the assertion validates cache behavior instead of event filtering.
(Alternative: explicitly clear the frm_actions cache between wp_insert_post() and trigger_actions().)
📝 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.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/phpunit/misc/test_FrmGatedContentController.php` around lines 105 -
128, The test creates the entry before inserting the form action which can prime
an empty frm_actions cache and make
FrmFormActionsController::trigger_actions('payment-success', $form_id,
$entry_id) skip the action; fix by inserting the action (wp_insert_post(...)
that creates the 'frm_form_actions' post) before creating the entry
($this->factory->entry->create(...)) or, if you must keep the order, explicitly
clear/refresh the frm_actions cache for that form (so trigger_actions sees the
newly inserted action) between wp_insert_post(...) and
FrmFormActionsController::trigger_actions(...).
Remove orphaned docblock and add required blank line.
Lines 217-227 contain a docblock describing "Delete all gated tokens linked to a gated content action when it is permanently deleted" but no method follows it—this is duplicated/misplaced documentation for on_action_deleted() (line 258). This also causes the PHP-CS-Fixer blank_line_before_statement failure.
Proposed fix
}
- /**- * Delete all gated tokens linked to a gated content action when it is permanently deleted.- *- * Fires on 'before_delete_post'. Only acts on frm_form_actions posts whose- * post_excerpt identifies them as gated_content actions.- *- * `@param` int $post_id Post ID being deleted.- * `@param` WP_Post $post Post object being deleted.- *- * `@return` void- */
/**
* Clear the action-item membership cache when a gated content action is updated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@classes/controllers/FrmGatedContentController.php` around lines 217 - 239,
Remove the orphaned/misplaced docblock (the one starting "Delete all gated
tokens linked to a gated content action when it is permanently deleted.") that
sits alone at lines 217-227 and ensure there is a single blank line before the
remaining docblock for on_action_saved/on_action_deleted; effectively delete
that duplicate docblock so the real method on_action_deleted() is immediately
preceded by the correct docblock and add the required blank line to satisfy
blank_line_before_statement.
Harden filtered query args before dereferencing post objects.
frm_gated_content_posts_query can set fields => 'ids', which makes get_posts() return ints; the callback at Line 216 then dereferences $p->post_status and can fatally error. Normalize query/results to WP_Post objects before filtering.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@classes/models/FrmGatedContentAction.php` around lines 195 - 216, The
filtered query args from apply_filters('frm_gated_content_posts_query') may set
'fields' => 'ids' causing get_posts($query_args) to return ints; before
filtering in the anonymous static function that reads $p->post_status and
$p->post_password, normalize results to WP_Post objects (either remove/override
a 'fields' key in $query_args or map integer IDs through get_post()) and/or
guard the filter callback with is_object($p) && $p instanceof WP_Post to avoid
dereferencing non-objects; update the code around $query_args, the get_posts
call, and the static function so only WP_Post instances are passed to the
post_status/post_password checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@classes/controllers/FrmGatedContentController.php`:
- Line 91: Replace the short ternary operator usage in FrmGatedContentController
where $post_id is assigned so it uses an explicit ternary instead of ?:;
specifically change the expression using $query->get('p') ?:
$query->get('page_id') to an explicit conditional like ( (int) $query->get('p')
) ? (int) $query->get('p') : (int) $query->get('page_id') (or retrieve $p into a
temp variable and use $p ? $p : $pageId to avoid duplicate calls). Apply the
same explicit-ternary fix to the other occurrence that uses $query->get('p') ?:
$query->get('page_id').
---
Outside diff comments:
In `@classes/controllers/FrmGatedContentController.php`:
- Around line 217-239: Remove the orphaned/misplaced docblock (the one starting
"Delete all gated tokens linked to a gated content action when it is permanently
deleted.") that sits alone at lines 217-227 and ensure there is a single blank
line before the remaining docblock for on_action_saved/on_action_deleted;
effectively delete that duplicate docblock so the real method
on_action_deleted() is immediately preceded by the correct docblock and add the
required blank line to satisfy blank_line_before_statement.
---
Duplicate comments:
In `@classes/models/FrmGatedContentAction.php`:
- Around line 195-216: The filtered query args from
apply_filters('frm_gated_content_posts_query') may set 'fields' => 'ids' causing
get_posts($query_args) to return ints; before filtering in the anonymous static
function that reads $p->post_status and $p->post_password, normalize results to
WP_Post objects (either remove/override a 'fields' key in $query_args or map
integer IDs through get_post()) and/or guard the filter callback with
is_object($p) && $p instanceof WP_Post to avoid dereferencing non-objects;
update the code around $query_args, the get_posts call, and the static function
so only WP_Post instances are passed to the post_status/post_password checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 69e2252c-23bc-4051-a637-d58f3db12525
📥 Commits
Reviewing files that changed from the base of the PR and between ddb131c and 74ce8d9.
Update template-mode docs to match the current default type.
The inline docs still state template rows default to post, but template rows now default to page. Please align the comments/docblock to avoid future maintenance mistakes.
Suggested doc update
- * - The post type div is always visible (post is the default type).+ * - The post type div is always visible (page is the default type).
...
- * `@var` string $frm_gc_item_type Active type key ('post', 'frm_file', …). Always 'post' for template.+ * `@var` string $frm_gc_item_type Active type key ('post', 'frm_file', …). Always 'page' for template.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@classes/views/frm-form-actions/_gated_content_item_row.php` around lines 9 -
21, The docblock for the template-mode behavior is out of date: update any
mentions that template rows default to "post" to instead say "page" —
specifically change the bullet "The post type div is always visible (post is the
default type)." to reference "page", and update the `@var` description for
$frm_gc_item_type (currently "Always 'post' for template") to "Always 'page' for
template"; keep $is_template and $frm_gc_item_type references intact so the
comment reflects the current default.
Guard filtered get_posts() results before object property access.
frm_gated_content_posts_query can set fields => 'ids'; then get_posts() returns integers, and Line 238/Line 241 will fatal on $post->.... Normalize fields and defensively skip non-WP_Post values.
In WordPress, what does get_posts() return when query arg "fields" is set to "ids"?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@classes/models/FrmGatedContentAction.php` around lines 218 - 243, The filter
frm_gated_content_posts_query can set fields=>'ids' so get_posts() may return
ints; to fix, ensure $query_args forces a full post return (e.g. set/override
$query_args['fields'] = '' before calling get_posts()) and also defensively
guard the foreach over $raw_posts by skipping non-WP_Post values (if ( ! $post
instanceof WP_Post ) { continue; }) so accesses to $post->post_status,
$post->post_password and $post->post_type in FrmGatedContentAction are safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@classes/controllers/FrmGatedContentController.php`:
- Line 101: Replace the short ternary usage in FrmGatedContentController where
you return get_post($post_id) ?: null with an explicit null-safe pattern: call
get_post($post_id) once into a local variable (e.g., $post) and return that
variable using the null coalescing operator (or an explicit ternary) so
PHPStan's rule is satisfied; reference the get_post($post_id) call and $post
variable when making the change.
In `@classes/views/frm-form-actions/_gated_content_item_row.php`:
- Around line 81-82: The if block checking post_type_exists($frm_gc_pt_key)
contains a continue; statement that lacks the required blank line before it per
PHP-CS-Fixer's blank_line_before_statement rule; update the block in
_gated_content_item_row.php (the if (! post_type_exists( $frm_gc_pt_key )) { ...
} block) by inserting a single blank line immediately before the continue;
statement so the formatter rule passes.
---
Outside diff comments:
In `@classes/views/frm-form-actions/_gated_content_item_row.php`:
- Around line 9-21: The docblock for the template-mode behavior is out of date:
update any mentions that template rows default to "post" to instead say "page" —
specifically change the bullet "The post type div is always visible (post is the
default type)." to reference "page", and update the `@var` description for
$frm_gc_item_type (currently "Always 'post' for template") to "Always 'page' for
template"; keep $is_template and $frm_gc_item_type references intact so the
comment reflects the current default.
---
Duplicate comments:
In `@classes/models/FrmGatedContentAction.php`:
- Around line 218-243: The filter frm_gated_content_posts_query can set
fields=>'ids' so get_posts() may return ints; to fix, ensure $query_args forces
a full post return (e.g. set/override $query_args['fields'] = '' before calling
get_posts()) and also defensively guard the foreach over $raw_posts by skipping
non-WP_Post values (if ( ! $post instanceof WP_Post ) { continue; }) so accesses
to $post->post_status, $post->post_password and $post->post_type in
FrmGatedContentAction are safe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4eaf732e-2314-4d83-87fa-6613798a2fba
📥 Commits
Reviewing files that changed from the base of the PR and between 74ce8d9 and 4a68b9b.
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
if ( ! post_type_exists( $frm_gc_pt_key ) ) {
continue;
if ( ! post_type_exists( $frm_gc_pt_key ) ) {
continue;
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@classes/views/frm-form-actions/_gated_content_item_row.php` around lines 81 -
82, The if block checking post_type_exists($frm_gc_pt_key) contains a continue;
statement that lacks the required blank line before it per PHP-CS-Fixer's
blank_line_before_statement rule; update the block in
_gated_content_item_row.php (the if (! post_type_exists( $frm_gc_pt_key )) { ...
} block) by inserting a single blank line immediately before the continue;
statement so the formatter rule passes.
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick comments (1)
classes/controllers/FrmFormsController.php (1)
1852-1852: 💤 Low value
Add type hint for $form_id parameter.
PHPStan reports the parameter lacks a type. Adding int $form_id = 0 improves static analysis coverage and aligns with the casting done in callers.
Suggested fix
- private static function get_shortcode_helpers( $settings_tab, $form_id = 0 ) {+ private static function get_shortcode_helpers( $settings_tab, int $form_id = 0 ) {
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@classes/controllers/FrmFormsController.php` at line 1852, Update the
get_shortcode_helpers function signature to add an integer type hint for the
$form_id parameter (change its declaration to accept int $form_id = 0) so static
analysis (PHPStan) knows the expected type; locate the private static function
get_shortcode_helpers(...) and modify its parameter to include the int type,
then run tests/lint to ensure no call sites violate the stricter type (cast or
adjust callers if needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@classes/controllers/FrmFormsController.php`:
- Line 1852: Update the get_shortcode_helpers function signature to add an
integer type hint for the $form_id parameter (change its declaration to accept
int $form_id = 0) so static analysis (PHPStan) knows the expected type; locate
the private static function get_shortcode_helpers(...) and modify its parameter
to include the int type, then run tests/lint to ensure no call sites violate the
stricter type (cast or adjust callers if needed).
ℹ️ Review info⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9570029e-0f65-4007-8005-82b9b66eecac
📥 Commits
Reviewing files that changed from the base of the PR and between 4a68b9b and a2dc00d.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Related PR: https://github.com/Strategy11/formidable-pro/pull/6451
Summary by CodeRabbit
New Features
Tests
Chores