[YAML] Added support for object-maps #10114
Conversation
|
Returning an object for an empty map is totally unusable for calling code as it must now distinguish the case of an empty or non-empty map (which is still an array). |
Previously, the parser treated maps ( {} ) the same as sets ( [] ).
Both were returned as PHP associative arrays. Since
these are distinct entities, this can cause considerably problems for
the users, especially when YAML is being serialized into another
format such as JSON.
This commit allows the user to enable object-map support via a third
parameter on the Parse method. It defaults to `false`, which means
that this commit does not break backwards compatibility.
If the user enables object-map support, maps are represented
by stdClass() objects. Sets remain as arrays.
|
Reconfigured the code to return objects for all maps, instead of array + empty objects as before. Tests have been updated to reflect the changes depending on which flag you use. Also updated the title of the PR and the description to match the new functionality. Added PR form/table too. |
|
Updated PR to comply with coding standards. |
|
I think rather than checking each time if you're using an object or not, you could work with an array, and then at the end test your |
|
Yeah, that makes sense. Updated the code to check only on return. Tests seem to pass...but there was a random failure in There is also a small coding standard fix that I'll push if this newest iteration of code looks good. Thanks for taking a look! |
| @@ -312,17 +314,19 @@ private static function parseSequence($sequence, &$i = 0) | |||
| throw new ParseException(sprintf('Malformed inline YAML string %s', $sequence)); | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Removing! I didn't think about it because Fabbot didn't yell at me :)
| $this->assertFail(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Argh, yep. Lemme look through the whole PR and see if I've done this anywhere else :)
| @@ -295,7 +296,8 @@ private static function parseSequence($sequence, &$i = 0) | |||
| if (!$isQuoted && false !== strpos($value, ': ')) { | |||
| // embedded mapping? | |||
| try { | |||
| $value = self::parseMapping('{'.$value.'}'); | |||
| $j = 0; | |||
| $value = self::parseMapping('{'.$value.'}', $j, $objectForMap); | |||
There was a problem hiding this comment.
The j variable is a bit useless here, why don't you use 0 as argument?
There was a problem hiding this comment.
Unfortunately, it is required. :(
parseMapping() requires the second parameter to be passed by reference:
private static function parseMapping($mapping, &$i = 0, $objectForMap = false)PHP disallows passing constants by reference because the method may mutate the value (and indeed it does). So you must create a temporary variable and pass that instead. PHP doesn't mind if you omit the parameter because of the default on the function, but if you specify arguments that come after the referenced parameter, you must supply one.
I could do the variable assignment inline, but it's effectively the same thing:
$value = self::parseMapping('{'.$value.'}', $j = 0, $objectForMap);Open to alternatives, but I did it this way because alternative approaches (removing the reference) would require fairly drastic restructuring of how the parsing works for what is ultimately just a little semantic blemish :)
There was a problem hiding this comment.
sorry, missed that. Sad it has to be done this way :(
|
Anything else that needs to be done? I have a whole pile of code that would love to start differentiating between objects and arrays :) |
|
Hello @polyfractal , |
|
Closing in favor of #10552 |
…olas-grekas) This PR was merged into the 2.5-dev branch. Discussion ---------- [YAML] Added support for object-maps | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | NA | License | MIT | Doc PR | NA Proposal for #10114 Commits ------- 0cee604 [Yaml] Cleanups for object maps e2d5468 [Yaml] Added support for object maps
Previously, the parser treated maps (
{}) the same as sets ([]). Both were returned as a PHP associative array. Since these are distinct entities, this can cause considerably problems if you treat the two types differently, especially when YAML is being serialized into another format such as JSON.This PR enables empty-map support via a third optional parameter on the Parse method. It defaults to
false, which means that this commit does not break backwards compatibility.If the user enables object-map support, maps are represented by a
stdClass()object. Sets remain as PHP arrays. This is compatible with other serialization methods, such as json_encode where empty objects are properly encoded to{}.The tests are a little bulky, since
assertSame()is too strict (e.g. two empty objects are not identical, since they are discrete objects), butassertEquals()is too loose (e.g. loses type-checking on the rest of the values). I've implemented a recursive assert that uses the stricterassertSame()on non-objects, and a slightly looser combo of asserts for objects.