[Yaml] parse merge keys with PARSE_OBJECT_FOR_MAP flag #24189

xabbuh · 2017-09-13T18:08:54Z

Q	A
Branch?	3.3
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#24133
License	MIT
Doc PR

ostrolucky · 2017-09-15T21:38:25Z

You use same kind of foreach loops I removed via #21756 Please consider using union operator again. It reduces big chunk of unnecessary verbosity and you get NULL handling for free. Example of usage in your patch:

@@ -257,14 +257,8 @@
 
                         $refValue = $this->refs[$refName];
 
-                        if (is_array($refValue)) {
-                            $data += $refValue; // array union
-                        } elseif ((bool) (Yaml::PARSE_OBJECT_FOR_MAP & $flags) && $refValue instanceof \stdClass) {
-                            foreach ($refValue as $refValueKey => $refValueValue) {
-                                if (!isset($data[$refValueKey])) {
-                                    $data[$refValueKey] = $refValueValue;
-                                }
-                            }
+                        if (is_array($refValue) || (bool) (Yaml::PARSE_OBJECT_FOR_MAP & $flags) && $refValue instanceof \stdClass) {
+                            $data += (array) $refValue; // array union
                         } else {
                             throw new ParseException('YAML merge keys used with a scalar value instead of an array.', $this->getRealCurrentLineNb() + 1, $this->currentLine);
                         }
@@ -294,11 +288,7 @@
                                 $data += $parsed; // array union
                             }
                         } elseif ((bool) (Yaml::PARSE_OBJECT_FOR_MAP & $flags) && $parsed instanceof \stdClass) {
-                            foreach ($parsed as $parsedKey => $parsedValue) {
-                                if (!isset($data[$parsedKey])) {
-                                    $data[$parsedKey] = $parsedValue;
-                                }
-                            }
+                            $data += (array) $parsed;
                         } else {
                             throw new ParseException('YAML merge keys used with a scalar value instead of an array.', $this->getRealCurrentLineNb() + 1, $this->currentLine);
                         }

xabbuh · 2017-09-28T11:01:08Z

ping @symfony/deciders :)

chalasr · 2017-09-28T11:21:05Z

@xabbuh Just tried it out, it seems to fix it for #24133 code but not for #23587.
Reproducer:

<?php
require __DIR__ .'/vendor/autoload.php';

use Symfony\Component\Yaml\Parser;
use Symfony\Component\Yaml\Yaml;

$yaml = <<<YAML
root:
    mergekeyrefdef:
        a: foo
        <<: &quux
            b: bar
            c: baz
    mergekeyderef:
        d: quux
        <<: *quux
YAML;

$arr = (new Parser())->parse($yaml, Yaml::PARSE_OBJECT_FOR_MAP);

xabbuh · 2017-09-28T12:30:10Z

@chalasr Thanks for checking. You are absolutely right. And the bug described in #23587 is actually not related to the PARSE_OBJECT_FOR_MAP flag, but purely to the fact that the parser fails when a reference is defined on a merge key. We need to fix that in a different PR on the 2.7 branch.

stof · 2017-09-28T12:35:02Z

src/Symfony/Component/Yaml/Parser.php

@@ -249,14 +249,18 @@ private function doParse($value, $flags)
                if ('<<' === $key) {
                    $mergeNode = true;
                    $allowOverwrite = true;
-                    if (isset($values['value']) && 0 === strpos($values['value'], '*')) {
+                    if (isset($values['value']) && isset($values['value'][0]) && '*' === $values['value'][0]) {


the first isset can be removed, as it is covered by the second one.

stof · 2017-09-28T12:35:48Z

src/Symfony/Component/Yaml/Parser.php

                        $refName = substr(rtrim($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 ((bool) (Yaml::PARSE_OBJECT_FOR_MAP & $flags) && $refValue instanceof \stdClass) {


the explicit bool cast is useless (&& will trigger it too)

fabpot · 2017-09-28T23:05:35Z

Thank you @xabbuh.

…bbuh) This PR was merged into the 3.3 branch. Discussion ---------- [Yaml] parse merge keys with PARSE_OBJECT_FOR_MAP flag | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24133 | License | MIT | Doc PR | Commits ------- 534eaed parse merge keys with PARSE_OBJECT_FOR_MAP flag

xabbuh added this to the 3.3 milestone Sep 13, 2017

carsonbot added Status: Needs Review Yaml Bug labels Sep 13, 2017

This was referenced Sep 13, 2017

[YAML] YAML merge does not work with PARSE_OBJECT_FOR_MAP #24133

Closed

[YAML] Defining a reference on a merge key does not work. #23587

Closed

xabbuh force-pushed the issue-24133 branch from 414645c to ba255ed Compare September 13, 2017 19:09

xabbuh force-pushed the issue-24133 branch from ba255ed to 464aaf3 Compare September 18, 2017 14:38

xabbuh force-pushed the issue-24133 branch from 464aaf3 to b18845a Compare September 28, 2017 10:55

stof reviewed Sep 28, 2017

View reviewed changes

parse merge keys with PARSE_OBJECT_FOR_MAP flag

534eaed

xabbuh force-pushed the issue-24133 branch from b18845a to 534eaed Compare September 28, 2017 12:42

carsonbot removed the Status: Needs Review label Sep 28, 2017

chalasr approved these changes Sep 28, 2017

View reviewed changes

carsonbot added the Status: Reviewed label Sep 28, 2017

fabpot merged commit 534eaed into symfony:3.3 Sep 28, 2017

xabbuh deleted the issue-24133 branch September 28, 2017 23:46

fabpot mentioned this pull request Oct 5, 2017

Release v3.3.10 #24452

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Yaml] parse merge keys with PARSE_OBJECT_FOR_MAP flag #24189

[Yaml] parse merge keys with PARSE_OBJECT_FOR_MAP flag #24189

xabbuh commented Sep 13, 2017 •

edited

Loading

ostrolucky commented Sep 15, 2017

xabbuh commented Sep 28, 2017

chalasr commented Sep 28, 2017

xabbuh commented Sep 28, 2017

stof Sep 28, 2017

stof Sep 28, 2017

fabpot commented Sep 28, 2017

[Yaml] parse merge keys with PARSE_OBJECT_FOR_MAP flag #24189

[Yaml] parse merge keys with PARSE_OBJECT_FOR_MAP flag #24189

Conversation

xabbuh commented Sep 13, 2017 • edited Loading

ostrolucky commented Sep 15, 2017

xabbuh commented Sep 28, 2017

chalasr commented Sep 28, 2017

xabbuh commented Sep 28, 2017

stof Sep 28, 2017

Choose a reason for hiding this comment

stof Sep 28, 2017

Choose a reason for hiding this comment

fabpot commented Sep 28, 2017

xabbuh commented Sep 13, 2017 •

edited

Loading