Enable denylist spam check by default#2411
Conversation
## Walkthrough
A private method named `update_settings_for_new_install()` was added to the `FrmMigrate` class. This method updates plugin settings by setting the `denylist_check` property to `1` during the upgrade process, specifically for new installations where no previous database version exists.
## Changes
| File(s) | Change Summary |
|-----------------------------|----------------------------------------------------------------------------------------------|
| classes/models/FrmMigrate.php | Added private method `update_settings_for_new_install()` to update settings for new installs; invoked during upgrade after first activation timestamp is set. |
## Suggested labels
`run analysis`, `run tests`
## Possibly related PRs
- Strategy11/formidable-forms#2361: Defines and manages the `denylist_check` property in `FrmSettings` and related spam check logic, directly relating to the new method setting this property on new installs.
## Suggested reviewers
- CrabcyborgWarning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.15)Note: Using configuration file /phpstan.neon. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (9)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
classes/models/FrmSettings.php (1)
178-178: LGTM! Consider user communication for the behavioral change.The change from
0to1correctly implements the PR objective to enable the denylist spam check by default for users who haven't saved their global settings. This improves the default security posture by enabling spam protection out of the box.However, since this is a behavioral change that will affect existing users, consider:
- Documenting this change in release notes
- Ensuring adequate testing covers the migration scenario
- Verifying that the denylist functionality performs well under increased load
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/models/FrmSettings.php(1 hunks)
⏰ 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). (2)
- GitHub Check: Psalm
- GitHub Check: Run ESLint
|
@truongwp Can we do this in some other way, where it stays off for old sites? I really just want new sites, with no forms, to have this as on by default. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
classes/models/FrmMigrate.php (1)
74-83: Update the @SInCE tag and consider error handling.The method logic is correct and straightforward. However, the @SInCE tag needs to be updated with the actual version number.
- * @since x.x + * @since 6.x.xAdditionally, consider adding basic error handling around the settings operations:
private function update_settings_for_new_install() { $settings = FrmAppHelper::get_settings(); + if ( ! $settings ) { + return; + } $settings->denylist_check = 1; $settings->store(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/models/FrmMigrate.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
classes/models/FrmMigrate.php (2)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4600)get_settings(200-210)classes/models/FrmSettings.php (1)
store(491-500)
⏰ 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). (10)
- GitHub Check: Cypress
- GitHub Check: Run PHPCS inspection
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Run PHP Syntax inspection (8.3)
- GitHub Check: Run PHPCS inspection
- GitHub Check: Run PHP Syntax inspection (8.3)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (1)
classes/models/FrmMigrate.php (1)
56-57: LGTM: Method call placement is correct.The method call is appropriately placed within the new installation condition, ensuring the denylist spam check is only enabled by default for fresh installations, not existing sites.
2 participants
Fixes https://github.com/Strategy11/formidable-pro/issues/5875