Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #884 +/- ##
============================================
- Coverage 30.60% 30.47% -0.13%
+ Complexity 6641 6627 -14
============================================
Files 293 292 -1
Lines 26493 26432 -61
============================================
- Hits 8107 8056 -51
+ Misses 18386 18376 -10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
A newly added test regex is too strict (assumes hex-only placeholder nonce) and can fail on environments where placeholder nonces are alphanumeric.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the ContactsEncoder shortcode pipeline to prevent buffer-mode regex pairing from “swallowing” unrelated HTML between shortcode open/close tags (notably in comment-list/block markup), and simplifies some internal placeholder/entropy handling.
Changes:
- Skip processing of shortcode pairs whose inner content contains HTML tags to avoid cross-fragment pairing in buffer mode.
- Remove the dedicated comment-list integration (
CEIntegrationCommentList) and replace coverage with broader regression tests around buffer mode behavior. - Refactor aria-label placeholder generation to use a shared secure-bytes helper.
File summaries
| File | Description |
|---|---|
| tests/ApbctWP/ContactsEncoder/TestContactsEncoderShortCodeEncode.php | Adds regression tests for buffer mode not absorbing markup/comments; adds tests for HTML-vs-plain-text shortcode extraction. |
| tests/ApbctWP/ContactsEncoder/Shortcodes/ExcludedEncodeContentSCTest.php | Adds a test ensuring skip-encoding shortcode with HTML content is not extracted. |
| tests/ApbctWP/ContactsEncoder/Integrations/TestCEIntegrationCommentList.php | Removes tests for the deleted comment-list integration. |
| lib/Cleantalk/Common/ContactsEncoder/ContactsEncoder.php | Refactors secure aria-label placeholder generation into a reusable getSecureRandomBytes() helper. |
| lib/Cleantalk/ApbctWP/ContactsEncoder/Shortcodes/ShortCodesService.php | Removes the comment-list integration from buffer pre/post processing. |
| lib/Cleantalk/ApbctWP/ContactsEncoder/Shortcodes/ExcludedEncodeContentSC.php | Skips placeholder extraction for skip-encoding shortcode content containing HTML tags. |
| lib/Cleantalk/ApbctWP/ContactsEncoder/Shortcodes/EncodeContentSC.php | Skips placeholder extraction for encode-data shortcode content containing HTML tags. |
| lib/Cleantalk/ApbctWP/ContactsEncoder/Shortcodes/EmailEncoderShortCode.php | Adds shortcodeContentContainsHtmlTags() and uses it to skip executing shortcode pairs that include HTML in the inner content. |
| lib/Cleantalk/ApbctWP/ContactsEncoder/Integrations/CEIntegrationCommentList.php | Deletes the comment-list integration. |
| js/apbct-public-bundle.min.js | Updates the bundled/minified public JS artifact. |
Review details
- Files reviewed: 9/17 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.
🟡 Changes recommended
The newly added buffer-mode tests leak global $apbct->buffer state (not restored), which can make the test suite order-dependent and flaky.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
tests/ApbctWP/ContactsEncoder/TestContactsEncoderShortCodeEncode.php:435
- Same as above: this test enables buffer mode and calls
modifyBufferAfter(), which assigns to the global$apbct->buffer. Restoring only the setting can leak global state into subsequent tests. Save/restore the prior buffer value in thefinallyblock to keep the test isolated.
public function testBufferModeDoesNotAbsorbArbitraryHtmlBetweenEncodeDataTags()
{
global $apbct;
$previous_buffer_setting = $apbct->settings['data__email_decoder_buffer'];
$apbct->settings['data__email_decoder_buffer'] = true;
- Files reviewed: 9/17 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core buffer-mode shortcode extraction behavior and removes an integration layer, which can have broad rendering impact across themes/content patterns and warrants final human verification.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
lib/Cleantalk/ApbctWP/ContactsEncoder/Shortcodes/EncodeContentSC.php:124
- The early-return on HTML tags is doing important work to avoid buffer-mode over-capture, but the intent isn’t obvious from the local code. Adding a short comment here will make the extraction rule clearer for future maintenance.
lib/Cleantalk/ApbctWP/ContactsEncoder/Shortcodes/ExcludedEncodeContentSC.php:95 - The method docblock says skip-encoding shortcodes are replaced with placeholders before encoding, but the new HTML-tag guard intentionally leaves some shortcode pairs untouched. Updating the docblock to document this behavior will prevent future callers/tests from assuming all pairs are extracted.
- Files reviewed: 9/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
task https://app.doboard.com/1/task/55588