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

json_encode isn't consistent when encoding anonymous functions #4035

Closed
donmccurdy opened this issue Oct 21, 2014 · 10 comments

Comments

@donmccurdy
Copy link

commented Oct 21, 2014

Admittedly not behavior that most good code should need to worry about, but json_encode() handles anonymous functions differently than the PHP implementation does. It's a more noticeable difference when encoding fails on a large array or object because of an anonymous function several levels down.

PHP 5.5 + HHVM 3.1.0

json_encode(function () {});
// => "{}"

HHVM 3.3.0 + HHVM 3.4.0-dev~nightly.2014.10.21

json_encode(function () {});
// => FALSE

@DmitrySoshnikov DmitrySoshnikov self-assigned this Oct 22, 2014

@DmitrySoshnikov

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2014

Before I started to fix it:

It seems that PHP encodes it as "{}" is probably a bug itself, since PHP's JSON serializer just (by coincidence?) encodes a closure as an array. It goes to IS_ARRAY case -- here, and then just unconditionally to this else -- here, that causes it to be serialized eventually as "{}". But by itself, PHP doesn't have special rule for encoding closures.

By itself, a closure is not serializable anywhere else, e.g. serialize(function() {}) throws in PHP. Not sure whether JSON should serialize it, and especially with "{}".

In HHVM, our JSON library is just a wrapper on top of variable-serializer, which also tries to encode a closure with an array encoder, and fails with recursion error, which is eventually is caught in the JSON wrapper, and the false is returned.

So two questions:

  1. Is it actually a "coincidence bug" in PHP, and PHP should fix it instead (by returning false e.g.)?
  2. Or do we want to conform PHP's output, and do the "{}"? In this case we'll just need to make a special case either directly in JSON wrapper, or in the variable-serializer in case if a value is of type Closure.

CC @JoelMarcey @ptarjan

@paulbiss

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2014

I would be in favor of not fixing this, might be worth opening a bug against PHP 5. It doesn't make any sense to encode a closure, and what you get back out when you decode it isn't going to be a closure.

@DmitrySoshnikov

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2014

@paulbiss: I would say so as well. I could imaging you want to encode a closure in case it could have some additional properties, but it's not possible to attach any property on a closure object (the t___set throws there), and it's not possible to decode "{}" into closure back.

FWIW, e.g. ECMAScript also doesn't encodes a function, but returns undefined (which can be considered similar to HHVM's false return which is not a JSON string):

// JavaScript
console.log(JSON.stringify(function() {}) === undefined); // true

This is reasonable, since JSON spec doesn't support functions, only objects, arrays and primitives.

@donmccurdy, you mention you have some legal practical use-case of encoding closures? Can you show one please? In any case, I think we should file a bug against PHP5 instead.

@donmccurdy

This comment has been minimized.

Copy link
Author

commented Oct 22, 2014

Thanks all – ideally PHP and HHVM would behave consistently, but yes I'd agree that PHP's behavior may not be intentional or correct. Opening a bug against PHP5 sounds reasonable.

@DmitrySoshnikov I don't know of any case where you would want to encode a closure and get anything meaningful back when you decode it, but I ran into this because there happened to be a closure mixed into the content I actually cared about. One interesting comparison with JavaScript and PHP here:

// JavaScript
JSON.stringify([1,2,3, function () {}]) // "[1,2,3,null]"
// PHP
json_encode([1,2,3, function () {}]); // "[1,2,3,{}]"

// HHVM
json_encode([1,2,3, function () {}]); // FALSE
@DmitrySoshnikov

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2014

@donmccurdy: OK, thanks for the example. Still the "{}" doesn't make big sense in case of PHP, unless it is specified behavior of PHP (although, I don't see specification of json_encode yet in the initial PHP spec).

ECMA-262 (JavaScript spec) actually specifies the behavior of returning undefined value (which is rendered as null in arrays) for the values not related to JSON (like function):

NOTE 5 Values that do not have a JSON representation (such as undefined and functions) do not produce a String. Instead they produce the undefined value. In arrays these values are represented as the String null. In objects an unrepresentable value causes the property to be excluded from stringification.

That HHVM completely fails and just returns false instead of returning an encoded array seems bad, I guess we can fix it. The thing is: how we should specify it? Returning null for closures as JS does seems reasonable. And we will file a bug for PHP to fix "{}" to be null as well. Unless again, it's specified somewhere, but I couldn't find this spec for PHP's json_encode (let me know in case).

So, I'd say:

// HHVM (now)
json_encode([1,2,3, function () {}]); // false

// HHVM (could/should be)
json_encode([1,2,3, function () {}]); // "[1, 2, 3, null]"

Thoughts?

@ptarjan

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2014

I actually quite like false and would even ask that we emit a warning like 5.3 and 5.4 used to do for files:

http://3v4l.org/B1Mr8

But yes, our policy is to match php5 by default and then add ini settings (like hack.lang.do_better_thing) to help people migrate to a saner runtime behavior.

@donmccurdy

This comment has been minimized.

Copy link
Author

commented Oct 22, 2014

Nice find on the JSON.stringify spec – That sounds perfect to me, all the better if PHP does the same. If there weren't historical behavior to go on, false would probably make more sense though.

@DmitrySoshnikov

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2014

Yeah, good point on raising a warning. I think we can do the warning, and at the same time destroying a container by just returning false as it is now seems less compatible with encoders from other languages (like JS or the same PHP).

OK, what I'm gonna do is:

  1. Clarify whether PHP guys want to return null instead of "{}" for closures (or in general -- for any non-encodeable value) -- I'm pretty sure it's just a bug form analyzing their code;
  2. Fix it for containers instead of failing with just false in HHVM;
  3. Issue a warning on a non-encodable value (or potentially even throw in a strict mode).

Does it sound about right?

@DmitrySoshnikov

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2014

OK, let's postpone this one for now, and see what PHP guys will reply: https://bugs.php.net/bug.php?id=68288.

@paulbiss

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2014

I think (1) sounds good, adding a warning sounds good too. I think we should still return false at least in EnableHipHopSyntax mode, but I don't really feel too strongly one way or the other though.

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