[Yaml][Inline] Fail properly on empty object tag and empty const tag #35332
Conversation
|
Should we add this to the legacy tags btw? |
1e9c967 to
5396cbc
Compare
5396cbc to
7a04b8e
Compare
7a04b8e to
bdf02c0
Compare
| @@ -506,7 +506,12 @@ private static function parseMapping($mapping, $flags, &$i = 0, $references = [] | |||
|
|
|||
| if ('!php/const' === $key) { | |||
| $key .= self::parseScalar($mapping, $flags, [':', ' '], $i, false, [], true); | |||
| $key = self::evaluateScalar($key, $flags); | |||
| if ('!php/const:' === $key && ':' !== $mapping[$i]) { | |||
There was a problem hiding this comment.
This is a bug fix. {!php/const: foo} is currently interpreted as legacy php/const: tag + '' as the const value while it should end up as ['' => 'foo'].
There was a problem hiding this comment.
Note that this must be removed once merged to 4.3.
| @@ -506,7 +506,12 @@ private static function parseMapping($mapping, $flags, &$i = 0, $references = [] | |||
|
|
|||
| if ('!php/const' === $key) { | |||
| $key .= self::parseScalar($mapping, $flags, [':', ' '], $i, false, [], true); | |||
| $key = self::evaluateScalar($key, $flags); | |||
| if ('!php/const:' === $key && ':' !== $mapping[$i]) { | |||
| $key = ''; | |||
There was a problem hiding this comment.
No need to evaluate because we know it's gonna end up as the empty string (the key === !php/const).
| @@ -692,6 +697,10 @@ private static function evaluateScalar($scalar, $flags, $references = []) | |||
| return null; | |||
| case 0 === strpos($scalar, '!php/object'): | |||
| if (self::$objectSupport) { | |||
| if (!isset($scalar[12])) { | |||
| return false; | |||
There was a problem hiding this comment.
unserialize('') === false
| return [ | ||
| ['', '!php/const'], | ||
| ['', '!php/const '], | ||
| ['', '!php/const '], |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
i mean, if constant(' ') were to be defined, the value would be "some"
There was a problem hiding this comment.
never mind :D it's still OK if it would be defined.
There was a problem hiding this comment.
It would work but only with no error reporting on notices.
There was a problem hiding this comment.
Anyway that's such a weird case. I think it doesn't matter.
There was a problem hiding this comment.
never mind :D it's still OK if it would be defined.
No. My previous replies were to your first message. If the '' constant is defined, it is never resolved now.
There was a problem hiding this comment.
return defined('') ? constant('') : '';? 😂
There was a problem hiding this comment.
Yes we can stay backward compatible by checking it. But do we want to? 😅
There was a problem hiding this comment.
no, but that's for 5.x. It would make the feature removal/deprecation more explicit.
|
Thank you @fancyweb. |
… 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
…t a value (fancyweb) This PR was merged into the 5.1-dev branch. Discussion ---------- [Yaml] Deprecate using the object and const tag without a value | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | Deprecations? | yes | Tickets | #35332 (comment) | License | MIT | Doc PR | - WIP because it needs 3.4 merged up into master + #35332. Commits ------- 89062b9 [Yaml] Deprecate using the object and const tag without a value
Rework of #35208 to not end up in
parseScalarwith an empty string or a boolean (and thus, avoid unfriendly error such asTrying to access array offset on value of type bool).Ping @xabbuh