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.

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

[Yaml][Inline] Fail properly on empty object tag and empty const tag #35332

Open
wants to merge 1 commit into
base: 3.4
from

Conversation

@fancyweb
Copy link
Contributor

fancyweb commented Jan 14, 2020

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

@fancyweb

This comment has been minimized.

Copy link
Contributor Author

fancyweb commented Jan 14, 2020

Should we add this to the legacy tags btw?

@fancyweb fancyweb force-pushed the fancyweb:yaml-fail-on-empty-php-const branch 2 times, most recently from 1e9c967 to 5396cbc Jan 14, 2020
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Jan 14, 2020
src/Symfony/Component/Yaml/Inline.php Outdated Show resolved Hide resolved
src/Symfony/Component/Yaml/Inline.php Outdated Show resolved Hide resolved
@fancyweb fancyweb force-pushed the fancyweb:yaml-fail-on-empty-php-const branch from 5396cbc to 7a04b8e Jan 16, 2020
@fancyweb fancyweb force-pushed the fancyweb:yaml-fail-on-empty-php-const branch from 7a04b8e to bdf02c0 Jan 16, 2020
@@ -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]) {

This comment has been minimized.

Copy link
@fancyweb

fancyweb Jan 16, 2020

Author Contributor

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'].

This comment has been minimized.

Copy link
@fancyweb

fancyweb Jan 16, 2020

Author Contributor

Note that this must be removed once merged to 4.3.

This comment has been minimized.

Copy link
@fancyweb

fancyweb Jan 16, 2020

Author Contributor

Ref #35318 btw

@@ -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 = '';

This comment has been minimized.

Copy link
@fancyweb

fancyweb Jan 16, 2020

Author Contributor

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;

This comment has been minimized.

Copy link
@fancyweb

fancyweb Jan 16, 2020

Author Contributor

unserialize('') === false

return [
['', '!php/const'],
['', '!php/const '],
['', '!php/const '],

This comment was marked as outdated.

Copy link
@ro0NL

ro0NL Jan 16, 2020

Contributor

previously it would be " " isnt it? :P

This comment has been minimized.

Copy link
@ro0NL

ro0NL Jan 16, 2020

Contributor

i mean, if constant(' ') were to be defined, the value would be "some"

This comment has been minimized.

Copy link
@ro0NL

ro0NL Jan 16, 2020

Contributor

never mind :D it's still OK if it would be defined.

This comment has been minimized.

Copy link
@fancyweb

fancyweb Jan 16, 2020

Author Contributor

It would work but only with no error reporting on notices.

This comment has been minimized.

Copy link
@fancyweb

fancyweb Jan 16, 2020

Author Contributor

Anyway that's such a weird case. I think it doesn't matter.

This comment has been minimized.

Copy link
@fancyweb

fancyweb Jan 16, 2020

Author Contributor

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.

This comment has been minimized.

Copy link
@ro0NL

ro0NL Jan 16, 2020

Contributor

return defined('') ? constant('') : '';? 😂

This comment has been minimized.

Copy link
@fancyweb

fancyweb Jan 16, 2020

Author Contributor

Yes we can stay backward compatible by checking it. But do we want to? 😅

This comment has been minimized.

Copy link
@ro0NL

ro0NL Jan 16, 2020

Contributor

no, but that's for 5.x. It would make the feature removal/deprecation more explicit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.