Skip to content

fix(document): preserve comments, plain-scalar leaders and comment co… #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ralflang merged 1 commit into from
Oct 2, 2026
Merged

fix(document): preserve comments, plain-scalar leaders and comment co… #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 49 additions & 8 deletions src/Document/Emitter/Emitter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ private function emitScalar(ScalarNode $node): void
{
$properties = $this->renderValueProperties($node);
if ($this->isBlockStyleScalar($node)) {
$raw = $node->getRawSource();
$raw = $this->rawSource($node);
if ($raw !== null) {
$this->buffer .= $properties . $raw;
} else {
Expand Down Expand Up @@ -322,7 +322,7 @@ private function emitMapEntry(MapEntry $entry, int $indent): void
// Block scalars span multiple lines and end with their own
// newline.
if ($this->isBlockStyleScalar($value)) {
$rawSource = $value->getRawSource();
$rawSource = $this->rawSource($value);
if ($rawSource !== null) {
$this->buffer .= ' ' . $properties . $rawSource;
} else {
Expand Down Expand Up @@ -623,14 +623,42 @@ private function emitSequenceItem(SequenceItem $item, int $indent): void

private function emitStandaloneComment(CommentNode $comment, int $indent): void
{
// Use the comment's stored indent if it has one; otherwise
// the surrounding container's indent.
$useIndent = $comment->getIndent() > 0 ? $comment->getIndent() : $indent;
// Use the comment's own column when the parser recorded one, so a
// comment comes back where its author put it. null means no column was
// recorded - a comment built programmatically rather than read from a
// document - and those follow the surrounding container.
//
// Nullable rather than `> 0`: column 1 is a real position, and a
// flush-left comment inside an indented block is a deliberate style
// that was previously indistinguishable from "unset".
$useIndent = $comment->getIndent() ?? $indent;
$this->buffer .= str_repeat(' ', $useIndent);
$this->buffer .= $comment->getText();
$this->buffer .= $this->lineEnding;
}

/**
* A node's raw source, in the line ending this document uses.
*
* Raw source is captured after the scanner has normalised CRLF to LF, so a
* multi-line scalar re-emitted verbatim carried LF into a CRLF document -
* a block scalar was the one construct that still broke a Windows file's
* round trip after the stream learned its line ending.
*
* A no-op for LF documents, which is every document until one says
* otherwise.
*/
private function rawSource(ScalarNode $node): ?string
{
$raw = $node->getRawSource();

if ($raw === null || $this->lineEnding === "\n") {
return $raw;
}

return str_replace("\n", $this->lineEnding, $raw);
}

private function emitBlankLines(BlankLineNode $node): void
{
// count() blank lines = count() newline characters since the
Expand All @@ -640,7 +668,7 @@ private function emitBlankLines(BlankLineNode $node): void

private function scalarOrRaw(ScalarNode $node): string
{
$rawSource = $node->getRawSource();
$rawSource = $this->rawSource($node);
if ($rawSource !== null) {
return $rawSource;
}
Expand Down Expand Up @@ -817,7 +845,7 @@ private function emitBlockScalarSynthesized(ScalarNode $node, int $parentIndent)

private function renderBlockScalar(ScalarNode $node, int $parentIndent): string
{
$rawSource = $node->getRawSource();
$rawSource = $this->rawSource($node);
if ($rawSource !== null) {
return $rawSource;
}
Expand Down Expand Up @@ -1003,10 +1031,23 @@ private function isPlainSafe(string $value, ?string $tag = null): bool
return false;
}
$first = $value[0];
$reservedLeaders = ['[', ']', '{', '}', '#', '&', '*', '!', '|', '>', "'", '"', '%', '@', '`', '?', ':', '-', ',', "\t", ' '];
$reservedLeaders = ['[', ']', '{', '}', '#', '&', '*', '!', '|', '>', "'", '"', '%', '@', '`', ',', "\t", ' '];
if (in_array($first, $reservedLeaders, true)) {
return false;
}
// "?", ":" and "-" are indicators only when they stand alone or are
// followed by a space. YAML 1.2 ns-plain-first admits them otherwise,
// so "--cert-dir=/tmp" and "-single-dash" are valid plain scalars and
// quoting them rewrites a line the author did not write that way.
if (in_array($first, ['?', ':', '-'], true)) {
if (strlen($value) === 1) {
return false;
}
$second = $value[1];
if ($second === ' ' || $second === "\t") {
return false;
}
}
if (str_contains($value, ': ') || str_contains($value, " #")) {
return false;
}
Expand Down
8 changes: 4 additions & 4 deletions src/Document/Node/CommentNode.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ final class CommentNode implements Node
use NodeTrait;

private string $text;
private int $indent;
private ?int $indent;
private string $gap;

/**
Expand All @@ -50,7 +50,7 @@ final class CommentNode implements Node
* Empty for synthesized nodes; the emitter falls
* back to a sensible default when emitting.
*/
public function __construct(string $text = '', int $indent = 0, string $gap = '')
public function __construct(string $text = '', ?int $indent = null, string $gap = '')
{
$this->text = $text;
$this->indent = $indent;
Expand All @@ -67,12 +67,12 @@ public function setText(string $text): void
$this->text = $text;
}

public function getIndent(): int
public function getIndent(): ?int
{
return $this->indent;
}

public function setIndent(int $indent): void
public function setIndent(?int $indent): void
{
$this->indent = $indent;
}
Expand Down
8 changes: 4 additions & 4 deletions src/Document/Parser/Parser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,7 @@ private function migrateTrivia(array $trivia, MapNode|SequenceNode $container):
{
foreach ($trivia as $t) {
if ($t->type === TriviaType::Comment) {
$node = new CommentNode($t->text, 0, $t->gap);
$node = new CommentNode($t->text, max(0, $t->column - 1), $t->gap);
$node->setPosition($t->line, $t->column);
$container->appendChildInternal($node);
continue;
Expand Down Expand Up @@ -652,7 +652,7 @@ private function drainTriviaIntoDoc(array $trivia, YamlDocument $doc): void
private function triviaToNode(TriviaToken $t): CommentNode|BlankLineNode|null
{
if ($t->type === TriviaType::Comment) {
$node = new CommentNode($t->text, 0, $t->gap);
$node = new CommentNode($t->text, max(0, $t->column - 1), $t->gap);
$node->setPosition($t->line, $t->column);
return $node;
}
Expand Down Expand Up @@ -885,7 +885,7 @@ private function extractEolCommentFromTrailingTrivia(Token $token): ?CommentNode
{
foreach ($token->trailingTrivia as $t) {
if ($t->type === TriviaType::Comment) {
$node = new CommentNode($t->text, 0, $t->gap);
$node = new CommentNode($t->text, max(0, $t->column - 1), $t->gap);
$node->setPosition($t->line, $t->column);
return $node;
}
Expand Down Expand Up @@ -955,7 +955,7 @@ private function parseBlockMapEntry(): MapEntry
$first->type === TriviaType::Comment
&& $first->line === $valueToken->line
) {
$eolComment = new CommentNode($first->text, 0, $first->gap);
$eolComment = new CommentNode($first->text, max(0, $first->column - 1), $first->gap);
$eolComment->setPosition($first->line, $first->column);
// Strip this one trivia entry from the inner token so
// it isn't re-emitted as a child of the nested value.
Expand Down
13 changes: 13 additions & 0 deletions src/Document/Parser/Pipeline.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,19 @@ public function parse(string $source): YamlStream
if ($source !== '' && str_ends_with($source, "\n")) {
$stream->setTrailingNewline(true);
}
// Round-trip: the scanner normalises CRLF to LF so the rest of the
// parser sees one line ending, and the emitter already writes whatever
// the stream names - but nothing recorded what the source used, so a
// CRLF document was silently rewritten with LF.
//
// Set only when EVERY newline is CRLF. A file with mixed endings has no
// single ending to preserve, and quietly picking one would rewrite the
// other; LF leaves that document visibly changed rather than subtly, so
// a caller comparing input to output can still tell.
$lf = substr_count($source, "\n");
if ($lf > 0 && substr_count($source, "\r\n") === $lf) {
$stream->setLineEnding("\r\n");
}
$this->resolver->resolve($stream);

return $stream;
Expand Down
14 changes: 7 additions & 7 deletions src/Document/Parser/Scanner.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ public function scan(string $source): array
$commentColumn = $this->column;
$text = '';
while ($this->pos < $this->length && $this->source[$this->pos] !== "\n") {
$text .= $this->source[$this->pos];
$text .= $this->currentChar();
$this->advance();
}
$this->triviaBuffer[] = TriviaToken::comment(
Expand Down Expand Up @@ -810,7 +810,7 @@ private function scanBlockSequenceItem(array &$tokens): void
$commentColumn = $this->column;
$text = '';
while ($this->pos < $this->length && $this->source[$this->pos] !== "\n") {
$text .= $this->source[$this->pos];
$text .= $this->currentChar();
$this->advance();
}
$this->triviaBuffer[] = TriviaToken::comment(
Expand Down Expand Up @@ -1746,7 +1746,7 @@ private function scanBlockMapEntry(array &$tokens): void
$commentColumn = $this->column;
$text = '';
while ($this->pos < $this->length && $this->source[$this->pos] !== "\n") {
$text .= $this->source[$this->pos];
$text .= $this->currentChar();
$this->advance();
}
$this->triviaBuffer[] = TriviaToken::comment(
Expand Down Expand Up @@ -1821,7 +1821,7 @@ private function scanBlockMapEntry(array &$tokens): void
$commentColumn = $this->column;
$text = '';
while ($this->pos < $this->length && $this->source[$this->pos] !== "\n") {
$text .= $this->source[$this->pos];
$text .= $this->currentChar();
$this->advance();
}
$this->triviaBuffer[] = TriviaToken::comment(
Expand Down Expand Up @@ -2079,7 +2079,7 @@ private function collectTrivia(): bool
$startColumn = $this->column;
$text = '';
while ($this->pos < $this->length && $this->source[$this->pos] !== "\n") {
$text .= $this->source[$this->pos];
$text .= $this->currentChar();
$this->advance();
}
$this->triviaBuffer[] = TriviaToken::comment(
Expand Down Expand Up @@ -2193,7 +2193,7 @@ private function consumeDirective(): Token
$this->advance();
$value = '';
while ($this->pos < $this->length && $this->source[$this->pos] !== "\n") {
$value .= $this->source[$this->pos];
$value .= $this->currentChar();
$this->advance();
}
if ($this->pos < $this->length && $this->source[$this->pos] === "\n") {
Expand Down Expand Up @@ -3950,7 +3950,7 @@ private function collectTrailingEolTrivia(): array
$startColumn = $this->column;
$text = '';
while ($this->pos < $this->length && $this->source[$this->pos] !== "\n") {
$text .= $this->source[$this->pos];
$text .= $this->currentChar();
$this->advance();
}
$trailing[] = TriviaToken::comment($text, $startLine, $startColumn, $gap);
Expand Down
54 changes: 54 additions & 0 deletions test/unit/Document/CommentIndentTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
<?php

declare(strict_types=1);

namespace Horde\Yaml\Test\Unit\Document;

use Horde\Yaml\Document\Emitter\Emitter;
use Horde\Yaml\Document\Node\CommentNode;
use Horde\Yaml\Document\YamlStringLoader;
use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\TestCase;

/**
* A comment comes back at the column its author put it.
*
* The emitter already honoured `CommentNode::getIndent()`, and the scanner
* already captured each comment's column - but the parser passed a hardcoded 0,
* so every standalone comment was re-indented to its surrounding container.
*
* The indent is nullable rather than zero-defaulted because column 1 is a real
* position: a flush-left comment inside an indented block is a deliberate style
* that "0 means unset" could not express.
*/
final class CommentIndentTest extends TestCase
{
/** @return array<string, array{string}> */
public static function comments(): array
{
return [
'matching its sibling' => ["parent:\n # aligned\n key: 1\n"],
'deeper than its sibling'=> ["parent:\n child:\n # over-indented\n key: 1\n"],
'shallower' => ["parent:\n # four spaces\n key: 1\n"],
'flush left in a block' => ["parent:\n# flush left\n key: 1\n"],
'before a nested block' => ["a:\n b:\n # note\n c: 1\n"],
];
}

#[DataProvider('comments')]
public function testCommentsKeepTheirColumn(string $source): void
{
self::assertSame($source, (new Emitter())->emit((new YamlStringLoader())->load($source)));
}

public function testAProgrammaticCommentStillFollowsItsContainer(): void
{
// No column was ever recorded for it, so it indents with the block it
// is added to rather than landing at column 1.
$stream = (new YamlStringLoader())->load("parent:\n key: 1\n");
$parent = $stream->getDocument(0)->root()->entry('parent')->getValue();
$parent->appendChildInternal(new CommentNode('# added later'));

self::assertStringContainsString(" # added later", (new Emitter())->emit($stream));
}
}
53 changes: 53 additions & 0 deletions test/unit/Document/LineEndingTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
<?php

declare(strict_types=1);

namespace Horde\Yaml\Test\Unit\Document;

use Horde\Yaml\Document\Emitter\Emitter;
use Horde\Yaml\Document\YamlStringLoader;
use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\TestCase;

/**
* A CRLF document comes back as CRLF.
*
* The scanner normalises CRLF to LF so the rest of the parser sees one line
* ending, and the emitter already writes whatever the stream names - but
* nothing recorded what the source used, so every Windows-authored document was
* silently rewritten with LF.
*/
final class LineEndingTest extends TestCase
{
/** @return array<string, array{string}> */
public static function documents(): array
{
return [
'crlf' => ["# note\r\na:\r\n b: 1\r\n"],
'lf' => ["# note\na:\n b: 1\n"],
'crlf with indentation' => ["a:\r\n b:\r\n c: 1\r\n"],
'crlf with a comment column' => ["a:\r\n # deep\r\n b: 1\r\n"],
'crlf with a block scalar' => ["a: |\r\n line one\r\n line two\r\n"],
'crlf with a sequence' => ["a:\r\n - one\r\n - two\r\n"],
];
}

#[DataProvider('documents')]
public function testLineEndingsRoundTrip(string $source): void
{
self::assertSame($source, (new Emitter())->emit((new YamlStringLoader())->load($source)));
}

public function testTheStreamReportsTheEndingItRead(): void
{
self::assertSame("\r\n", (new YamlStringLoader())->load("a: 1\r\n")->getLineEnding());
self::assertSame("\n", (new YamlStringLoader())->load("a: 1\n")->getLineEnding());
}

public function testAMixedDocumentIsNotGuessedAt(): void
{
// No single ending to preserve. LF leaves it visibly changed rather
// than subtly, so a caller comparing input to output can still tell.
self::assertSame("\n", (new YamlStringLoader())->load("a:\r\n b: 1\n")->getLineEnding());
}
}
Loading
Loading