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
We reviewed changes in 6b58fff...063b942 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.
Visibility should be explicitly set for `CATEGORY` constant
Visibility (also know as Access Modifiers) can be used to define where it can be accessed. There are three access modifiers available in PHP:
public - The class members can be accessed from everywhere. This is default.
protected - The class members can be accessed within the class and by classes derived from that class.
private - The class members can only be accessed within the class.
The class members(properties, constants, or methods) declared without any explicit visibility keyword are by default considered as public. It is recommended to set visibility explicitly, which increases code readability. In addition, it gives the developer a mental model of where the class member would be accessible, which also leads to a better API design and makes sure that you are not making something public which isn't supposed to be.
Also, as per PSR-12: Extended Coding Style, visibility should be explicitly declared with all class properties, constants and methods.
The reason will be displayed to describe this comment to others. Learn more.
Visibility should be explicitly set for `REQUIRED_CLASSES` constant
Visibility (also know as Access Modifiers) can be used to define where it can be accessed. There are three access modifiers available in PHP:
public - The class members can be accessed from everywhere. This is default.
protected - The class members can be accessed within the class and by classes derived from that class.
private - The class members can only be accessed within the class.
The class members(properties, constants, or methods) declared without any explicit visibility keyword are by default considered as public. It is recommended to set visibility explicitly, which increases code readability. In addition, it gives the developer a mental model of where the class member would be accessible, which also leads to a better API design and makes sure that you are not making something public which isn't supposed to be.
Also, as per PSR-12: Extended Coding Style, visibility should be explicitly declared with all class properties, constants and methods.
The reason will be displayed to describe this comment to others. Learn more.
Visibility should be explicitly set for `CREATE_SERVER_PARAMS` constant
Visibility (also know as Access Modifiers) can be used to define where it can be accessed. There are three access modifiers available in PHP:
public - The class members can be accessed from everywhere. This is default.
protected - The class members can be accessed within the class and by classes derived from that class.
private - The class members can only be accessed within the class.
The class members(properties, constants, or methods) declared without any explicit visibility keyword are by default considered as public. It is recommended to set visibility explicitly, which increases code readability. In addition, it gives the developer a mental model of where the class member would be accessible, which also leads to a better API design and makes sure that you are not making something public which isn't supposed to be.
Also, as per PSR-12: Extended Coding Style, visibility should be explicitly declared with all class properties, constants and methods.
The reason will be displayed to describe this comment to others. Learn more.
Visibility should be explicitly set for `REQUIRED_ABILITY_FUNCTIONS` constant
Visibility (also know as Access Modifiers) can be used to define where it can be accessed. There are three access modifiers available in PHP:
public - The class members can be accessed from everywhere. This is default.
protected - The class members can be accessed within the class and by classes derived from that class.
private - The class members can only be accessed within the class.
The class members(properties, constants, or methods) declared without any explicit visibility keyword are by default considered as public. It is recommended to set visibility explicitly, which increases code readability. In addition, it gives the developer a mental model of where the class member would be accessible, which also leads to a better API design and makes sure that you are not making something public which isn't supposed to be.
Also, as per PSR-12: Extended Coding Style, visibility should be explicitly declared with all class properties, constants and methods.
The reason will be displayed to describe this comment to others. Learn more.
Visibility should be explicitly set for `MIN_PHP_ID` constant
Visibility (also know as Access Modifiers) can be used to define where it can be accessed. There are three access modifiers available in PHP:
public - The class members can be accessed from everywhere. This is default.
protected - The class members can be accessed within the class and by classes derived from that class.
private - The class members can only be accessed within the class.
The class members(properties, constants, or methods) declared without any explicit visibility keyword are by default considered as public. It is recommended to set visibility explicitly, which increases code readability. In addition, it gives the developer a mental model of where the class member would be accessible, which also leads to a better API design and makes sure that you are not making something public which isn't supposed to be.
Also, as per PSR-12: Extended Coding Style, visibility should be explicitly declared with all class properties, constants and methods.
The reason will be displayed to describe this comment to others. Learn more.
Visibility should be explicitly set for `SERVER_ID` constant
Visibility (also know as Access Modifiers) can be used to define where it can be accessed. There are three access modifiers available in PHP:
public - The class members can be accessed from everywhere. This is default.
protected - The class members can be accessed within the class and by classes derived from that class.
private - The class members can only be accessed within the class.
The class members(properties, constants, or methods) declared without any explicit visibility keyword are by default considered as public. It is recommended to set visibility explicitly, which increases code readability. In addition, it gives the developer a mental model of where the class member would be accessible, which also leads to a better API design and makes sure that you are not making something public which isn't supposed to be.
Also, as per PSR-12: Extended Coding Style, visibility should be explicitly declared with all class properties, constants and methods.
The reason will be displayed to describe this comment to others. Learn more.
Visibility should be explicitly set for `ROUTE_NAMESPACE` constant
Visibility (also know as Access Modifiers) can be used to define where it can be accessed. There are three access modifiers available in PHP:
public - The class members can be accessed from everywhere. This is default.
protected - The class members can be accessed within the class and by classes derived from that class.
private - The class members can only be accessed within the class.
The class members(properties, constants, or methods) declared without any explicit visibility keyword are by default considered as public. It is recommended to set visibility explicitly, which increases code readability. In addition, it gives the developer a mental model of where the class member would be accessible, which also leads to a better API design and makes sure that you are not making something public which isn't supposed to be.
Also, as per PSR-12: Extended Coding Style, visibility should be explicitly declared with all class properties, constants and methods.
The reason will be displayed to describe this comment to others. Learn more.
Use of insecure md5() function found
Using md5(), sha1() function is not recommended to generate secure passwords. Due to its fast nature to compute passwords too quickly, these functions can become really easy to crack a password using brute force attack.
It is recommended to use PHP's password hashing function password_hash() to create a secure password hash.
The reason will be displayed to describe this comment to others. Learn more.
Visibility should be explicitly set for `SKILL_RELEASE_TRANSIENT` constant
Visibility (also know as Access Modifiers) can be used to define where it can be accessed. There are three access modifiers available in PHP:
public - The class members can be accessed from everywhere. This is default.
protected - The class members can be accessed within the class and by classes derived from that class.
private - The class members can only be accessed within the class.
The class members(properties, constants, or methods) declared without any explicit visibility keyword are by default considered as public. It is recommended to set visibility explicitly, which increases code readability. In addition, it gives the developer a mental model of where the class member would be accessible, which also leads to a better API design and makes sure that you are not making something public which isn't supposed to be.
Also, as per PSR-12: Extended Coding Style, visibility should be explicitly declared with all class properties, constants and methods.
The reason will be displayed to describe this comment to others. Learn more.
Visibility should be explicitly set for `SKILL_DOWNLOAD_META` constant
Visibility (also know as Access Modifiers) can be used to define where it can be accessed. There are three access modifiers available in PHP:
public - The class members can be accessed from everywhere. This is default.
protected - The class members can be accessed within the class and by classes derived from that class.
private - The class members can only be accessed within the class.
The class members(properties, constants, or methods) declared without any explicit visibility keyword are by default considered as public. It is recommended to set visibility explicitly, which increases code readability. In addition, it gives the developer a mental model of where the class member would be accessible, which also leads to a better API design and makes sure that you are not making something public which isn't supposed to be.
Also, as per PSR-12: Extended Coding Style, visibility should be explicitly declared with all class properties, constants and methods.
This will return "success":true with a fabricated style object named "Hello world!" / post_name: "hello-world", instead of a 404 "invalid style ID" error.
This returns "success": true, "message": "Item added to application successfully." You can check the application as well to see nothing was ever added.
Every ability fails on empty parameters ({}). list-forms, list-views, list-view-layouts, and list-coupons all fail with a generic error when called with parameters: {}, but succeed instantly if you add literally any key, even a made-up one like {"zzz": 1}.
delete-style/get-style operating on any post (garretlaxton, Sep 8): already fixed on this branch — FrmAbilitiesStylesController::get_style() guards with is_style_post() before returning. delete-style itself isn't registered here at all; this PR's Styles controller only registers list/get/update-style, matching the PR description. Delete lives in Pro (formidable-pro#6580), tracked separately there.
Empty {} params failing on list-forms/list-views/list-view-layouts/list-coupons (garretlaxton, Sep 10): traced to AbilityArgumentNormalizer::normalize() in the vendored mcp-adapter package (lib/vendor/wordpress/mcp-adapter), not Formidable's own code. For a schema with properties but no top-level default, it returns [] for both null and {} input — the ability's own execute callback never sees a distinction. list-forms (the only one of the four actually registered in this repo) has no top-level default, so it hits this path. list-views/list-view-layouts/list-coupons aren't registered here at all (formidable-views/formidable-coupons). Out of scope for a fix in this repo — either a mcp-adapter fix upstream, or each affected ability adding a top-level schema default (which changes normalizer behavior to return null, letting WP_Ability apply that default instead).
Not claiming further action on either — clearing labels back.
❌ Patch coverage is 71.58868% with 1014 lines in your changes missing coverage. Please review.
✅ Project coverage is 33.94%. Comparing base (38ec34b) to head (99d5fcc). ⚠️ Report is 35 commits behind head on master.
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the authored surface of this PR (33 non-vendor files, ~10.8k new lines; the remaining ~292 changed files are the vendored lib/vendor/wordpress/php-mcp-schema / mcp-adapter / automattic/jetpack-autoloader / composer dependency trees this feature is built on, not hand-written here).
Verified: the MCP boot/registration gating (FrmMcpController, FrmAbilitiesController) and its interop with an older API add-on; the permission model across all 7 ability domains (forms/fields/entries/styles/form-actions/payments/subscriptions) — every mutating ability has a permission_callback mapped to a sensible frm_view_*/frm_edit_*/frm_delete_* capability (or administrator), no domain found with a missing or over-permissive check; the settings-page download handler (FrmMcpSettingsController::download_skill) has both a capability check and a nonce check; and the payments/subscriptions list query builder (build_list_query) — table names go through %i, values through %s/%d, and order_by/order are passed through the existing FrmDb::esc_order()/esc_order_by() allow-list before reaching SQL, so user-supplied sort input can't inject.
One CI-confirmed blocking issue (inline below): FrmAbilitiesFormActionsController::execute_list_form_actions() queries with numberposts => -1 (VIP-lint NoPaging, currently the one real PHPCS failure on this PR) where the existing, otherwise-identical query shape in FrmFormAction::action_args() already uses a bounded default ($limit = 99) — this is a new unbounded variant of a pattern the codebase already solved bounded.
One non-blocking gap: the new architectural/wiring code — FrmMcpCompat (492 lines), FrmMcpAbilityRegistry (519 lines), FrmMcpConnection (394 lines), FrmMcpSettingsController, FrmHooksController, FrmAbilitiesHelper — has no new PHPUnit coverage. The 7 domain ability controllers and a shared contract test are well covered; this ~2000-line core layer (including FrmMcpController::is_enabled()'s three-way own-setting/inherited-setting/filtered-default fallback, which is exactly the kind of branching logic worth a direct test) isn't. CI is otherwise green (PHPUnit passes on PHP 7.4 and 8, syntax/CS-Fixer/Rector/Psalm/PHPStan/Mago/ESLint/Stylelint/Oxlint/Typos all pass).
Not exercised this pass (disclosing per review scope, not asking for anything): line-by-line read of the individual CRUD bodies of the Fields/Entries/Forms/Styles/Subscriptions ability controllers (only Payments was read in full as the representative), the vendored MCP adapter/schema source itself, stubs.php, the two touched Stripe-Lite files, and DeepSource's own PHP findings (its check failed but the review comment carries no body and the dashboard link needs a JS-rendered/authenticated session this pass couldn't reach). No live MCP-client exercise — this is a backend API surface with no visual/UX component to screenshot.
The reason will be displayed to describe this comment to others. Learn more.
PHPCS is currently red on this line (WordPressVIPMinimum.Performance.NoPaging.posts_per_page_numberposts): numberposts => -1 disables pagination on a get_posts() call.
The existing, same-shape query in FrmFormAction::action_args() already solved this bounded rather than unbounded:
Suggest matching that precedent here instead of introducing a new unbounded variant — e.g. reuse FrmFormAction::action_args()'s bound, or give this ability its own bounded default (this endpoint's sibling list abilities already cap page_size at 200, so a comparable cap here would be consistent).
The reason will be displayed to describe this comment to others. Learn more.
Fixed -- capped at 200, matching the page_size cap the sibling list abilities (payments/subscriptions/styles) already use. This ability has no page_size input of its own to honor instead, so a hardcoded bound is the right fix here rather than reusing action_args()s 99 default. Pushed in 85096a3.
Fixed the one blocking finding: execute_list_form_actions()'s numberposts => -1 capped at 200, matching the page_size cap already used by the sibling list abilities. This ability has no page_size input of its own, so a hardcoded bound is the right fix rather than honoring an existing arg.
Left the non-blocking test-coverage gap (FrmMcpCompat/FrmMcpAbilityRegistry/FrmMcpConnection/etc.) to the PR's own author -- Franky split it out as non-blocking and this is an active human author's PR, not an orphaned-author handoff.
The reason will be displayed to describe this comment to others. Learn more.
Re-review at 85096a3 (my prior review was on 99d5fcc, CHANGES_REQUESTED).
Fix verified:execute_list_form_actions()'s numberposts => -1 is now 200, matching the page_size cap already used by the sibling list abilities (FrmAbilitiesPaymentsController/FrmAbilitiesSubscriptionsController, "capped at 200. Default 50."). Only this one line changed since my last review, so the permission-model/SQL-safety/boot-registration findings from that pass still hold. CI is now fully green — the PHPCS NoPaging failure that blocked the prior round is gone; PHPUnit (7.4/8), PHPCS, PHPStan, Psalm, Mago, Rector, CS-Fixer, ESLint, Oxlint, Stylelint, Typos, DeepScan, and Scrutinizer all pass.
Non-blocking, new this pass: DeepSource: PHP now fails — checked all 24 of its inline comments. 20 are PSR-12 "explicit visibility on class constant" style nits; one flags md5() in FrmMcpMissingAbilityController::logged_recently() (not security-sensitive — it's a transient-key hash for log dedup, no auth/crypto use); one flags an apply_filters() call with an extra optional arg (valid); two flag high cyclomatic complexity (get_endpoint 17, prepare_entry_data 19). None block — filing alongside the still-open test-coverage gap from my last review (FrmMcpCompat/FrmMcpAbilityRegistry/FrmMcpConnection/etc.), which Vivi's fix-push above left to the author.
Setup steps
- Drop the card around the steps; they sit on the page, aligned with the
toggle. Numbered markers sit in the heading row, and a finished step
shows a green check circle.
- One primary action at a time: Download leads until a file exists, then
Copy setup prompt until an assistant connects, then nothing. The
download label and button styles update without a reload.
- Setup prompt, connection files and manual skill install move behind
disclosures; disclosure content aligns with its label.
- The assistant picker is keyboard reachable (the radios were
display:none) and no longer reuses the captcha's frm_captchas class,
which admin.js targets.
- Copy setup prompt is a labelled button; Copied swaps to a check sized
to match the copy icon.
- Waiting state shows a spinner (off under reduced motion) and the
content staggers in when the server is switched on.
- Prose is capped at a readable measure; spacing uses the 8/16/24/32
tokens.
Copy
- Shorter, plainer strings throughout, with descriptive links. The revoke
note says why files must be revoked before deactivating Formidable.
Error pages use "connection file" and say how to recover.
- MCP Connections keeps one description for every state.
Components
- Icon buttons (.button.frm-with-icon) now lay out as inline-flex, so the
gap applies; WordPress core's inline-block used to win. 4px gap, 20px
icons (16px small) per the Figma spec, icons at full strength.
- Add frm_file_download_icon from Figma to the sprite, and the
--success-600 token from the design system.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
delete-style succeeded on a style assigned to a form. However, the form's custom_style still pointed at the deleted ID, and its Submit button rendered unstyled. There is no warning or fallback.
Whenever you enable the MCP then click update, it shows MCP is turned on but not running. The MCP adapter could not be loaded., but refreshing the page clears this banner and everything works properly. I'm guessing this is a false positive?
I pushed new updates to Lite and Pro. Lite has the fix for the error you shared, and Pro has the update for re-assigning all forms to the default style when their previously selected style is deleted.
The reason will be displayed to describe this comment to others. Learn more.
`get_skill_status` has a cyclomatic complexity of 16 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.
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}
vivi-the-going-merry
Bot
added
vivi-working
![franky-the-going-merry[bot]](https://avatars.githubusercontent.com/in/4233782?s=60&v=4){kind=small}
Pro functionality https://github.com/Strategy11/formidable-pro/pull/6580
Views functionality https://github.com/Strategy11/formidable-views/pull/751
Landing pages functionality https://github.com/Strategy11/formidable-landing/pull/57
Coupons functionality https://github.com/Strategy11/formidable-coupons/pull/33
Logs functionality https://github.com/Strategy11/formidable-logs/pull/61
WPML functionality https://github.com/Strategy11/formidable-wpml/pull/155
Related MCP Skill update Strategy11/formidable-mcp-skill#7
API add-on compatibility https://github.com/Strategy11/formidable-api/pull/237
Lite includes the following abilities: