[Yaml] support parsing files #24253

xabbuh · 2017-09-19T09:10:39Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#24191 (comment)
License	MIT
Doc PR	TODO

This PR adds a new flag PARSE_FILE which can be passed to the YAML parser. When given, the input will be interpreted as a filename whose contents will then be parsed. We already supported passing filenames in the past. Back then the features was not deterministic as it just relied on the string being an existing file or not. Now that we are able to control the behaviour of the parser by passing flags this can be done in a much cleaner way and allows to properly deal with errors (i.e. non existent or unreadable files).

This change will also allow to improve error/deprecation messages to include the filename being parsed and thus showing more descriptive error messages when people are using the YAML parser with a bunch of files (e.g. as part of the DependencyInjection or Routing component).

nicolas-grekas

(improving deprecation message planned in a latter PR, isn't it?)

nicolas-grekas · 2017-09-19T09:14:30Z

src/Symfony/Component/Yaml/Parser.php

@@ -92,6 +92,18 @@ public function parse($value, $flags = 0)
            @trigger_error('Using the Yaml::PARSE_KEYS_AS_STRINGS flag is deprecated since version 3.4 as it will be removed in 4.0. Quote your keys when they are evaluable instead.', E_USER_DEPRECATED);
        }

+        if ((bool) (Yaml::PARSE_FILE & $flags)) {


if (Yaml::PARSE_FILE & $flags) {

nicolas-grekas · 2017-09-19T09:15:43Z

src/Symfony/Component/Yaml/Yaml.php

@@ -33,6 +33,7 @@ class Yaml
    const PARSE_CONSTANT = 256;
    const PARSE_CUSTOM_TAGS = 512;
    const DUMP_EMPTY_ARRAY_AS_SEQUENCE = 1024;
+    const PARSE_FILE = 4096;


nope, we have a deprecated flag below that uses 2048 :)

stof · 2017-09-19T09:42:52Z

src/Symfony/Component/Yaml/Parser.php

+                throw new ParseException(sprintf('File "%s" cannot be read.', $value));
+            }
+
+            $value = file_get_contents($value);


In this case, we should ensure that ParseException have the file set in them when throwing them (either through the constructor or through setParsedFile). Otherwise, we don't benefit from the better error reporting.

jvasseur · 2017-09-19T12:35:59Z

This feels strange to use a flag to switch from filename to yaml content based on an flag.

Why not creating a parseFile method instead ?

ro0NL · 2017-09-20T08:56:50Z

@jvasseur i think we prefer a single API, e.g. parse() + flags.

jvasseur · 2017-09-20T09:23:19Z

@ro0NL but this flag is fundamentally different from the others, the others modify the parsing behavior, this one change what the method expect as an argument.

Let's look at the YamlEncoder for example, it allow passing flags in the serialization context (https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Serializer/Encoder/YamlEncoder.php#L62) which is a good feature to allow controlling how the yaml is decoded. It works well for all the current flags, but it will break (or even worse create a security breach) if someone pass the PARSE_FILE flag. Yes it would be a developer error to pass this flag, but it's a clue that this flag is wrong and this should go in a separate method.

stof · 2017-09-20T10:04:52Z

