-
Notifications
You must be signed in to change notification settings - Fork 39
Add a prefix to turnstile elements #2554
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis PR refactors CAPTCHA class prefix handling by introducing a Changes
Sequence DiagramsequenceDiagram
participant FC as FrmFieldCaptcha
participant FCF as FrmCaptchaFactory
participant SO as Settings Object<br/>(Recaptcha/Turnstile)
Note over FC,SO: Old: Inline class prefix logic
rect rgb(240, 248, 255)
Note over FC,SO: New: Delegated approach
FC->>FCF: get_settings_object()
FCF-->>FC: Returns Settings Object instance
FC->>SO: get_class_prefix($allow_multiple)
SO-->>FC: Returns 'frm-' or ''
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes introduce a consistent delegation pattern across multiple settings classes and refactor existing logic in FrmFieldCaptcha, with additional selector updates in JavaScript. While the individual changes are straightforward, the spread across multiple files and need to verify the delegation logic integration warrants moderate review effort. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
classes/models/FrmFieldCaptchaSettings.php(1 hunks)classes/models/FrmRecaptchaSettings.php(1 hunks)classes/models/FrmTurnstileSettings.php(1 hunks)classes/models/fields/FrmFieldCaptcha.php(2 hunks)js/formidable.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
classes/models/FrmFieldCaptchaSettings.php (3)
classes/models/FrmRecaptchaSettings.php (1)
get_class_prefix(126-128)classes/models/FrmTurnstileSettings.php (1)
get_class_prefix(86-88)classes/models/fields/FrmFieldCaptcha.php (1)
allow_multiple(271-273)
classes/models/FrmTurnstileSettings.php (3)
classes/models/FrmFieldCaptchaSettings.php (1)
get_class_prefix(265-267)classes/models/FrmRecaptchaSettings.php (1)
get_class_prefix(126-128)classes/models/fields/FrmFieldCaptcha.php (1)
allow_multiple(271-273)
classes/models/FrmRecaptchaSettings.php (3)
classes/models/FrmFieldCaptchaSettings.php (1)
get_class_prefix(265-267)classes/models/FrmTurnstileSettings.php (1)
get_class_prefix(86-88)classes/models/fields/FrmFieldCaptcha.php (1)
allow_multiple(271-273)
classes/models/fields/FrmFieldCaptcha.php (3)
classes/models/FrmFieldCaptchaSettings.php (1)
get_class_prefix(265-267)classes/models/FrmRecaptchaSettings.php (1)
get_class_prefix(126-128)classes/models/FrmTurnstileSettings.php (1)
get_class_prefix(86-88)
🪛 PHPMD (2.15.0)
classes/models/FrmFieldCaptchaSettings.php
265-265: Avoid unused parameters such as '$allow_multiple'. (undefined)
(UnusedFormalParameter)
classes/models/FrmTurnstileSettings.php
86-86: Avoid unused parameters such as '$allow_multiple'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Run PHP Syntax inspection (8.3)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (5)
classes/models/fields/FrmFieldCaptcha.php (2)
174-174: LGTM: Improved encapsulation.Refactoring to use the
allow_multiple()method instead of direct property access improves encapsulation and consistency.
255-257: LGTM: Clean delegation to settings object.The refactor properly delegates prefix determination to the settings object, removing inline conditional logic and improving maintainability.
js/formidable.js (3)
968-970: LGTM: Selector update is consistent with PHP changes.The Turnstile selector correctly updated to
.frm-cf-turnstileto match the prefixed class name generated by the PHP changes.
2068-2068: LGTM: Consistent selector in frmTurnstile function.The selector update aligns with the new prefixed class name.
2117-2119: LGTM: Consistent detection logic.The Turnstile detection properly uses the updated
.frm-cf-turnstileselector.
Related ticket https://secure.helpscout.net/conversation/3115553783/240278
This fixes a conflict with All in One Security, where more than one Turnstile element would get rendered.
To avoid this I'm avoid the
cf-turnstileclass name and usingfrm-cf-turnstileinstead now.Pre-release
formidable-6.25.1b.zip