From 15d23ae421e2cdc644d8ddb5ed80e503e5016c65 Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Wed, 5 Sep 2018 20:54:58 -0400 Subject: [PATCH 1/3] Fixing a bug where we improperly tracked indentation in 2 different ways --- src/Util/YamlSourceManipulator.php | 19 ++++++++++++++++++- .../add_new_item_in_multiline_hash.test | 18 ++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) create mode 100644 tests/Util/yaml_fixtures/add_new_item_in_multiline_hash.test diff --git a/src/Util/YamlSourceManipulator.php b/src/Util/YamlSourceManipulator.php index 73db3f0e3..ffa78c961 100644 --- a/src/Util/YamlSourceManipulator.php +++ b/src/Util/YamlSourceManipulator.php @@ -783,6 +783,19 @@ private function advanceCurrentPosition(int $newPosition) return; } + /* + * A bit of a workaround. At times, this function will be called when the + * position is at the beginning of the line: so, one character *after* + * a line break. In that case, if there are a group of spaces at the + * beginning of this first line, they *should* be used to calculate the new + * indentation. To force this, if we detect this situation, we move one + * character backwards, so that the first line is considered a valid line + * to look for indentation. + */ + if ($this->isCharLineBreak(substr($this->contents, $originalPosition - 1, 1))) { + $originalPosition--; + } + // look for empty lines and track the current indentation $advancedContent = substr($this->contents, $originalPosition, $newPosition - $originalPosition); $previousIndentation = $this->indentationForDepths[$this->depth]; @@ -872,7 +885,11 @@ private function guessNextArrayTypeAndAdvance(): string // get the next char & advance immediately $nextCharacter = substr($this->contents, $this->currentPosition, 1); - $this->advanceCurrentPosition($this->currentPosition + 1); + // advance, but without advanceCurrentPosition() + // because we are either moving along one line until [ { + // or we are finding a line break and stopping: indentation + // should not be calculated + $this->currentPosition++; if ($this->isCharLineBreak($nextCharacter)) { return self::ARRAY_FORMAT_MULTILINE; diff --git a/tests/Util/yaml_fixtures/add_new_item_in_multiline_hash.test b/tests/Util/yaml_fixtures/add_new_item_in_multiline_hash.test new file mode 100644 index 000000000..d692bb827 --- /dev/null +++ b/tests/Util/yaml_fixtures/add_new_item_in_multiline_hash.test @@ -0,0 +1,18 @@ +security: + firewalls: + main: + anonymous: true + guard: + authenticators: + - App\Security\Test +=== +$data['security']['firewalls']['main']['guard']['authenticators'][] = 'App\Security\Test2'; +=== +security: + firewalls: + main: + anonymous: true + guard: + authenticators: + - App\Security\Test + - App\Security\Test2 \ No newline at end of file From 3582b1df03f90f5f48dcb58cd7259a71947a0905 Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Wed, 5 Sep 2018 20:56:21 -0400 Subject: [PATCH 2/3] improving some logging --- src/Util/YamlSourceManipulator.php | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/Util/YamlSourceManipulator.php b/src/Util/YamlSourceManipulator.php index ffa78c961..09174bc5f 100644 --- a/src/Util/YamlSourceManipulator.php +++ b/src/Util/YamlSourceManipulator.php @@ -814,14 +814,7 @@ private function advanceCurrentPosition(int $newPosition) } } - /* - if ($newIndentation === $previousIndentation) { - $this->log(sprintf('Indentation unchanged: %d', $newIndentation)); - } else { - $this->log(sprintf('Indentation changed to: %d', $newIndentation)); - } - */ - + $this->log(sprintf('Calculating new indentation: changing from %d to %d', $this->indentationForDepths[$this->depth], $newIndentation), true); $this->indentationForDepths[$this->depth] = $newIndentation; } @@ -850,12 +843,13 @@ private function log(string $message, $includeContent = false) 'key' => isset($this->currentPath[$this->depth]) ? $this->currentPath[$this->depth] : 'n/a', 'depth' => $this->depth, 'position' => $this->currentPosition, + 'indentation' => $this->indentationForDepths[$this->depth], ]; if ($includeContent) { $context['content'] = sprintf( - '%s...', - str_replace(["\r\n", "\n"], ['\r\n', '\n'], substr($this->contents, $this->currentPosition, 20)) + '>%s<', + str_replace(["\r\n", "\n"], ['\r\n', '\n'], substr($this->contents, $this->currentPosition, 50)) ); } From 7f4f77a1931a598ca2cae103eef750ca2afcfcef Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Wed, 5 Sep 2018 21:11:22 -0400 Subject: [PATCH 3/3] Fixing a bug where we didn't convert properly from a scalar value to array --- src/Util/YamlSourceManipulator.php | 46 ++++++++++++++----- .../Util/yaml_fixtures/add_array_to_null.test | 29 ++++++++++++ tests/Util/yaml_fixtures/add_key_to_null.test | 10 ++++ 3 files changed, 74 insertions(+), 11 deletions(-) create mode 100644 tests/Util/yaml_fixtures/add_array_to_null.test create mode 100644 tests/Util/yaml_fixtures/add_key_to_null.test diff --git a/src/Util/YamlSourceManipulator.php b/src/Util/YamlSourceManipulator.php index 09174bc5f..65dcae266 100644 --- a/src/Util/YamlSourceManipulator.php +++ b/src/Util/YamlSourceManipulator.php @@ -130,7 +130,7 @@ private function updateData(array $newData) $this->arrayTypeForDepths[$this->depth] = $this->isHash($currentData) ? self::ARRAY_TYPE_HASH : self::ARRAY_TYPE_SEQUENCE; $this->log(sprintf( - 'Array type: %s, format: %s ', + 'Changing array type & format via updateData()', $this->arrayTypeForDepths[$this->depth], $this->arrayFormatForDepths[$this->depth] )); @@ -333,12 +333,7 @@ private function addNewKeyToYaml($key, $value) // because we're inside a multi-line array, put this item // onto the *next* line & indent it - // But, if the *value* is an array, then ITS children will - // also need to be indented artificially by the same amount - $newYamlValue = str_replace("\n", "\n".$this->getCurrentIndentation(), $newYamlValue); - - // now add the new line & indentation to the top-level - $newYamlValue = "\n".$this->getCurrentIndentation().$newYamlValue; + $newYamlValue = "\n".$this->indentMultilineYamlArray($newYamlValue); } else { if ($firstItemInArray) { // avoid the starting "," if first item in array @@ -454,13 +449,23 @@ private function changeValueInYaml($value) $endValuePosition = $this->findEndPositionOfValue($originalVal); - // empty space between key & value - $newDataString = ' '.$this->convertToYaml($value); + $newYamlValue = $this->convertToYaml($value); + if (!is_array($originalVal) && is_array($value)) { + // we're converting from a scalar to a (multiline) array + // this means we need to break onto the next line + + // increase the indentation + $this->manuallyIncrementIndentation(); + $newYamlValue = "\n".$this->indentMultilineYamlArray($newYamlValue); + } else { + // empty space between key & value + $newYamlValue = ' '.$newYamlValue; + } $newContents = substr($this->contents, 0, $this->currentPosition) - .$newDataString + .$newYamlValue .substr($this->contents, $endValuePosition); - $newPosition = $this->currentPosition + \strlen($newDataString); + $newPosition = $this->currentPosition + \strlen($newYamlValue); $newData = $this->currentData; $newData = $this->setValueAtCurrentPath($value, $newData); @@ -844,6 +849,8 @@ private function log(string $message, $includeContent = false) 'depth' => $this->depth, 'position' => $this->currentPosition, 'indentation' => $this->indentationForDepths[$this->depth], + 'type' => $this->arrayTypeForDepths[$this->depth], + 'format' => $this->arrayFormatForDepths[$this->depth], ]; if ($includeContent) { @@ -1123,4 +1130,21 @@ private function isCharLineBreak(string $char): bool { return "\n" === $char || "\r" === $char; } + + /** + * Takes an unindented multi-line YAML string and indents it so + * it can be inserted into the current position. + * + * Usually an empty line needs to be prepended to this result before + * adding to the content. + */ + private function indentMultilineYamlArray(string $yaml): string + { + // But, if the *value* is an array, then ITS children will + // also need to be indented artificially by the same amount + $yaml = str_replace("\n", "\n".$this->getCurrentIndentation(), $yaml); + + // now indent this level + return $this->getCurrentIndentation().$yaml; + } } diff --git a/tests/Util/yaml_fixtures/add_array_to_null.test b/tests/Util/yaml_fixtures/add_array_to_null.test new file mode 100644 index 000000000..f9be5fdb0 --- /dev/null +++ b/tests/Util/yaml_fixtures/add_array_to_null.test @@ -0,0 +1,29 @@ +security: + firewalls: + main: ~ + other_firewall: + security: false +=== +$data['security']['firewalls']['main'] = [ + 'anonymous' => true, + 'guard' => [ + 'authenticators' => [ + 'some_key' => true, + 'great_colors' => ['green', 'pink', 'orange'], + ], + ], +]; +=== +security: + firewalls: + main: + anonymous: true + guard: + authenticators: + some_key: true + great_colors: + - green + - pink + - orange + other_firewall: + security: false \ No newline at end of file diff --git a/tests/Util/yaml_fixtures/add_key_to_null.test b/tests/Util/yaml_fixtures/add_key_to_null.test new file mode 100644 index 000000000..24b3c8104 --- /dev/null +++ b/tests/Util/yaml_fixtures/add_key_to_null.test @@ -0,0 +1,10 @@ +security: + firewalls: + main: ~ +=== +$data['security']['firewalls']['main'] = ['anonymous' => true]; +=== +security: + firewalls: + main: + anonymous: true \ No newline at end of file