[Yaml] Fail properly when the !php/const tag is used in a mapping key #35208
Conversation
de6715b to
1a71ac4
Compare
| @@ -712,8 +717,12 @@ private static function evaluateScalar($scalar, $flags, $references = []) | |||
| return null; | |||
| case 0 === strpos($scalar, '!php/const'): | |||
There was a problem hiding this comment.
The legacy tag does not need to be taken into account since they finish with ":", the mapping key is always the new tag.
There was a problem hiding this comment.
If this works with the legacy tag, this means to me that having an error during parsing when using the new tag is a bug and we should see how to fix that instead of changing the error message.
There was a problem hiding this comment.
It doesn't work with the legacy tag. If you use the legacy tag as a key, eg !php/const:MY_CONST: "value", then the parsed key is !php/const (the new one) because : is the key / value separator. So it's impossible to be a in a $isMappingKey case and in a legacy tag that ends with :.
| @@ -712,8 +717,12 @@ private static function evaluateScalar($scalar, $flags, $references = []) | |||
| return null; | |||
| case 0 === strpos($scalar, '!php/const'): | |||
| if (self::$constantSupport) { | |||
| if ($isMappingKey) { | |||
There was a problem hiding this comment.
One edge case is if the constant '' is defined (define('', 'foo')). In this case it currently works but the resulting key value is the tag (!php/const), not the evaluated value. So I guess we don't care of breaking this case.
|
Other tags also have major problems, the key is never evaluated, but in the end they "work", ie they don't end up in a exception. That's why I changed only this one since it already end up in a exception anyway. But I think we should completely fix all those behaviors, but on master (with deprecations). It's difficult on 3.4 with all the legacy tags. |
1a71ac4 to
e4254fe
Compare
| @@ -320,7 +320,11 @@ private static function dumpArray($value, $flags) | |||
| */ | |||
| public static function parseScalar($scalar, $flags = 0, $delimiters = null, &$i = 0, $evaluate = true, $references = [], $legacyOmittedKeySupport = false) | |||
| { | |||
| if (\in_array($scalar[$i], ['"', "'"])) { | |||
| if ('' === $scalar = (string) $scalar) { | |||
There was a problem hiding this comment.
This cast can be removed when merged in upper branches since the parameter is typed, so when false is passed it is converted to ''.
There was a problem hiding this comment.
The docblock states that $scalar is a string. Can we detect the cases where we pass something else here and terminate there instead?
There was a problem hiding this comment.
What I did first was to cast to string before calling this method, to stay compliant with the method contract, but see #35208 (comment).
There was a problem hiding this comment.
Why do we have to call the method at all if the scalar is not a string?
| @@ -320,7 +320,11 @@ private static function dumpArray($value, $flags) | |||
| */ | |||
| public static function parseScalar($scalar, $flags = 0, $delimiters = null, &$i = 0, $evaluate = true, $references = [], $legacyOmittedKeySupport = false) | |||
| { | |||
| if (\in_array($scalar[$i], ['"', "'"])) { | |||
| if ('' === $scalar = (string) $scalar) { | |||
There was a problem hiding this comment.
The docblock states that $scalar is a string. Can we detect the cases where we pass something else here and terminate there instead?
| @@ -712,8 +717,12 @@ private static function evaluateScalar($scalar, $flags, $references = []) | |||
| return null; | |||
| case 0 === strpos($scalar, '!php/const'): | |||
There was a problem hiding this comment.
If this works with the legacy tag, this means to me that having an error during parsing when using the new tag is a bug and we should see how to fix that instead of changing the error message.
|
I had a deeper look at this and I think not parsing the constants in YAML keys is a bug of the parser when parsing the inline notation. Using the expanded syntax constants are parsed properly (we actually did a bugfix for that in the past too, see #22878). So I would actually fix the parser instead (see #35318 for it). |
|
#35318 has been merged |
… const tag (fancyweb) This PR was merged into the 3.4 branch. Discussion ---------- [Yaml][Inline] Fail properly on empty object tag and empty const tag | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Rework of #35208 to not end up in `parseScalar` with an empty string or a boolean (and thus, avoid unfriendly error such as `Trying to access array offset on value of type bool`). Ping @xabbuh Commits ------- bdf02c0 [Yaml][Inline] Fail properly on empty object tag and empty const tag
This PR improves the DX of the linked issue case + fixes the parseScalar method + fixes some cases that call it with non string values.