Skip to content

Update prefer esc_html_e/esc_attr_e sniff to catch more cases#2906

Merged
Crabcyborg merged 2 commits into
masterfrom
update_prefer_esc_html_e_sniff_to_catch_more_cases
Jan 29, 2026
Merged

Update prefer esc_html_e/esc_attr_e sniff to catch more cases#2906
Crabcyborg merged 2 commits into
masterfrom
update_prefer_esc_html_e_sniff_to_catch_more_cases

Conversation

@Crabcyborg

@Crabcyborg Crabcyborg commented Jan 29, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Refactor

    • Improved internal code quality by standardizing translation and escaping function patterns across multiple templates for better maintainability.
  • Tests

    • Added enhanced code analysis rules to enforce improved translation and escaping practices in the codebase.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai

coderabbitai Bot commented Jan 29, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

This pull request refactors WordPress i18n function calls across multiple template and view files to use direct echo variants (esc_html_e, esc_attr_e) instead of combining echo with return-based variants (esc_html__, esc_attr__). Additionally, a PHP CodeSniffer sniff was enhanced to detect and auto-fix these patterns, including handling of direct combined function calls.

Changes

Cohort / File(s) Summary
Translation Function Refactoring (Templates)
classes/views/dashboard/templates/notification-banner.php, classes/views/form-templates/template.php, classes/views/shared/errors.php
Replaced echo esc_html__() calls with esc_html_e() across 5 instances (3 in notification banner, 1 each in other files). Output remains identical; only the function call pattern changed.
Attribute Translation Refactoring
classes/helpers/FrmHtmlHelper.php, classes/views/styles/components/templates/slider.php, stripe/views/payments/show.php
Replaced echo esc_attr__() with esc_attr_e() for aria-label and data attributes across 20 instances (1 in FrmHtmlHelper, 18 in slider.php, 1 in payments). Functionally equivalent output.
CodeSniffer Sniff Enhancement
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/PreferEscHtmlESniff.php
Added detection and auto-fix for direct combined translation functions via new processDirectCombinedFunction() and replaceEchoWithCombinedFunction() helper methods. Introduced $directReplacements mapping (esc_html__ → esc_html_e, esc_attr__ → esc_attr_e). Early-detection routes appropriate patterns to dedicated handler.
Potential HTML Structure Issue
stripe/views/subscriptions/show.php
Summary indicates missing closing angle bracket (>) on anchor opening tag, which may represent a syntax error requiring verification.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Poem

🐰 The echo patterns now are clean and neat,
Combined functions make translations sweet,
No more double calls to do the dance,
With esc_*_e we simplify the chant,
A sniff now guides us down the right path! 🌿

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title accurately describes the main objective: updating the prefer esc_html_e/esc_attr_e sniff to catch more cases, which is implemented in the PreferEscHtmlESniff.php file with new direct replacement detection logic.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Crabcyborg Crabcyborg changed the title Update prefer esc html e sniff to catch more cases Update prefer esc_html_e/esc_attr_e sniff to catch more cases Jan 29, 2026
@Crabcyborg
Crabcyborg merged commit 0b29fed into master Jan 29, 2026
16 of 17 checks passed
@Crabcyborg
Crabcyborg deleted the update_prefer_esc_html_e_sniff_to_catch_more_cases branch January 29, 2026 17:43

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@classes/views/dashboard/templates/notification-banner.php`:
- Line 17: The anchor that opens a new tab (the link using
FrmAppHelper::admin_upgrade_link and class "frm-button-secondary") should
include rel="noopener noreferrer" to mitigate reverse-tabnabbing; update the <a>
tag that currently has target="_blank" (the one generating the upgrade link) to
add rel="noopener noreferrer" while keeping the existing esc_url() href and
esc_html_e() label intact.

</div>
<div class="frm-shrink-0">
<a target="_blank" href="<?php echo esc_url( FrmAppHelper::admin_upgrade_link( '', 'features' ) ); ?>" class="frm-button-secondary"><?php echo esc_html__( 'Check All Features', 'formidable' ); ?></a>
<a target="_blank" href="<?php echo esc_url( FrmAppHelper::admin_upgrade_link( '', 'features' ) ); ?>" class="frm-button-secondary"><?php esc_html_e( 'Check All Features', 'formidable' ); ?></a>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add rel="noopener noreferrer" for the new-tab link.

Line 17 opens a new tab to an external site; add rel to mitigate reverse‑tabnabbing.

🔧 Suggested change
-			<a target="_blank" href="<?php echo esc_url( FrmAppHelper::admin_upgrade_link( '', 'features' ) ); ?>" class="frm-button-secondary"><?php esc_html_e( 'Check All Features', 'formidable' ); ?></a>
+			<a target="_blank" rel="noopener noreferrer" href="<?php echo esc_url( FrmAppHelper::admin_upgrade_link( '', 'features' ) ); ?>" class="frm-button-secondary"><?php esc_html_e( 'Check All Features', 'formidable' ); ?></a>
📝 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.

Suggested change
<a target="_blank" href="<?php echo esc_url( FrmAppHelper::admin_upgrade_link( '', 'features' ) ); ?>" class="frm-button-secondary"><?php esc_html_e( 'Check All Features', 'formidable' ); ?></a>
<a target="_blank" rel="noopener noreferrer" href="<?php echo esc_url( FrmAppHelper::admin_upgrade_link( '', 'features' ) ); ?>" class="frm-button-secondary"><?php esc_html_e( 'Check All Features', 'formidable' ); ?></a>
🤖 Prompt for AI Agents
In `@classes/views/dashboard/templates/notification-banner.php` at line 17, The
anchor that opens a new tab (the link using FrmAppHelper::admin_upgrade_link and
class "frm-button-secondary") should include rel="noopener noreferrer" to
mitigate reverse-tabnabbing; update the <a> tag that currently has
target="_blank" (the one generating the upgrade link) to add rel="noopener
noreferrer" while keeping the existing esc_url() href and esc_html_e() label
intact.

stephywells pushed a commit that referenced this pull request Apr 4, 2026
…ff_to_catch_more_cases

Update prefer `esc_html_e`/`esc_attr_e` sniff to catch more cases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant