fix(textsanitizer): repair [code] and [quote] rendering on PHP 8.3+ - #150
Open
mambax7 wants to merge 3 commits into
Open
fix(textsanitizer): repair [code] and [quote] rendering on PHP 8.3+#150mambax7 wants to merge 3 commits into
mambax7 wants to merge 3 commits into
Conversation
highlight_string() changed in PHP 8.3. It now returns "<pre><code style=...>" and relies on real newlines, where earlier versions returned "<code><span style=...>" and encoded line breaks as <br /> and indentation as . Three defects followed on 8.3+: * The extension located the opening marker with strpos($buffer, '<?php ') and then computed $length_open = $pos_open + 14. On 8.3+ that strpos() returns false, so false + 14 evaluated to 14 and the first 14 characters of the real output were cut, leaving a block that began with the literal text le="color: #000000">. The marker is now found with a pattern that accepts either encoding and its length is taken from the match; when it is genuinely absent no offset is applied instead of guessing one. * The raw newlines inside the new <pre> wrapper are normalised away by nl2Br(), collapsing a whole block onto a single line. The 8.3+ output is converted back to the contract the rest of the pipeline expects: the wrapper is removed, newlines become <br />, and leading spaces and tabs become so indentation survives without the <pre>. * nl2Br() runs before codeConv(), so the newline typed after [/code] or [/quote] has already become a <br /> by the time the block-level div is built, rendering an empty line on top of the div's own spacing. trimBlockBreaks() removes exactly one trailing break. It anchors on </code></div> and </blockquote></div> rather than a bare </div>, because quotes nest and user content may contain its own divs. Only the trailing side is trimmed: a <br /> before a block merely ends the previous line, so removing one there would close an author's deliberate blank line. A blank line left after a block is preserved. Verified on PHP 8.2, 8.3, 8.4 and 8.5.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Contributor
Reviewer's GuideAdjusts syntax highlighting and text sanitization to be compatible with PHP 8.3+ highlight_string() output and removes stray line breaks around [code]/[quote] blocks. Sequence diagram for updated displayTarea text processing pipelinesequenceDiagram
participant Caller
participant MyTextSanitizer
Caller->>MyTextSanitizer: displayTarea(text, html, smiley, xcode, image)
MyTextSanitizer->>MyTextSanitizer: nl2Br(text)
MyTextSanitizer->>MyTextSanitizer: codeConv(text, xcode)
MyTextSanitizer->>MyTextSanitizer: trimBlockBreaks(text)
MyTextSanitizer->>MyTextSanitizer: makeClickable(text)
MyTextSanitizer-->>Caller: rendered text
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The PHP 8.3
<pre>normalization assumes a bare<pre>tag; to avoid future breakage ifhighlight_string()adds attributes, consider matching and stripping<pre[^>]*>/</pre>instead of the exact tag names. - Applying
trimBlockBreaks()to the entire rendered text means any user-generated</code></div><br>/</blockquote></div><br>fragments will be altered as well; you may want to tighten the pattern (e.g. anchor it to the known xoopsCode/xoopsQuote wrappers) or restrict where this post-processing runs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The PHP 8.3 `<pre>` normalization assumes a bare `<pre>` tag; to avoid future breakage if `highlight_string()` adds attributes, consider matching and stripping `<pre[^>]*>` / `</pre>` instead of the exact tag names.
- Applying `trimBlockBreaks()` to the entire rendered text means any user-generated `</code></div><br>` / `</blockquote></div><br>` fragments will be altered as well; you may want to tighten the pattern (e.g. anchor it to the known xoopsCode/xoopsQuote wrappers) or restrict where this post-processing runs.
## Individual Comments
### Comment 1
<location path="htdocs/class/textsanitizer/syntaxhighlight/syntaxhighlight.php" line_range="95-102" />
<code_context>
+ // Indentation too: 8.2 encoded leading whitespace as (it had no <pre> to
+ // rely on), 8.3+ emits real spaces. Having just removed the <pre>, those spaces
+ // would collapse and every line would start at column 0.
+ $buffer = preg_replace_callback(
+ '/(<br \/>)([ \t]+)/',
+ static fn (array $m): string => $m[1] . str_replace(
+ [' ', "\t"],
+ [' ', ' '],
+ $m[2]
+ ),
+ $buffer
+ );
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Leading indentation on the first line of a code block is still at risk of being collapsed.
The current callback only converts whitespace following `<br />`, so leading spaces on the first line stay as plain spaces and collapse once `<pre>` is removed, unlike 8.2 where all leading whitespace was encoded as ` `. Please extend the logic to also convert leading indentation before the first line break (e.g. immediately after `<code>`), so that every line in the non-`<pre>` representation preserves its indentation.
</issue_to_address>
### Comment 2
<location path="htdocs/class/module.textsanitizer.php" line_range="736-738" />
<code_context>
+ * @param string $text rendered text
+ * @return string
+ */
+ protected function trimBlockBreaks($text)
+ {
+ return preg_replace('#(</(?:code|blockquote)>\s*</div>)\s*<br\s*/?>#i', '$1', (string) $text);
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The regex currently strips all trailing `<br />`s, not just a single stray one as described.
Because ` *<br\s*/?>` is used with global `preg_replace`, any trailing sequence like `</code></div><br /><br />` after the block will be entirely removed, not just a single stray `<br />` as documented. If you intend to keep intentional extra blank lines, constrain the pattern to a single `<br />` or use `preg_replace` with a replacement limit of 1.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #150 +/- ##
=========================================
Coverage 19.29% 19.29%
- Complexity 8227 8234 +7
=========================================
Files 672 672
Lines 44266 44293 +27
=========================================
+ Hits 8539 8547 +8
- Misses 35727 35746 +19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
The PR introduces a stray/orphaned PHPDoc block and contains an inaccurate pipeline description in a new docblock, and it would benefit from regression tests covering the updated [code]/[quote] behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR updates XOOPS text sanitization to keep [code] and [quote] blocks rendering correctly across PHP 8.2–8.5, addressing PHP 8.3+ highlight_string() output changes and removing an extra trailing line break after rendered block elements.
Changes:
- Normalize PHP 8.3+
highlight_string()output in the syntaxhighlight extension back into the older “<br />+ indentation” contract expected by the downstream pipeline. - Replace the fixed “magic number” offset used to strip an injected
<?phpopener with a match-length-based approach. - Add a post-processing step in
displayTarea()to trim exactly one stray<br />immediately after rendered[code]/[quote]blocks.
File summaries
| File | Description |
|---|---|
| htdocs/class/textsanitizer/syntaxhighlight/syntaxhighlight.php | Normalizes PHP 8.3+ highlight_string() HTML output and fixes opener detection/offset handling to prevent truncation. |
| htdocs/class/module.textsanitizer.php | Adds trimBlockBreaks() after codeConv() to remove an extra trailing <br /> after [code]/[quote] block rendering. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
707
to
711
| @@ -710,6 +711,33 @@ public function codeConvCallback($match) | |||
| * @param mixed $xcode | |||
Comment on lines
+717
to
+719
| * `nl2Br()` runs BEFORE `codeConv()`/`quoteConv()` in {@see self::displayTarea()}, so by the | ||
| * time those wrap their content in `<div class="xoopsCode">` / `<div class="xoopsQuote">` | ||
| * the newline the author typed after `[/code]` or `[/quote]` has already become a `<br />`. |
| $text = $this->nl2Br($text); | ||
| } | ||
| $text = $this->codeConv($text, $xcode); | ||
| $text = $this->trimBlockBreaks($text); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@htdocs/class/module.textsanitizer.php`:
- Around line 736-738: Update trimBlockBreaks() to also remove a trailing <br>
after the wrapped unhighlighted code-block form, matching </pre></div><br> in
addition to the existing code and blockquote endings. Preserve the current
behavior for existing </code> and </blockquote> patterns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e7871cbd-7ceb-48db-aa26-0d23a82d502d
📒 Files selected for processing (2)
htdocs/class/module.textsanitizer.phphtdocs/class/textsanitizer/syntaxhighlight/syntaxhighlight.php
Comment on lines
+736
to
+738
| protected function trimBlockBreaks($text) | ||
| { | ||
| return preg_replace('#(</(?:code|blockquote)>\s*</div>)\s*<br\s*/?>#i', '$1', (string) $text); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle code blocks when syntax highlighting is disabled.
When highlight is disabled, MytsSyntaxhighlight::load() returns <pre>...</pre>. codeConvCallback() then wraps it in <div class="xoopsCode">. This pattern does not match </pre></div><br>, so that code path retains the extra trailing break.
Proposed fix
-return preg_replace('#(</(?:code|blockquote)>\s*</div>)\s*<br\s*/?>`#i`', '$1', (string) $text);
+return preg_replace('#(</(?:code|pre|blockquote)>\s*</div>)\s*<br\s*/?>`#i`', '$1', (string) $text);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| protected function trimBlockBreaks($text) | |
| { | |
| return preg_replace('#(</(?:code|blockquote)>\s*</div>)\s*<br\s*/?>#i', '$1', (string) $text); | |
| protected function trimBlockBreaks($text) | |
| { | |
| return preg_replace('#(</(?:code|pre|blockquote)>\s*</div>)\s*<br\s*/?>`#i`', '$1', (string) $text); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@htdocs/class/module.textsanitizer.php` around lines 736 - 738, Update
trimBlockBreaks() to also remove a trailing <br> after the wrapped unhighlighted
code-block form, matching </pre></div><br> in addition to the existing code and
blockquote endings. Preserve the current behavior for existing </code> and
</blockquote> patterns.
trimBlockBreaks() anchored on `</code>` and `</blockquote>`, but MytsSyntaxhighlight::load() returns a plain `<pre>...</pre>` when its `highlight` option is off, so on those sites the box closes `</pre></div>` and the stray break survived -- the very blank line this was meant to remove. `pre` joins the alternation. The docblock also described the pipeline wrongly. quoteConv() runs inside xoopsCodeDecode(), which displayTarea() calls before nl2Br(); only codeConv() runs after it. The trailing break reaches the same place by two different routes, and the comment now says so. Restores the codeConv() docblock, which had been left stranded above trimBlockBreaks() when that method was inserted. Eight tests lock the contract: the break is trimmed after code, quote and pre boxes; exactly one break is removed when several follow; a break that precedes a box and an unrelated user `</div><br />` are both left alone.
…our own boxes Two defects in the 8.3 normalisation, both found by adversarially diffing the rendered output against the PHP 8.2 reference rather than reading the code. Whitespace inside a line was still being lost. Having no <pre> to lean on, 8.2 encoded every space in the highlighted source as and every tab as four; the normalisation only re-encoded the run that followed a <br />, so leading indentation survived but nothing else did. Once the <pre> is stripped the rest collapses, and makeClickable() folds runs of whitespace anyway, so `$a = 1;` above `$bb = 2;` rendered ragged and `echo "a b"` lost its second space. All whitespace in the text between tags is now encoded, and only there -- the spaces inside `<span style="color: #007700">` have to survive, since rewriting those is exactly what corrupts the markup. 8.3, 8.4 and 8.5 output now matches the 8.2 reference for indentation, alignment, tabs and interior runs alike. trimBlockBreaks() was anchored only on the closing `</code></div>`, which is not unique to a generated box. An author writing their own `<div><code>…</code></div>` followed by a break -- reachable wherever HTML is permitted -- had that break silently eaten. The pattern now has to open on the xoopsCode/xoopsQuote wrapper as well. The span between the anchors stays lazy so nested quotes still trim: the inner close is not followed by a break, so the match grows out to the outer one that is. Tests cover both, and the reflection helpers drop setAccessible(), which is a no-op since 8.1 and raises a deprecation on 8.5.
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
trimBlockBreaks() is currently applied unconditionally and can mutate user-authored HTML even when $xcode is disabled, which is an unintended side effect.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
htdocs/class/module.textsanitizer.php:631
trimBlockBreaks()is intended to clean up rendering artifacts introduced by[code]/[quote]processing, which only happens when$xcode != 0. Calling it unconditionally can change user-authored HTML when$htmlis allowed but$xcodeis disabled (e.g. a literal<div class="xoopsCode">…</div><br />would be modified), which is an unintended side effect.
$text = $this->codeConv($text, $xcode);
$text = $this->trimBlockBreaks($text);
$text = $this->makeClickable($text);
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
highlight_string() changed in PHP 8.3. It now returns "