Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #887 +/- ##
============================================
+ Coverage 30.51% 30.56% +0.05%
- Complexity 6632 6636 +4
============================================
Files 292 292
Lines 26446 26488 +42
============================================
+ Hits 8069 8096 +27
- Misses 18377 18392 +15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
The new excluded-strings setting can bypass the documented “20 values / 128 chars” limits when users mix commas and newlines, because validation and runtime parsing split the input differently.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds a “do not encode these contacts” option to the ContactsEncoder feature, allowing administrators to exclude specific emails/phones (and fragments like domains) from being obfuscated during content processing.
Changes:
- Introduces an exclusion list setting (
data__email_decoder_excluded_strings) and wires it into ContactsEncoder params. - Adds exclusion parsing + matching logic (including phone digit normalization and
mailto:/tel:normalization). - Updates settings UI (field + help text) and adds unit tests covering exclusion behavior.
File summaries
| File | Description |
|---|---|
| tests/ApbctWP/ContactsEncoder/TestContactsEncoder.php | Adds tests ensuring excluded strings keep emails/phones unencoded in typical “title-like” content. |
| tests/ApbctWP/ContactsEncoder/Exclusions/TestExclusionsService.php | Adds tests for parsing excluded strings and matching email/phone variants. |
| lib/Cleantalk/Common/Helper.php | Updates dnsResolve() PHPDoc return/param types. |
| lib/Cleantalk/Common/ContactsEncoder/Exclusions/ExclusionsService.php | Adds excluded-strings parsing and contact exclusion matching logic. |
| lib/Cleantalk/Common/ContactsEncoder/Dto/Params.php | Adds excluded_strings param to pass exclusion list into the encoder. |
| lib/Cleantalk/Common/ContactsEncoder/ContactsEncoder.php | Skips encoding for matches that are deemed excluded by ExclusionsService. |
| lib/Cleantalk/ApbctWP/State.php | Adds default WP setting for the new excluded-strings textarea. |
| lib/Cleantalk/ApbctWP/PluginSettingsPage/SettingsField.php | Adjusts textarea title rendering to add a modifier class when help is present. |
| lib/Cleantalk/ApbctWP/ContactsEncoder/ContactsEncoder.php | Adds description/long-description strings for the new setting UI. |
| inc/cleantalk-settings.php | Registers the new setting, validates/sanitizes it, and adds long-description metadata. |
| css/src/cleantalk-admin-settings-page.css | Adds styling for the textarea title when help icon is present. |
| css/cleantalk-admin-settings-page.min.css | Minified CSS update reflecting the new modifier class styling. |
| cleantalk.php | Passes parsed excluded strings into ContactsEncoder params during instantiation. |
Review details
Files not reviewed (1)
- css/cleantalk-admin-settings-page.min.css: Generated file
- Files reviewed: 12/13 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟢 Approval recommended
The change is cohesive, covered by new tests, and integrates the new setting end-to-end with no confirmed functional regressions.
Review details
Files not reviewed (1)
- css/cleantalk-admin-settings-page.min.css: Generated file
- Files reviewed: 13/14 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The feature is implemented end-to-end (settings, sanitization, runtime behavior) with targeted tests, and only minor documentation clarity feedback remains.
Review details
Files not reviewed (1)
- css/cleantalk-admin-settings-page.min.css: Generated file
- Files reviewed: 13/14 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new code introduces a UTF-8 truncation risk in excluded-string sanitization, and the touched dnsResolve() implementation appears incompatible with common hostname inputs (breaking its primary call site).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- css/cleantalk-admin-settings-page.min.css: Generated file
- Files reviewed: 13/14 changed files
- Comments generated: 3
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Exclusion matching currently applies digit-based phone comparisons to email matches too, which can incorrectly skip encoding for emails containing long digit sequences.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- css/cleantalk-admin-settings-page.min.css: Generated file
- Files reviewed: 13/14 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Helper::dnsResolve() now rejects bare hostnames (e.g., HTTP_HOST), breaking at least one existing call site and changing runtime behavior beyond the PR’s stated scope.
Review details
Files not reviewed (1)
- css/cleantalk-admin-settings-page.min.css: Generated file
Suppressed comments (1)
lib/Cleantalk/Common/Helper.php:610
- dnsResolve() now rejects bare hostnames/IPs because it requires FILTER_VALIDATE_URL before doing any parsing. This breaks existing callers that pass HTTP_HOST (e.g. lib/Cleantalk/ApbctWP/Firewall/SFW.php:858) and also makes the later domain/IP handling unreachable unless the input is wrapped into a URL.
/**
* Resolve DNS to IP.
*
* $host must be a full URL that passes FILTER_VALIDATE_URL (scheme required).
* A bare hostname such as HTTP_HOST, or any non-string value, is rejected
* and $out is returned.
*
* @param mixed $host Full URL (FILTER_VALIDATE_URL); other types are rejected
* @param bool|string $out Fallback when $host is invalid or lookup fails
*
* @return bool|string First A-record IPv4 on success, otherwise $out
* @psalm-suppress PossiblyUnusedMethod
*/
public static function dnsResolve($host, $out = false)
{
// Check if the $url is set and it is an url
if ( ! is_string($host) || $host === '' || ! filter_var($host, FILTER_VALIDATE_URL)) {
return $out;
}
- Files reviewed: 13/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
task https://app.doboard.com/1/task/44985