From 02614e064f3d4165f09d7713b2632fb4d5a219f0 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Wed, 18 Jun 2014 16:18:19 +0200 Subject: [PATCH 1/3] [Yaml] refactoring of merges for performance --- src/Symfony/Component/Yaml/Parser.php | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/Symfony/Component/Yaml/Parser.php b/src/Symfony/Component/Yaml/Parser.php index 9539e36aa3..543c3d0850 100644 --- a/src/Symfony/Component/Yaml/Parser.php +++ b/src/Symfony/Component/Yaml/Parser.php @@ -148,23 +148,23 @@ class Parser $parser->refs =& $this->refs; $parsed = $parser->parse($value, $exceptionOnInvalidType, $objectSupport); - $merged = array(); if (!is_array($parsed)) { throw new ParseException('YAML merge keys used with a scalar value instead of an array.', $this->getRealCurrentLineNb() + 1, $this->currentLine); - } elseif (isset($parsed[0])) { + } + + $isProcessed = true; + if (isset($parsed[0])) { // Numeric array, merge individual elements - foreach (array_reverse($parsed) as $parsedItem) { + foreach ($parsed as $parsedItem) { if (!is_array($parsedItem)) { throw new ParseException('Merge items must be arrays.', $this->getRealCurrentLineNb() + 1, $parsedItem); } - $merged = array_merge($parsedItem, $merged); + $data = array_merge($data, $parsedItem); } } else { - // Associative array, merge - $merged = array_merge($merged, $parsed); + // Associative array + $data = $parsed; } - - $isProcessed = $merged; } } elseif (isset($values['value']) && preg_match('#^&(?P[^ ]+) *(?P.*)#u', $values['value'], $matches)) { $isRef = $matches['ref']; @@ -173,9 +173,8 @@ class Parser if ($isProcessed) { // Merge keys - $data = $isProcessed; - // hash } elseif (!isset($values['value']) || '' == trim($values['value'], ' ') || 0 === strpos(ltrim($values['value'], ' '), '#')) { + // hash // if next line is less indented or equal, then it means that the current value is null if (!$this->isNextLineIndented() && !$this->isNextLineUnIndentedCollection()) { // Spec: Keys MUST be unique; first one wins. From 8c621ab4b57f32c69c3b1ca92e9c4c4ee2226a74 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Wed, 18 Jun 2014 17:42:59 +0200 Subject: [PATCH 2/3] [Yaml] fix priority of sequence merges according to spec fixes symfony/symfony#11154 --- src/Symfony/Component/Yaml/Parser.php | 60 ++++++++++++------- .../Yaml/Tests/Fixtures/sfMergeKey.yml | 4 +- 2 files changed, 43 insertions(+), 21 deletions(-) diff --git a/src/Symfony/Component/Yaml/Parser.php b/src/Symfony/Component/Yaml/Parser.php index 543c3d0850..05c2db67cf 100644 --- a/src/Symfony/Component/Yaml/Parser.php +++ b/src/Symfony/Component/Yaml/Parser.php @@ -76,7 +76,7 @@ class Parser throw new ParseException('A YAML file cannot contain tabs as indentation.', $this->getRealCurrentLineNb() + 1, $this->currentLine); } - $isRef = $isInPlace = $isProcessed = false; + $isRef = $mergeNode = false; if (preg_match('#^\-((?P\s+)(?P.+?))?\s*$#u', $this->currentLine, $values)) { if ($context && 'mapping' == $context) { throw new ParseException('You cannot define a sequence item when in a mapping'); @@ -132,10 +132,23 @@ class Parser } if ('<<' === $key) { + $mergeNode = true; if (isset($values['value']) && 0 === strpos($values['value'], '*')) { - $isInPlace = substr($values['value'], 1); - if (!array_key_exists($isInPlace, $this->refs)) { - throw new ParseException(sprintf('Reference "%s" does not exist.', $isInPlace), $this->getRealCurrentLineNb() + 1, $this->currentLine); + $refName = substr($values['value'], 1); + if (!array_key_exists($refName, $this->refs)) { + throw new ParseException(sprintf('Reference "%s" does not exist.', $refName), $this->getRealCurrentLineNb() + 1, $this->currentLine); + } + + $refValue = $this->refs[$refName]; + + if (!is_array($refValue)) { + throw new ParseException('YAML merge keys used with a scalar value instead of an array.', $this->getRealCurrentLineNb() + 1, $this->currentLine); + } + + foreach ($refValue as $key => $value) { + if (!isset($data[$key])) { + $data[$key] = $value; + } } } else { if (isset($values['value']) && $values['value'] !== '') { @@ -152,18 +165,29 @@ class Parser throw new ParseException('YAML merge keys used with a scalar value instead of an array.', $this->getRealCurrentLineNb() + 1, $this->currentLine); } - $isProcessed = true; if (isset($parsed[0])) { - // Numeric array, merge individual elements + // If the value associated with the merge key is a sequence, then this sequence is expected to contain mapping nodes + // and each of these nodes is merged in turn according to its order in the sequence. Keys in mapping nodes earlier + // in the sequence override keys specified in later mapping nodes. foreach ($parsed as $parsedItem) { if (!is_array($parsedItem)) { throw new ParseException('Merge items must be arrays.', $this->getRealCurrentLineNb() + 1, $parsedItem); } - $data = array_merge($data, $parsedItem); + + foreach ($parsedItem as $key => $value) { + if (!isset($data[$key])) { + $data[$key] = $value; + } + } } } else { - // Associative array - $data = $parsed; + // If the value associated with the key is a single mapping node, each of its key/value pairs is inserted into the + // current mapping, unless the key already exists in it. + foreach ($parsed as $key => $value) { + if (!isset($data[$key])) { + $data[$key] = $value; + } + } } } } elseif (isset($values['value']) && preg_match('#^&(?P[^ ]+) *(?P.*)#u', $values['value'], $matches)) { @@ -171,7 +195,7 @@ class Parser $values['value'] = $matches['value']; } - if ($isProcessed) { + if ($mergeNode) { // Merge keys } elseif (!isset($values['value']) || '' == trim($values['value'], ' ') || 0 === strpos(ltrim($values['value'], ' '), '#')) { // hash @@ -196,16 +220,12 @@ class Parser } } } else { - if ($isInPlace) { - $data = $this->refs[$isInPlace]; - } else { - $value = $this->parseValue($values['value'], $exceptionOnInvalidType, $objectSupport);; - // Spec: Keys MUST be unique; first one wins. - // Parser cannot abort this mapping earlier, since lines - // are processed sequentially. - if (!isset($data[$key])) { - $data[$key] = $value; - } + $value = $this->parseValue($values['value'], $exceptionOnInvalidType, $objectSupport); + // Spec: Keys MUST be unique; first one wins. + // Parser cannot abort this mapping earlier, since lines + // are processed sequentially. + if (!isset($data[$key])) { + $data[$key] = $value; } } } else { diff --git a/src/Symfony/Component/Yaml/Tests/Fixtures/sfMergeKey.yml b/src/Symfony/Component/Yaml/Tests/Fixtures/sfMergeKey.yml index 3eec4f877d..c04943eec1 100644 --- a/src/Symfony/Component/Yaml/Tests/Fixtures/sfMergeKey.yml +++ b/src/Symfony/Component/Yaml/Tests/Fixtures/sfMergeKey.yml @@ -11,6 +11,8 @@ yaml: | b: Clark c: Brian bar: &bar + a: before + d: other <<: *foo x: Oren foo2: &foo2 @@ -24,4 +26,4 @@ yaml: | head: <<: [ *foo , *dong , *foo2 ] php: | - array('foo' => array('a' => 'Steve', 'b' => 'Clark', 'c' => 'Brian'), 'bar' => array('a' => 'Steve', 'b' => 'Clark', 'c' => 'Brian', 'x' => 'Oren'), 'foo2' => array('a' => 'Ballmer'), 'ding' => array('fi', 'fei', 'fo', 'fam'), 'check' => array('a' => 'Steve', 'b' => 'Clark', 'c' => 'Brian', 'fi', 'fei', 'fo', 'fam', 'isit' => 'tested'), 'head' => array('a' => 'Ballmer', 'b' => 'Clark', 'c' => 'Brian', 'fi', 'fei', 'fo', 'fam')) + array('foo' => array('a' => 'Steve', 'b' => 'Clark', 'c' => 'Brian'), 'bar' => array('a' => 'before', 'd' => 'other', 'b' => 'Clark', 'c' => 'Brian', 'x' => 'Oren'), 'foo2' => array('a' => 'Ballmer'), 'ding' => array('fi', 'fei', 'fo', 'fam'), 'check' => array('a' => 'Steve', 'b' => 'Clark', 'c' => 'Brian', 'fi', 'fei', 'fo', 'fam', 'isit' => 'tested'), 'head' => array('a' => 'Steve', 'b' => 'Clark', 'c' => 'Brian', 'fi', 'fei', 'fo', 'fam')) From dee15623ae4a008b10027ab87803f3136c41ebbd Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Wed, 18 Jun 2014 19:47:01 +0200 Subject: [PATCH 3/3] [Yaml] fix overwriting of keys after merged map fixes symfony/symfony#11142 --- src/Symfony/Component/Yaml/Parser.php | 17 ++++++++-------- .../Yaml/Tests/Fixtures/sfMergeKey.yml | 20 +++++++++++++++++-- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/Symfony/Component/Yaml/Parser.php b/src/Symfony/Component/Yaml/Parser.php index 05c2db67cf..0577b5e885 100644 --- a/src/Symfony/Component/Yaml/Parser.php +++ b/src/Symfony/Component/Yaml/Parser.php @@ -66,6 +66,7 @@ class Parser $data = array(); $context = null; + $allowOverwrite = false; while ($this->moveToNextLine()) { if ($this->isCurrentLineEmpty()) { continue; @@ -133,6 +134,7 @@ class Parser if ('<<' === $key) { $mergeNode = true; + $allowOverwrite = true; if (isset($values['value']) && 0 === strpos($values['value'], '*')) { $refName = substr($values['value'], 1); if (!array_key_exists($refName, $this->refs)) { @@ -202,9 +204,8 @@ class Parser // if next line is less indented or equal, then it means that the current value is null if (!$this->isNextLineIndented() && !$this->isNextLineUnIndentedCollection()) { // Spec: Keys MUST be unique; first one wins. - // Parser cannot abort this mapping earlier, since lines - // are processed sequentially. - if (!isset($data[$key])) { + // But overwriting is allowed when a merge node is used in current block. + if ($allowOverwrite || !isset($data[$key])) { $data[$key] = null; } } else { @@ -213,18 +214,16 @@ class Parser $parser->refs =& $this->refs; $value = $parser->parse($this->getNextEmbedBlock(), $exceptionOnInvalidType, $objectSupport); // Spec: Keys MUST be unique; first one wins. - // Parser cannot abort this mapping earlier, since lines - // are processed sequentially. - if (!isset($data[$key])) { + // But overwriting is allowed when a merge node is used in current block. + if ($allowOverwrite || !isset($data[$key])) { $data[$key] = $value; } } } else { $value = $this->parseValue($values['value'], $exceptionOnInvalidType, $objectSupport); // Spec: Keys MUST be unique; first one wins. - // Parser cannot abort this mapping earlier, since lines - // are processed sequentially. - if (!isset($data[$key])) { + // But overwriting is allowed when a merge node is used in current block. + if ($allowOverwrite || !isset($data[$key])) { $data[$key] = $value; } } diff --git a/src/Symfony/Component/Yaml/Tests/Fixtures/sfMergeKey.yml b/src/Symfony/Component/Yaml/Tests/Fixtures/sfMergeKey.yml index c04943eec1..fd9910174d 100644 --- a/src/Symfony/Component/Yaml/Tests/Fixtures/sfMergeKey.yml +++ b/src/Symfony/Component/Yaml/Tests/Fixtures/sfMergeKey.yml @@ -10,11 +10,19 @@ yaml: | a: Steve b: Clark c: Brian - bar: &bar + bar: a: before d: other <<: *foo + b: new x: Oren + c: + foo: bar + foo: ignore + bar: foo + duplicate: + foo: bar + foo: ignore foo2: &foo2 a: Ballmer ding: &dong [ fi, fei, fo, fam] @@ -26,4 +34,12 @@ yaml: | head: <<: [ *foo , *dong , *foo2 ] php: | - array('foo' => array('a' => 'Steve', 'b' => 'Clark', 'c' => 'Brian'), 'bar' => array('a' => 'before', 'd' => 'other', 'b' => 'Clark', 'c' => 'Brian', 'x' => 'Oren'), 'foo2' => array('a' => 'Ballmer'), 'ding' => array('fi', 'fei', 'fo', 'fam'), 'check' => array('a' => 'Steve', 'b' => 'Clark', 'c' => 'Brian', 'fi', 'fei', 'fo', 'fam', 'isit' => 'tested'), 'head' => array('a' => 'Steve', 'b' => 'Clark', 'c' => 'Brian', 'fi', 'fei', 'fo', 'fam')) + array( + 'foo' => array('a' => 'Steve', 'b' => 'Clark', 'c' => 'Brian'), + 'bar' => array('a' => 'before', 'd' => 'other', 'b' => 'new', 'c' => array('foo' => 'bar', 'bar' => 'foo'), 'x' => 'Oren'), + 'duplicate' => array('foo' => 'bar'), + 'foo2' => array('a' => 'Ballmer'), + 'ding' => array('fi', 'fei', 'fo', 'fam'), + 'check' => array('a' => 'Steve', 'b' => 'Clark', 'c' => 'Brian', 'fi', 'fei', 'fo', 'fam', 'isit' => 'tested'), + 'head' => array('a' => 'Steve', 'b' => 'Clark', 'c' => 'Brian', 'fi', 'fei', 'fo', 'fam') + )