I tend to agree with @jvasseur here. Having a flag in the bitmask changing totally the meaning of the argument is dangerous.
The description of the PR says that the usage of the flag avoids the security vulnerability we had in the past. but this is true only if the code calling the YAML parser controls the flag fully. As soon as it makes it configurable, the same risk exists again.
And if the code does not make the flag configurable, using a different method to parse a file is not an issue (as it won't need to be configurable)

xabbuh · 2017-09-20T10:19:23Z

The example given by @jvasseur is indeed something we should consider and having a method like parseFile() will fully solve that issue. Updated accordingly.

stof · 2017-09-20T10:23:08Z

src/Symfony/Component/Yaml/Parser.php

@@ -26,6 +26,7 @@ class Parser
    const TAG_PATTERN = '(?P<tag>![\w!.\/:-]+)';
    const BLOCK_SCALAR_HEADER_PATTERN = '(?P<separator>\||>)(?P<modifiers>\+|\-|\d+|\+\d+|\-\d+|\d+\+|\d+\-)?(?P<comments> +#.*)?';

+    private $filename;


This property must be reset when doing a public call to parse(), otherwise subsequent usages of the parser would still report the previous filename.
The other alternative is to reset it to null in a finally clause in parseFile

stof · 2017-09-20T10:24:39Z

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

@@ -690,7 +705,7 @@ public function getInvalidBinaryData()

    /**
     * @expectedException \Symfony\Component\Yaml\Exception\ParseException
-     * @expectedExceptionMessage Malformed inline YAML string: {this, is not, supported}.
+     * @expectedExceptionMessage Malformed inline YAML string: {this, is not, supported} (near "{this, is not, supported}").


this message is weird. Should it really include the snippet here ?

jvasseur · 2017-09-20T12:13:28Z

src/Symfony/Component/Yaml/Parser.php

@@ -111,6 +137,8 @@ public function parse($value, $flags = 0)
            $data = $this->doParse($value, $flags);
        } catch (\Exception $e) {
        } catch (\Throwable $e) {
+        } finally {
+            $this->filename = null;


It's strange to set the property in parseFile and reset it here. Why not adding a try finally in the parseFile method instead ?

jvasseur · 2017-09-20T12:15:00Z

src/Symfony/Component/Yaml/Parser.php

+
+        $this->filename = $filename;
+
+        return $this->parse(file_get_contents($filename, $flags));


The parenthesis are at the wrong place, $flags is an argument of the parse method not the file_get_contents function.

nicolas-grekas · 2017-09-24T15:13:50Z

@stof OK for you? Still 👍 on my side with Yaml/Parser::parseFile().

javiereguiluz · 2017-09-25T14:11:27Z

src/Symfony/Component/Yaml/Parser.php

@@ -51,6 +52,35 @@ public function __construct()
    }

    /**
+     * Parses a YAML file into a PHP value.
+     *
+     * @param string $filename A string containing YAML


Please update this:

@param string $filename A string containing YAML

to something like this:

@param string $filename A relative or absolute path to the YAML file

Same for the Yaml:: parseFile() method below.

By the way ... should we replace $filename by $filepath everywhere in the code of this PR?

stof · 2017-09-25T15:26:07Z

Other components should be updated to use it

This PR was merged into the 3.4 branch. Discussion ---------- [Yaml] support parsing files | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24191 (comment) | License | MIT | Doc PR | TODO This PR adds a new flag `PARSE_FILE` which can be passed to the YAML parser. When given, the input will be interpreted as a filename whose contents will then be parsed. We already supported passing filenames in the past. Back then the features was not deterministic as it just relied on the string being an existing file or not. Now that we are able to control the behaviour of the parser by passing flags this can be done in a much cleaner way and allows to properly deal with errors (i.e. non existent or unreadable files). This change will also allow to improve error/deprecation messages to include the filename being parsed and thus showing more descriptive error messages when people are using the YAML parser with a bunch of files (e.g. as part of the DependencyInjection or Routing component). Commits ------- 9becb8a [Yaml] support parsing files

…[Validator] use the parseFile() method of the YAML parser (xabbuh) This PR was merged into the 3.4 branch. Discussion ---------- [DependencyInjection][Routing][Serializer][Translations][Validator] use the parseFile() method of the YAML parser | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24253 (comment) | License | MIT | Doc PR | Commits ------- 8bf465f use the parseFile() method of the YAML parser

xabbuh added this to the 3.4 milestone Sep 19, 2017

xabbuh mentioned this pull request Sep 19, 2017

[DependencyInjection] include file and line number in deprecation #24191

Merged

xabbuh added Feature Yaml labels Sep 19, 2017

nicolas-grekas approved these changes Sep 19, 2017

View reviewed changes

xabbuh force-pushed the yaml-parse-files branch 2 times, most recently from c7e8dce to daf9fcf Compare September 19, 2017 09:42

chalasr approved these changes Sep 19, 2017

View reviewed changes

stof requested changes Sep 19, 2017

View reviewed changes

xabbuh force-pushed the yaml-parse-files branch from daf9fcf to bc20917 Compare September 19, 2017 14:18

xabbuh force-pushed the yaml-parse-files branch from bc20917 to ae1379d Compare September 20, 2017 10:19

stof requested changes Sep 20, 2017

View reviewed changes

xabbuh force-pushed the yaml-parse-files branch 3 times, most recently from 61fb0c9 to 349120a Compare September 20, 2017 11:08

jvasseur reviewed Sep 20, 2017

View reviewed changes

xabbuh force-pushed the yaml-parse-files branch from 349120a to f8950c3 Compare September 20, 2017 22:44

chalasr approved these changes Sep 24, 2017

View reviewed changes

javiereguiluz approved these changes Sep 25, 2017

View reviewed changes

stof approved these changes Sep 25, 2017

View reviewed changes

[Yaml] support parsing files

9becb8a

xabbuh force-pushed the yaml-parse-files branch from f8950c3 to 9becb8a Compare September 25, 2017 19:23

xabbuh merged commit 9becb8a into symfony:3.4 Sep 25, 2017

xabbuh deleted the yaml-parse-files branch September 25, 2017 19:34

This was referenced Sep 25, 2017

[DependencyInjection][Routing][Serializer][Translations][Validator] use the parseFile() method of the YAML parser #24322

Merged

Ability to parse YAML files directly symfony/symfony-docs#8424

Closed

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Yaml] support parsing files #24253

[Yaml] support parsing files #24253

xabbuh commented Sep 19, 2017

nicolas-grekas left a comment

nicolas-grekas Sep 19, 2017

nicolas-grekas Sep 19, 2017

xabbuh Sep 19, 2017

stof Sep 19, 2017

jvasseur commented Sep 19, 2017

ro0NL commented Sep 20, 2017

jvasseur commented Sep 20, 2017

stof commented Sep 20, 2017

xabbuh commented Sep 20, 2017

stof Sep 20, 2017

xabbuh Sep 20, 2017

stof Sep 20, 2017

jvasseur Sep 20, 2017

jvasseur Sep 20, 2017 •

edited

Loading

nicolas-grekas commented Sep 24, 2017

javiereguiluz Sep 25, 2017

stof commented Sep 25, 2017


		$this->filename = $filename;

		return $this->parse(file_get_contents($filename, $flags));

[Yaml] support parsing files #24253

[Yaml] support parsing files #24253

Conversation

xabbuh commented Sep 19, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvasseur commented Sep 19, 2017

ro0NL commented Sep 20, 2017

jvasseur commented Sep 20, 2017

stof commented Sep 20, 2017

xabbuh commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvasseur Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas commented Sep 24, 2017

Choose a reason for hiding this comment

stof commented Sep 25, 2017

jvasseur Sep 20, 2017 •

edited

Loading