Fix. Code. Fixed compatibility with php8.5. - #888
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #888 +/- ##
============================================
+ Coverage 30.51% 30.56% +0.05%
- Complexity 6635 6637 +2
============================================
Files 292 292
Lines 26445 26489 +44
============================================
+ Hits 8069 8097 +28
- Misses 18376 18392 +16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
Configured exclusions are not wired through the factory or shortcode path, and matching behavior and test coverage need correction.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes PHP 8.5 compatibility and adds configurable email and phone exclusions to the contact encoder.
Changes:
- Avoids deprecated
finfo_close()usage and adds MIME detection coverage. - Adds exclusion parsing, normalization, and matching.
- Applies exclusions during global encoding.
File summaries
| File | Summary | Review findings |
|---|---|---|
tests/Common/HelperTest.php |
Adds MIME detection regression coverage. | — |
lib/Cleantalk/Common/Helper.php |
Updates MIME cleanup and DNS documentation. | Nit (3 votes): Remove the invalid $out annotation from dnsResolve(). |
lib/Cleantalk/Common/ContactsEncoder/Exclusions/ExclusionsService.php |
Implements exclusion parsing and matching. | Moderate (1 vote): Prevent cross-type matches for phone-like exclusions. Nit (3 votes): Add regression tests for normalized exclusions and encoding paths. |
lib/Cleantalk/Common/ContactsEncoder/Dto/Params.php |
Adds excluded-string configuration. | Moderate (2 votes): Populate excluded_strings in the WordPress encoder factory. |
lib/Cleantalk/Common/ContactsEncoder/ContactsEncoder.php |
Skips encoding for excluded global contacts. | Moderate (2 votes): Ensure shortcode encoding also applies exclusion checks. |
Review details
Suppressed comments (1)
lib/Cleantalk/Common/ContactsEncoder/Exclusions/ExclusionsService.php:151
- The substring check runs before the phone/email distinction below, so a phone-like exclusion such as
12345678also excludes an email containing those digits (user12345678@example.com). That contradicts the following comment about preventing cross-type matches; classify phone-like exclusions before applying substring matching or otherwise block cross-type comparisons.
if ( strpos($normalized_match, $normalized_exclusion) !== false ) {
return true;
- Files reviewed: 5/5 changed files
- Comments generated: 4
- 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.
🟡 Changes recommended
Moderate issues remain in exclusion matching, UTF-8 truncation, and cache detection.
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
Suppressed comments (4)
inc/cleantalk-settings.php:3300
substr()counts bytes, not characters, so a non-ASCII exclusion can be cut in the middle of a UTF-8 code point and stored truncated/invalid even though the setting promises 128 characters. Use the project’s supported UTF-8-aware truncation routine (with an appropriate fallback) for these text fragments.
$part = trim(substr($part, 0, 128), " \n\r\t\v\x00");
lib/Cleantalk/Common/ContactsEncoder/Exclusions/ExclusionsService.php:152
- The generic substring check runs before the phone-only digit guard. An unformatted phone exclusion such as
18005551234therefore matches an unrelated email liketicket18005551234@shop.com, contrary to the stated email/phone non-cross-match behavior. Classify phone-like exclusions before substring matching while preserving domain/text-fragment matching for emails.
if ( strpos($normalized_match, $normalized_exclusion) !== false ) {
return true;
}
lib/Cleantalk/Common/Helper.php:602
$outis documented as a second parameter, but this method only accepts$host;$outis initialized locally tofalseand cannot be supplied by callers. The return annotation is consequently misleading as well, so callers may assume a fallback can be configured.
* @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
lib/Cleantalk/Common/Helper.php:597
- These new constraints do not match
dnsResolve()below: the implementation accepts bare hostnames, IPs, URLs, and host:port, andSFW.php:858passes a bareHTTP_HOST. Please document the actual accepted inputs (or change the implementation); otherwise this public contract is misleading.
* $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.
- Files reviewed: 15/18 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Four unresolved moderate findings remain in truncation and contact-exclusion handling.
Review details
Files not reviewed (1)
- css/cleantalk-admin-settings-page.min.css: Generated file
Suppressed comments (4)
inc/cleantalk-settings.php:3300
substr()limits bytes, not UTF-8 characters. A non-ASCII value near the 128-character limit can be cut in the middle of a code point, producing invalid stored text and violating the documented 128-character limit. Use UTF-8-aware truncation with a fallback that is available on supported WordPress installations.
$part = trim(substr($part, 0, 128), " \n\r\t\v\x00");
lib/Cleantalk/Common/ContactsEncoder/ContactsEncoder.php:313
- This guard only runs after
modifyContent()has calleddropAttributesContainEmail(), which removes email-bearingtitleattributes before the regex can see them. Consequently an excluded address in<a title="...">is deleted rather than kept readable, despite the new description promising exclusions in titles; skip that attribute removal for excluded matches or move the exclusion check earlier.
if ( $this->exclusions->isContactExcluded($matches[0]) ) {
return $matches[0];
}
lib/Cleantalk/Common/ContactsEncoder/Exclusions/ExclusionsService.php:159
- This branch is documented as a phone-only comparison, but the
@check classifies every other value as a phone. For example, an exclusion text such asSupport line 1234567890will digit-match(123) 456-7890and suppress encoding even though the text fragment is not present. Restrict digit normalization to values that are actually recognized as phone numbers (or classify entries when parsing) before applying this comparison.
if (
strpos($normalized_match, '@') === false
&& strpos($normalized_exclusion, '@') === false
) {
lib/Cleantalk/Common/ContactsEncoder/Exclusions/ExclusionsService.php:150
- The generic substring check runs before the phone-only digit branch, so a digits-only phone exclusion can still cross-match an email. For example, with
excluded_strings = array('12345678'),isContactExcluded('user12345678@example.com')returns true, despite the comment below promising that phone/email digit sequences do not cross-match. Classify phone-like exclusions before this check, or otherwise skip the generic substring match for them, while retaining the digit comparison only for phone matches.
if ( strpos($normalized_match, $normalized_exclusion) !== false ) {
- Files reviewed: 15/18 changed files
- Comments generated: 0 new
- Review effort level: Lite
task https://app.doboard.com/1/task/56095