bug symfony#19028 [Yaml] properly count skipped comment lines (xabbuh)

This PR was merged into the 2.7 branch.

Discussion
----------

[Yaml] properly count skipped comment lines

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony#15437, symfony#17733, symfony#19023
| License       | MIT
| Doc PR        |

Commits
-------

da7fc36 [Yaml] properly count skipped comment lines

Author: fabpot

src/Symfony/Component/Yaml/Parser.php

@@ -30,17 +30,21 @@ class Parser
     private $currentLineNb = -1;
     private $currentLine = '';
     private $refs = array();
+    private $skippedLineNumbers = array();
+    private $locallySkippedLineNumbers = array();
 
     /**
      * Constructor.
      *
      * @param int      $offset             The offset of YAML document (used for line numbers in error messages)
      * @param int|null $totalNumberOfLines The overall number of lines being parsed
+     * @param int[]    $skippedLineNumbers Number of comment lines that have been skipped by the parser
      */
-    public function __construct($offset = 0, $totalNumberOfLines = null)
+    public function __construct($offset = 0, $totalNumberOfLines = null, array $skippedLineNumbers = array())
     {
         $this->offset = $offset;
         $this->totalNumberOfLines = $totalNumberOfLines;
+        $this->skippedLineNumbers = $skippedLineNumbers;
     }
 
     /**
@@ -101,25 +105,18 @@ public function parse($value, $exceptionOnInvalidType = false, $objectSupport =
 
                 // array
                 if (!isset($values['value']) || '' == trim($values['value'], ' ') || 0 === strpos(ltrim($values['value'], ' '), '#')) {
-                    $c = $this->getRealCurrentLineNb() + 1;
-                    $parser = new self($c, $this->totalNumberOfLines);
-                    $parser->refs = &$this->refs;
-                    $data[] = $parser->parse($this->getNextEmbedBlock(null, true), $exceptionOnInvalidType, $objectSupport, $objectForMap);
+                    $data[] = $this->parseBlock($this->getRealCurrentLineNb() + 1, $this->getNextEmbedBlock(null, true), $exceptionOnInvalidType, $objectSupport, $objectForMap);
                 } else {
                     if (isset($values['leadspaces'])
                         && preg_match('#^(?P<key>'.Inline::REGEX_QUOTED_STRING.'|[^ \'"\{\[].*?) *\:(\s+(?P<value>.+?))?\s*$#u', $values['value'], $matches)
                     ) {
                         // this is a compact notation element, add to next block and parse
-                        $c = $this->getRealCurrentLineNb();
-                        $parser = new self($c, $this->totalNumberOfLines);
-                        $parser->refs = &$this->refs;
-
                         $block = $values['value'];
                         if ($this->isNextLineIndented()) {
                             $block .= "\n".$this->getNextEmbedBlock($this->getCurrentLineIndentation() + strlen($values['leadspaces']) + 1);
                         }
 
-                        $data[] = $parser->parse($block, $exceptionOnInvalidType, $objectSupport, $objectForMap);
+                        $data[] = $this->parseBlock($this->getRealCurrentLineNb(), $block, $exceptionOnInvalidType, $objectSupport, $objectForMap);
                     } else {
                         $data[] = $this->parseValue($values['value'], $exceptionOnInvalidType, $objectSupport, $objectForMap);
                     }
@@ -175,10 +172,7 @@ public function parse($value, $exceptionOnInvalidType = false, $objectSupport =
                         } else {
                             $value = $this->getNextEmbedBlock();
                         }
-                        $c = $this->getRealCurrentLineNb() + 1;
-                        $parser = new self($c, $this->totalNumberOfLines);
-                        $parser->refs = &$this->refs;
-                        $parsed = $parser->parse($value, $exceptionOnInvalidType, $objectSupport, $objectForMap);
+                        $parsed = $this->parseBlock($this->getRealCurrentLineNb() + 1, $value, $exceptionOnInvalidType, $objectSupport, $objectForMap);
 
                         if (!is_array($parsed)) {
                             throw new ParseException('YAML merge keys used with a scalar value instead of an array.', $this->getRealCurrentLineNb() + 1, $this->currentLine);
@@ -226,10 +220,7 @@ public function parse($value, $exceptionOnInvalidType = false, $objectSupport =
                             $data[$key] = null;
                         }
                     } else {
-                        $c = $this->getRealCurrentLineNb() + 1;
-                        $parser = new self($c, $this->totalNumberOfLines);
-                        $parser->refs = &$this->refs;
-                        $value = $parser->parse($this->getNextEmbedBlock(), $exceptionOnInvalidType, $objectSupport, $objectForMap);
+                        $value = $this->parseBlock($this->getRealCurrentLineNb() + 1, $this->getNextEmbedBlock(), $exceptionOnInvalidType, $objectSupport, $objectForMap);
                         // Spec: Keys MUST be unique; first one wins.
                         // But overwriting is allowed when a merge node is used in current block.
                         if ($allowOverwrite || !isset($data[$key])) {
@@ -317,14 +308,42 @@ public function parse($value, $exceptionOnInvalidType = false, $objectSupport =
         return empty($data) ? null : $data;
     }
 
+    private function parseBlock($offset, $yaml, $exceptionOnInvalidType, $objectSupport, $objectForMap)
+    {
+        $skippedLineNumbers = $this->skippedLineNumbers;
+
+        foreach ($this->locallySkippedLineNumbers as $lineNumber) {
+            if ($lineNumber < $offset) {
+                continue;
+            }
+
+            $skippedLineNumbers[] = $lineNumber;
+        }
+
+        $parser = new self($offset, $this->totalNumberOfLines, $skippedLineNumbers);
+        $parser->refs = &$this->refs;
+
+        return $parser->parse($yaml, $exceptionOnInvalidType, $objectSupport, $objectForMap);
+    }
+
     /**
      * Returns the current line number (takes the offset into account).
      *
      * @return int The current line number
      */
     private function getRealCurrentLineNb()
     {
-        return $this->currentLineNb + $this->offset;
+        $realCurrentLineNumber = $this->currentLineNb + $this->offset;
+
+        foreach ($this->skippedLineNumbers as $skippedLineNumber) {
+            if ($skippedLineNumber > $realCurrentLineNumber) {
+                break;
+            }
+
+            ++$realCurrentLineNumber;
+        }
+
+        return $realCurrentLineNumber;
     }
 
     /**
@@ -426,7 +445,15 @@ private function getNextEmbedBlock($indentation = null, $inSequence = false)
             }
 
             // we ignore "comment" lines only when we are not inside a scalar block
-            if (empty($blockScalarIndentations) && $this->isCurrentLineComment() && false === $this->checkIfPreviousNonCommentLineIsCollectionItem()) {
+            if (empty($blockScalarIndentations) && $this->isCurrentLineComment()) {
+                // remember ignored comment lines (they are used later in nested
+                // parser calls to determine real line numbers)
+                //
+                // CAUTION: beware to not populate the global property here as it
+                // will otherwise influence the getRealCurrentLineNb() call here
+                // for consecutive comment lines and subsequent embedded blocks
+                $this->locallySkippedLineNumbers[] = $this->getRealCurrentLineNb();
+
                 continue;
             }
 
@@ -786,44 +813,4 @@ private function isBlockScalarHeader()
     {
         return (bool) preg_match('~'.self::BLOCK_SCALAR_HEADER_PATTERN.'$~', $this->currentLine);
     }
-
-    /**
-     * Returns true if the current line is a collection item.
-     *
-     * @return bool
-     */
-    private function isCurrentLineCollectionItem()
-    {
-        $ltrimmedLine = ltrim($this->currentLine, ' ');
-
-        return '' !== $ltrimmedLine && '-' === $ltrimmedLine[0];
-    }
-
-    /**
-     * Tests whether the current comment line is in a collection.
-     *
-     * @return bool
-     */
-    private function checkIfPreviousNonCommentLineIsCollectionItem()
-    {
-        $isCollectionItem = false;
-        $moves = 0;
-        while ($this->moveToPreviousLine()) {
-            ++$moves;
-            // If previous line is a comment, move back again.
-            if ($this->isCurrentLineComment()) {
-                continue;
-            }
-            $isCollectionItem = $this->isCurrentLineCollectionItem();
-            break;
-        }
-
-        // Move parser back to previous line.
-        while ($moves > 0) {
-            $this->moveToNextLine();
-            --$moves;
-        }
-
-        return $isCollectionItem;
-    }
 }

src/Symfony/Component/Yaml/Tests/ParserTest.php

@@ -607,6 +607,25 @@ public function testSequenceInMappingStartedBySingleDashLine()
         $this->assertSame($expected, $this->parser->parse($yaml));
     }
 
+    public function testSequenceFollowedByCommentEmbeddedInMapping()
+    {
+        $yaml = <<<EOT
+a:
+    b:
+        - c
+# comment
+    d: e
+EOT;
+        $expected = array(
+            'a' => array(
+                'b' => array('c'),
+                'd' => 'e',
+            ),
+        );
+
+        $this->assertSame($expected, $this->parser->parse($yaml));
+    }
+
     /**
      * @expectedException \Symfony\Component\Yaml\Exception\ParseException
      */