Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Yaml] add tests for specific mapping keys #21650

Merged
merged 1 commit into from Mar 1, 2017
Merged

[Yaml] add tests for specific mapping keys #21650

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/Symfony/Component/Yaml/Tests/InlineTest.php
View file
Open in desktop
Expand Up @@ -420,4 +420,15 @@ public function testVeryLongQuotedStrings()

$this->assertEquals($longStringWithQuotes, $arrayFromYaml['longStringWithQuotes']);
}

public function testBooleanMappingKeysAreConvertedToStrings()
{
$this->assertSame(array('false' => 'foo'), Inline::parse('{false: foo}'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would test both true and false

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, did that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should not be allowed actually, keys should be treated like any other value and evaluated, so boolean should not be allowed as a key.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if we want to forbid using boolean as keys because they are not supported in PHP, this must be done in master with a deprecation.
Having these tests in our maintenance branches are good to avoid introducing BC breaks by mistake.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, if we start evaluating, we would also have to forbid floats to avoid breaking BC.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we don't know if the user wants false to be dumped as a string or as the boolean value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a bool key is supported neither in php nor in symfony, i think we can safelly guess the user wants a string.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is this is broken if they use the same YAML file with another language that supports boolean keys as parsing and then dumping the parsed structure changes the result for other languages. Deprecating the current behaviour doesn't change anything about the inconsistency between the different languages, but it makes it obvious and prevents hard to discover bugs related to these inconsistencies.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point too... Sounds complicated to fix after all, should we consider it as won't fix or would you prefer deprecating evaluated keys as I proposed at the beginning (note that this second solution may cause a small overhead)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest that we deprecate parsing keys as strings implicitly and make this an opt-in feature instead (see #21774).

$this->assertSame(array('true' => 'foo'), Inline::parse('{true: foo}'));
}

public function testTheEmptyStringIsAValidMappingKey()
{
$this->assertSame(array('' => 'foo'), Inline::parse('{ "": foo }'));
}
}