-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Fixed list handing when dealing with object maps #17711
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
Conversation
johnknl
commented
Feb 6, 2016
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 |
Build is failing but that has nothing to do with this PR :( |
Can you give an example of one of the 1% use cases where this would still fail? |
@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. |
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 |
@kleijnweb Can't we simply solve it by checking whether the array was created by parsing a sequence or by parsing a mapping? |
Sure. But I am not going to touch the parsing logic with a stick. It should be refactored first. |
Well, I don't see any refactoring being done here soon, but I would like to see this issue solved properly. |
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:
|
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! |
closing in favour of #17729 |
@kleijnweb Can you please check if #17729 solves all of your issues? |
Tests look good. Nice job :) |
This PR was merged into the 2.8 branch. Discussion ---------- [Yaml] properly parse lists in object maps | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #17709, #17710, #17711 | License | MIT | Doc PR | * do not cast parsed sequences to objects * properly handle numeric mapping keys Commits ------- ee9ca93 [Yaml] properly parse lists in object maps