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.

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

[Yaml] parse merge keys with PARSE_OBJECT_FOR_MAP flag #24189

Merged
merged 1 commit into from Sep 28, 2017

Conversation

Projects
None yet
6 participants
@xabbuh
Member

xabbuh commented Sep 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ostrolucky

ostrolucky Sep 15, 2017

Contributor

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);
                         }
Contributor

ostrolucky commented Sep 15, 2017

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

This comment has been minimized.

Show comment
Hide comment
@xabbuh
Member

xabbuh commented Sep 28, 2017

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Sep 28, 2017

Member

@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);
Member

chalasr commented Sep 28, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Sep 28, 2017

Member

@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.

Member

xabbuh commented Sep 28, 2017

@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.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Sep 28, 2017

Member

Thank you @xabbuh.

Member

fabpot commented Sep 28, 2017

Thank you @xabbuh.

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Sep 28, 2017

bug #24189 [Yaml] parse merge keys with PARSE_OBJECT_FOR_MAP flag (xa…
…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 xabbuh deleted the xabbuh:issue-24133 branch Sep 28, 2017

@fabpot fabpot referenced this pull request Oct 5, 2017

Merged

Release v3.3.10 #24452

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment