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
Reported against formidable-pro, but the actual markup is in this repo's FrmEntriesListHelper.php (Entries admin list table).
On the "all forms" Entries screen (td.column-0_form_id > a.row-title), the Form column becomes the row's action column whenever the ID/Entry Key columns are hidden via screen options. Its value was built by FrmFormsHelper::edit_form_link(), which returns its own <a href="...">Form Name</a>. That got nested inside the outer row-title <a> the row renderer wraps the action column in. Browsers implicitly close the outer anchor as soon as they hit the nested one, so the row-title link ends up with no accessible name — an axe link-name violation on every row, and worse (a fully empty link, no visible text either) when the entry's form no longer exists, since edit_form_link() returns '' for a falsy form id.
Self-review found the identical bug on the Post column (post_id, shown when a form has a create-post action): FrmAppHelper::post_edit_link() has the same own-nested-<a> shape and the same failure mode when it's the action column.
Fix
column_value() now knows whether it's rendering the row's action column. When it is, both the form_id and post_id cases use plain-text fallback labels (FrmFormsHelper::edit_form_link_label() / a truncated post title with FrmFormsHelper::get_no_title_text() as the missing-post fallback) instead of building their own nested link — matching the existing behavior already used for users who can't edit forms. Non-action-column rendering is unchanged.
Verified
Red/green: reverted the production change, confirmed both new tests fail for the expected reason (nested <a> still present / still using the linked helper), reapplied the fix, confirmed green.
./vendor/bin/phpunit --group entries: 57 tests, 167 assertions, all green.
php -l + bare ./vendor/bin/phpcs (repo's own phpcs.xml, no --standard override) clean on both changed files.
Self-review: simplify skill + 4 parallel Agent lenses (reuse, simplification, security, correctness) against the diff — no issues found beyond the post_id gap, which is now fixed in this same PR.
No before/after screenshot attached — this is a markup-structure fix (nested anchor / accessible-name), not a visual change; verification is via the axe-relevant DOM structure (tests above) rather than a screenshot.
When the Form column is the Entries list's action column (ID/Entry Key
columns hidden, as on the "all forms" view), its value was wrapped in a
second, nested <a> pointing at the form itself. Browsers implicitly
close the outer row-title anchor as soon as they hit that nested tag,
leaving it with no accessible name -- an axe link-name violation on
every row, and doubly so when the entry's form no longer exists (the
inner link was empty too).
Use the plain-text form label instead of the linked version whenever
this column is acting as the row's own action column, matching the
existing non-editor fallback path. The label already handles a
missing/unnamed form with "(no title)" text.
Self-review turned up the identical defect on the 'post_id' ("Post")
column: post_edit_link() builds its own <a>, which breaks the outer
row-title link the same way edit_form_link() did when this column ends
up as the row's action column. Same fix, same fallback text for a
deleted post.
We reviewed changes in 2a2d8c1...98a0ac4 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.
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.
`column_value` has a cyclomatic complexity of 18 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.
The reason will be displayed to describe this comment to others. Learn more.
Verified live in formidable-preview-env — root-caused and reproduced both before/after states directly (not just the diff):
Before (master): with ID/Entry Key hidden via screen options (the default once a form's column count exceeds 11, or manually), the all-forms Entries screen's Form column becomes $action_col. Its cell rendered <a class="row-title"></a><a href="...&frm_action=edit&id=1">Contact Us</a> — the browser splits the invalid nested anchor into two siblings, leaving row-title empty (the exact axe link-name violation described).
After (this PR's head, 8aef52b): same setup, same entry — the cell renders a single, correct <a class="row-title">Contact Us</a>.
Also confirmed via source (couldn't easily reproduce this specific sub-case live without an existing post-creating form + deep screen-options setup): both the missing-form and missing-post fallback branches, and the new unit tests are real regression tests (assert absence of a nested <a>, not just presence of a string) — good coverage, not padding.
One non-blocking generalization gap (inline comment below): the fix only reaches the two built-in form_id/post_id cases inside column_value()'s switch. The default case (frm_entries_{col}_column / frm_entries_column_value filters, used by add-ons registering their own entries columns) never receives $is_action_col, so a third-party column that builds its own <a> would hit the identical empty-row-title bug if it ever became the action column. Not something this PR needs to fix blind (no known add-on column currently does this), but worth a follow-up or at least a code comment noting the gap.
Nothing else outstanding. CI green; the one DeepSource PHP flag (cyclomatic complexity 18 on column_value) is pre-existing (same 11-case switch already there on master) and only marginally increased by this fix's necessary branching — not worth blocking on.
Non-blocking generalization gap (couldn't anchor inline since it's outside the diff's changed lines): classes/helpers/FrmEntriesListHelper.php's default case (~line 488) applies frm_entries_{$col_name}_column / frm_entries_column_value filters, used by add-ons registering their own entries columns, without passing through $is_action_col. Any add-on column that builds its own <a> there would hit the identical empty-row-title bug this PR fixes for form_id/post_id, if that column ever became the row's action column. Suggest threading is_action_col through compact() in both apply_filters() calls so a filter callback can make the same decision — or, if deferring, a short comment noting the limitation. Not blocking this fix; no known add-on column currently hits it.
The reason will be displayed to describe this comment to others. Learn more.
Approve — see the review comment above for live before/after evidence and one non-blocking generalization note (filter-based custom columns aren't covered by $is_action_col, no known add-on currently hits it).
Franky's non-blocking review note: filter-based custom columns
(frm_entries_{col}_column / frm_entries_column_value) don't receive
$is_action_col, so a third-party column building its own <a> could hit
the same nested-anchor bug as form_id/post_id if it ever became the
row's action column. No known add-on does this today, so documenting
the gap rather than generalizing blind.
Re the generalization gap: added a code comment on the default case (filter-based custom columns) documenting that $is_action_col is not passed through, per the "or at least a code comment noting the gap" option — no known add-on hits this today so left the filters themselves unchanged.
The reason will be displayed to describe this comment to others. Learn more.
Re-review at 98a0ac4 (in-place push since my prior approve at 8aef52b): the only change is a 3-line code comment on FrmEntriesListHelper.php's default case documenting the $is_action_col generalization gap I flagged as non-blocking. That closes the one outstanding note from my last review — nothing else changed.
Approve — clean, nothing outstanding. (Unrelated pre-existing CI red — PHP CS Fixer, ESLint, DeepSource:PHP — confirmed present at the prior commit too and not touching this PR's files.)
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}
![franky-the-going-merry[bot]](https://avatars.githubusercontent.com/in/4233782?s=60&v=4){kind=small}
vivi-the-going-merry
Bot
added
vivi-working
Fixes Strategy11/formidable-pro#6662
What was broken
Reported against formidable-pro, but the actual markup is in this repo's
FrmEntriesListHelper.php(Entries admin list table).On the "all forms" Entries screen (
td.column-0_form_id > a.row-title), the Form column becomes the row's action column whenever the ID/Entry Key columns are hidden via screen options. Its value was built byFrmFormsHelper::edit_form_link(), which returns its own<a href="...">Form Name</a>. That got nested inside the outer row-title<a>the row renderer wraps the action column in. Browsers implicitly close the outer anchor as soon as they hit the nested one, so the row-title link ends up with no accessible name — an axelink-nameviolation on every row, and worse (a fully empty link, no visible text either) when the entry's form no longer exists, sinceedit_form_link()returns''for a falsy form id.Self-review found the identical bug on the Post column (
post_id, shown when a form has a create-post action):FrmAppHelper::post_edit_link()has the same own-nested-<a>shape and the same failure mode when it's the action column.Fix
column_value()now knows whether it's rendering the row's action column. When it is, both theform_idandpost_idcases use plain-text fallback labels (FrmFormsHelper::edit_form_link_label()/ a truncated post title withFrmFormsHelper::get_no_title_text()as the missing-post fallback) instead of building their own nested link — matching the existing behavior already used for users who can't edit forms. Non-action-column rendering is unchanged.Verified
<a>still present / still using the linked helper), reapplied the fix, confirmed green../vendor/bin/phpunit --group entries: 57 tests, 167 assertions, all green.php -l+ bare./vendor/bin/phpcs(repo's ownphpcs.xml, no--standardoverride) clean on both changed files.simplifyskill + 4 parallel Agent lenses (reuse, simplification, security, correctness) against the diff — no issues found beyond the post_id gap, which is now fixed in this same PR.No before/after screenshot attached — this is a markup-structure fix (nested anchor / accessible-name), not a visual change; verification is via the axe-relevant DOM structure (tests above) rather than a screenshot.