Fix unbalanced markdown when an emphasis span is next to a whitespace-only one - #274
Open
youdie006 wants to merge 3 commits into
Open
Fix unbalanced markdown when an emphasis span is next to a whitespace-only one#274youdie006 wants to merge 3 commits into
youdie006 wants to merge 3 commits into
Conversation
The merge-adjacent optimization suppressed a span's opening/closing emphasis marker whenever the neighbouring sibling was of the same type, assuming that neighbour emitted its own markers. A whitespace-only sibling (e.g. <strong> </strong>) returns its bare value and emits none, so <strong> </strong><strong>hello</strong> lost its opening ** and produced the unbalanced " hello**". Only merge when the same-type sibling has non-whitespace content, so it actually emitted markers. Add regression tests.
There was a problem hiding this comment.
Pull request overview
This PR fixes unbalanced Markdown output produced by EmphasisConverter when its “merge adjacent emphasis” optimization encounters an adjacent same-type emphasis span whose content is whitespace-only (and therefore emits no emphasis markers), addressing issue #252.
Changes:
- Adjusts the adjacent-emphasis merge logic to only merge with same-type siblings that have non-whitespace content.
- Adds PHPUnit coverage for whitespace-only adjacent emphasis spans in
testConsecutiveSpans.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Converter/EmphasisConverter.php |
Adds a sibling “mergeability” check so marker suppression only happens when the adjacent same-type sibling would actually emit markers. |
tests/HtmlConverterTest.php |
Adds regression tests covering whitespace-only adjacent emphasis spans that previously caused unbalanced markers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+71
to
+76
| private function isMergeableSibling(?ElementInterface $sibling, string $tag): bool | ||
| { | ||
| return $sibling !== null | ||
| && $this->getNormTag($sibling) === $tag | ||
| && \trim($sibling->getValue()) !== ''; | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
isMergeableSibling used `trim($value) !== ''` to decide whether a same-type sibling emits emphasis markers, but convert()'s early return uses `! \trim($value)`, which is also true for the string "0" (falsy in PHP). So `<em>0</em><em>hello</em>` treated the "0" sibling as mergeable and suppressed the adjacent marker, producing the unbalanced `0hello*`. Mirror convert()'s truthiness check with `(bool) \trim(...)` so a falsy sibling (whitespace-only or "0") no longer suppresses the neighbour's markers, and add regression tests for the "0" case in both positions.
Author
|
Thanks for the review. Addressed the automated feedback:
|
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.
Fixes #252.
EmphasisConverterhas a "merge adjacent same-type emphasis" optimization (added in #202) so that<em>foo</em><em>bar</em>becomes*foobar*rather than*foo**bar*. It suppresses a span's opening marker when the previous sibling is the same emphasis type (and its closing marker when the next sibling is), assuming that neighbour emitted markers of its own.But a whitespace-only span short-circuits earlier (
if (! \trim($value)) { return $value; }) and emits no markers. So when such a span is adjacent to a real one, the real span still drops its marker and the output is unbalanced:Fix: only merge with a same-type sibling that has non-whitespace content (i.e. one that actually emitted markers). Content-bearing merges (
<em>foo</em><em>bar</em>->*foobar*) are unchanged.Tests: added whitespace-only-neighbour cases to
testConsecutiveSpans(<strong> </strong><strong>hello</strong>->**hello**, plus the<em>variant). Verified red before the fix (" hello**") and green after.composer phpunit(50 tests, 231 assertions),phpstan, andpsalmall pass locally.Disclosure: developed with the assistance of Claude Code (AI); reviewed and verified by me.