[Yaml] fix for wrong parsing functionality on some indentation cases #8100

dekelev · 2013-05-19T23:05:36Z

No description provided.

I have problems at the moments setting up PHPUnit testing environment, so any help with testing it would be appreciated.

indeyets · 2013-05-20T06:20:16Z

@dekelev is your "will work" example valid? I can't parse it in my head, as you inject a sequence in the middle of a mapping.

"won't work" example is another thing — sequence is a value, corresponding to key a in mapping

dekelev · 2013-05-20T06:57:34Z

Because YAML depends on space indentation, "- b: c" line will always be a sibling to key "a", and not consider its mapped value.
For it to be consider as value of "a", "- b: c " line should be indented at least one space more.

And regards to the empty value:
null is a "no value" value.

#7274
http://yaml.org/spec/history/2001-08-01.html

So "a" key has a value, NULL value, because next line indentation is equal to this line which excludes it from mapping it as "a" value.

indeyets · 2013-05-20T07:07:36Z

Indentation is not the only mean for key/value distinction. see this http://www.yaml.org/spec/1.2/spec.html#id2772312

dekelev · 2013-05-20T07:38:33Z

Right, but in these examples there is no other means of key/value distinction. only key/value mapping using colon+indentation OR colon+inline value

indeyets · 2013-05-20T07:51:13Z

"won't work" example is very similar to example from spec and unambiguously implies the following structure:

{test: {a: [{b: c}]}}

dekelev · 2013-05-20T07:59:30Z

---
test:
  a:
  - b: c

should implies

{test: {a: null, [{b: c}]}}

while

---
test:
  a:
   - b: c

should implies

{test: {a: [{b: c}]}}

stof · 2013-05-20T08:12:14Z

@dekelev Tests are missing for your fix.

and please update the description with the PR summary

indeyets · 2013-05-20T08:21:19Z

@dekelev {test: {a: null, [{b: c}]}} is NOT a valid data structure. YAML spec definitely means {test: {a: [{b: c}]}} in both cases. I already provided link to example in spec which supports my point of view.

stof · 2013-05-20T08:26:14Z

FYI, both http://yamllint.com/ (using Ruby) and http://yaml-online-parser.appspot.com/ (using pyyaml in Python) are parsing it in the way described by @indeyets in the previous comment.

dekelev · 2013-05-20T08:33:27Z

@indeyets @stof you are probably right here
In any case, this is an issue required a fix instead of throwing ParseException.
Will get to it later

indeyets · 2013-05-20T08:36:15Z

@dekelev yup. see #8076
I filed #8093 separately, as symptoms differ.

it's good if those are fixed by single commit, but I decided that it makes sense to track them separately

dekelev · 2013-05-20T08:40:57Z

thanks @indeyets, I'll try handle both cases in next commit.

lavoiesl · 2013-11-23T22:03:45Z

I dont get it, {test: {a: [{b: c}]}} should be :

test:
  a:
    - b: c

and this should not even be valid:

test:
  a:
  - b: c

it would indeed correspond to {test: {a: null, [{b: c}]}}, but it makes no sense because you are mixing numerical indices and keys

From what I understand, this PR tries to let the parser be more forgiving while the input should have been correct in the first place.

indeyets · 2013-11-24T07:01:32Z

@lavoiesl please read the spec and previous comments. It is a valid syntax

cordoval · 2013-12-07T03:00:55Z

just in case you guys are not aware #8562
I personally think the yaml component and the Parse class is one of the most needing love. It does the job for symfony and that was enough back at the time but if you are trying to parse yaml independently and use all spec from yaml then you will find several weaknesses. i was thinking working on this but the constraints of "not a lot of code" discouraged me. If i were to implement this parser again then i would port this stdlib from ruby https://github.com/tenderlove/psych/blob/master/test/psych/test_parser.rb not sure if still at the time capable of doing successfully or quick enough. But it is a hint. Let's relate the tickets regarding these yaml limitations at least so we don't multiply tickets.

👶

fabpot · 2013-12-29T16:03:41Z

Closing this one as the Symfony YAML component has never been about implementing the whole YAML spec, but just the most-commonly used subset for configuration files. Doing any changes must come with a large number of unit tests to be sure than nothing breaks. This change does break things, so it cannot be merged anyway. Feel free to reopen with updated unit tests though.

This is a fix for YAML error from issue #8093

325d9fb

I have problems at the moments setting up PHPUnit testing environment, so any help with testing it would be appreciated.

dekelev mentioned this pull request May 19, 2013

[YAML] more indentation weirdness #8093

Closed

fabpot closed this Dec 29, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Yaml] fix for wrong parsing functionality on some indentation cases #8100

[Yaml] fix for wrong parsing functionality on some indentation cases #8100

[Yaml] fix for wrong parsing functionality on some indentation cases #8100

[Yaml] fix for wrong parsing functionality on some indentation cases #8100

Conversation