Fixed list handing when dealing with object maps

[kleijnweb]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#17710, #17709
License	MIT
Doc PR	N/A

[kleijnweb]

Build is failing but that has nothing to do with this PR :(

[xabbuh]

Can you give an example of one of the 1% use cases where this would still fail?

[kleijnweb]

@xabbuh It's probably less than 1%. It's because if you don't make object for object literals at parse time, you can end up with arrays intended as hash maps that are indistinguishable from arrays intended as lists.

    public function testWillParseZeroIndexedNumericMapAsObject()
    {
        $yaml = <<<YAML
map:
  0: one
  1: two
YAML;
        $actual = $this->parser->parse($yaml, true, false, true);
        $this->assertInternalType('object', $actual);
        $this->assertInternalType('object', $actual->map);
    }

The component should definitely be refactored though. Looks pretty daunting, eyeballing it the cyclomatic complexity looks through the roof.

Given the rarity of these cases, I'd say this should not be conditional for this fix though.

[kleijnweb]

Should anyone else be running into this issue (older versions are affected as well), here's an example that does pretty much the same but on the result (less efficient but does the trick): https://github.com/kleijnweb/swagger-bundle/blob/feature/issue-27/src/Document/YamlParser.php

[xabbuh]

@kleijnweb Can't we simply solve it by checking whether the array was created by parsing a sequence or by parsing a mapping?

[kleijnweb]

Sure. But I am not going to touch the parsing logic with a stick. It should be refactored first.

[xabbuh]

Well, I don't see any refactoring being done here soon, but I would like to see this issue solved properly.

[kleijnweb]

Then I'm afraid we've reached an impasse, because I can't donate that much time right now. So you're left with the following choices:

1. Wait for someone who doesn't mind hacking into a huge decision tee, in the mean time leave this in the completely broken state that it is
2. Merge this anyway so that it is fixed for over 99% of use cases. You could leave [YAML] Not handling numeric keys in objects #17709 open, although I would suggest creating a new ticket for hash maps with sequential keys starting at 0.

[xabbuh]

I'll have a look into this later today to check if it turns out to be as easy as I expect it to be. If it proves to be too difficult I agree with you to merge your patch as it already improves the majority of use cases that are affected by this issue.

Anyway, thank you very much for raising the issue and for opening this PR. That's really appreciated!

[xabbuh]

closing in favour of #17729

[xabbuh]

@kleijnweb Can you please check if #17729 solves all of your issues?

[kleijnweb]

Tests look good. Nice job :)