Welcome! Log In Create A New Profile

Advanced

[PHP-DEV] Exposing jsonSerialize's depth

Posted by Jani Ollikainen 
Jani Ollikainen
[PHP-DEV] Exposing jsonSerialize's depth
January 11, 2017 09:10AM
Hi,

I've made a pull request where I was asked to start internal discussion about it.. The pull request can be found:
https://github.com/php/php-src/pull/1884
Which relates to request I've also made:
https://bugs.php.net/bug.php?id=72073

For short the thing is:
"We have objects that do dynamic loading and there might be recursions. We could use recursion depth to decide when we want to do dynamic loading and when not.

Now there doesn't seem to be any way to know how deep is the json_encode process going. If we try to use some level in jsonSerialize it just calls one of those, no recursion there.

This could be easily archived by exposing the internal encoder_depth to jsonSerialize."

As I don't know any other way to get the depth info from jsonSerialize, as it isn't called recursively in PHP side, so adding some static class variable and incrementing/decrementing it in jsonSerialize will just not work.

Then to why would someone really need that? Well, we have models that uses MongoDB as a database (created from MongoDB objects) and the models can have references to other models (coming from MongoDB objects). That loading will happen dynamically and there might be recursions, but we want
still to serialize them as we really don't need to go that deep even if there is a recursion. This way we can control the depth we are loading new objects and
can avoid recursion happening in jsonSerialize and can get the data we need serialized.

If you are thinking why don't you just use the depth option of json_encode, well unfortunately it doesn't help. It seems to go too deep and get recursion errors and would just later limit the depth it's going as my example will show if you try if you run it without patched json.

Any comments/ideas?

--
Jani Ollikainen


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Jakub Zelenka
Re: [PHP-DEV] Exposing jsonSerialize's depth
January 15, 2017 10:00PM
On Wed, Jan 11, 2017 at 8:06 AM, Jani Ollikainen <[email protected]>
wrote:

> Hi,
>
> I've made a pull request where I was asked to start internal discussion
> about it.. The pull request can be found:
> https://github.com/php/php-src/pull/1884
> Which relates to request I've also made:
> https://bugs.php.net/bug.php?id=72073
>
> For short the thing is:
> "We have objects that do dynamic loading and there might be recursions. We
> could use recursion depth to decide when we want to do dynamic loading and
> when not.
>
> Now there doesn't seem to be any way to know how deep is the json_encode
> process going. If we try to use some level in jsonSerialize it just calls
> one of those, no recursion there.
>
> This could be easily archived by exposing the internal encoder_depth to
> jsonSerialize."
>
> As I don't know any other way to get the depth info from jsonSerialize, as
> it isn't called recursively in PHP side, so adding some static class
> variable and incrementing/decrementing it in jsonSerialize will just not
> work.
>
> Then to why would someone really need that? Well, we have models that uses
> MongoDB as a database (created from MongoDB objects) and the models can
> have references to other models (coming from MongoDB objects). That loading
> will happen dynamically and there might be recursions, but we want
> still to serialize them as we really don't need to go that deep even if
> there is a recursion. This way we can control the depth we are loading new
> objects and
> can avoid recursion happening in jsonSerialize and can get the data we
> need serialized.
>
> If you are thinking why don't you just use the depth option of
> json_encode, well unfortunately it doesn't help. It seems to go too deep
> and get recursion errors and would just later limit the depth it's going as
> my example will show if you try if you run it without patched json.
>
> Any comments/ideas?
>
>

I don't really like this. The reason is that you don't actually modify
JsonSerializable interface for the obvious BC break that it would cause it.
It means that the function just gets this parameter and it kind of raises a
question how we should document it? The solution would be to extend
JsonSerializable with some new interface. However I don't think it's worth
it for such thing. Maybe you should consider to pre-process your data
before passing it to json_encode...

Cheers

Jakub
Stanislav Malyshev
Re: [PHP-DEV] Exposing jsonSerialize's depth
January 16, 2017 03:00AM
Hi!

> For short the thing is: "We have objects that do dynamic loading and
> there might be recursions. We could use recursion depth to decide
> when we want to do dynamic loading and when not.
>

This looks like a hack covering for deficient design. Having objects
that look differently depending on hidden parameters without anybody
explicitly modifying them is a recipe for very hard to track bugs. I
don't think we should encourage this.

There are ways to present serialized view of the object (sleep/wakeup,
Serializable, etc.) but I don't think depth has anything to do with it.

--
Stas Malyshev
smalyshev@gmail.com

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Jani Ollikainen
RE: [PHP-DEV] Exposing jsonSerialize's depth
January 16, 2017 09:30AM
Hi,

> I don't really like this. The reason is that you don't actually modify JsonSerializable interface for the obvious BC break that it would cause it. It means that the > function just gets this parameter and it kind of raises a question how we should document it? The solution would be to extend JsonSerializable with some
> new interface. However I don't think it's worth it for such thing. Maybe you should consider to pre-process your data before passing it to json_encode...

What BC break are you talking about? There is no need for using the parameter in old codes. Even if we pass that depth to jsonSerialize doing something like:
"public function jsonSerialize() {...}" will still work without any problems.

And how would you document it it? Like any other.. if there now is: http://php.net/manual/en/jsonserializable.jsonserialize.php
abstract public mixed JsonSerializable::jsonSerialize ( void )
This function has no parameters.
Then it would just be:
abstract public mixed JsonSerializable::jsonSerialize ( [integer $depth] )
Parameters:
depth
Depth of the current json_encode -call.

New interface would also be good, but as you said it, it doesn't seem to be worth the trouble.

Well, and what do you think how much does going pre-processing slow the code versus without need for pre-processing? That really doesn't sound option to me.
Jani Ollikainen
RE: [PHP-DEV] Exposing jsonSerialize's depth
January 16, 2017 09:30AM
Hi,

> This looks like a hack covering for deficient design. Having objects that look differently depending on hidden parameters without anybody explicitly
> modifying them is a recipe for very hard to track bugs. I don't think we should encourage this.
>
> There are ways to present serialized view of the object (sleep/wakeup, Serializable, etc.) but I don't think depth has anything to do with it.

There is no hidden parameters. There is no "without anybody explicitly modifying". It's data that is in MongoDB. It's basicly autoloading for mongodb's dbrefs. https://docs.mongodb.com/manual/reference/database-references/#dbrefs

And yes, it doesn't have anything to do with that.




--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Niklas Keller
Re: [PHP-DEV] Exposing jsonSerialize's depth
January 16, 2017 11:20AM
2017-01-16 9:19 GMT+01:00 Jani Ollikainen <[email protected]>:

> Hi,
>
> > I don't really like this. The reason is that you don't actually modify
> JsonSerializable interface for the obvious BC break that it would cause it.
> It means that the > function just gets this parameter and it kind of raises
> a question how we should document it? The solution would be to extend
> JsonSerializable with some
> > new interface. However I don't think it's worth it for such thing. Maybe
> you should consider to pre-process your data before passing it to
> json_encode...
>
> What BC break are you talking about? There is no need for using the
> parameter in old codes. Even if we pass that depth to jsonSerialize doing
> something like:
> "public function jsonSerialize() {...}" will still work without any
> problems.
>

That BC break: https://3v4l.org/L1u7q

Regards, Niklas


> And how would you document it it? Like any other.. if there now is:
> http://php.net/manual/en/jsonserializable.jsonserialize.php
> abstract public mixed JsonSerializable::jsonSerialize ( void )
> This function has no parameters.
> Then it would just be:
> abstract public mixed JsonSerializable::jsonSerialize ( [integer $depth] )
> Parameters:
> depth
> Depth of the current json_encode -call.
>
> New interface would also be good, but as you said it, it doesn't seem to
> be worth the trouble.
>
> Well, and what do you think how much does going pre-processing slow the
> code versus without need for pre-processing? That really doesn't sound
> option to me.
>
>
Jakub Zelenka
Re: [PHP-DEV] Exposing jsonSerialize's depth
January 16, 2017 01:10PM
On Mon, Jan 16, 2017 at 8:19 AM, Jani Ollikainen <[email protected]>
wrote:

> Hi,
>
> > I don't really like this. The reason is that you don't actually modify
> JsonSerializable interface for the obvious BC break that it would cause it.
> It means that the > function just gets this parameter and it kind of raises
> a question how we should document it? The solution would be to extend
> JsonSerializable with some
> > new interface. However I don't think it's worth it for such thing. Maybe
> you should consider to pre-process your data before passing it to
> json_encode...
>
> What BC break are you talking about? There is no need for using the
> parameter in old codes. Even if we pass that depth to jsonSerialize doing
> something like:
> "public function jsonSerialize() {...}" will still work without any
> problems.
>
> And how would you document it it? Like any other.. if there now is:
> http://php.net/manual/en/jsonserializable.jsonserialize.php
> abstract public mixed JsonSerializable::jsonSerialize ( void )
> This function has no parameters.
> Then it would just be:
> abstract public mixed JsonSerializable::jsonSerialize ( [integer $depth] )
>

That's not what is implemented. You just call jsonSerialize with an extra
parameter and basically create a contract that is not specified by the
interface. As you can see from the example that Niklas sent, you can't just
add an optional parameter to the interface method as it would be a BC break.

Cheers

Jakub
Jani Ollikainen
RE: [PHP-DEV] Exposing jsonSerialize's depth
January 16, 2017 02:40PM
Hi.

>> What BC break are you talking about? There is no need for using the parameter in old codes. Even if we pass that depth to jsonSerialize
>> doing something like: "public function jsonSerialize() {...}" will still work without any problems.
> That BC break: https://3v4l.org/L1u7q

And where you have it like that? Yes, if I run your example I have that error. If with patched php/json or non-patched, I can have:
https://3v4l.org/BccPi and it just works. Without any problems.. But now I understands what you meant by documentation,
if you want to describe interface JsonSerializable with PHP. Yes then if you do it like that it causes BC problems.
If you just leave it like it is, it works. Granted it's kind of weird then. With BC problem I don't see a problem seeing this in some
future PHP version that has that BC change.
Niklas Keller
Re: [PHP-DEV] Exposing jsonSerialize's depth
January 16, 2017 04:00PM
>
> >> What BC break are you talking about? There is no need for using the
> parameter in old codes. Even if we pass that depth to jsonSerialize
> >> doing something like: "public function jsonSerialize() {...}" will
> still work without any problems.
> > That BC break: https://3v4l.org/L1u7q
>
> And where you have it like that? Yes, if I run your example I have that
> error. If with patched php/json or non-patched, I can have:
> https://3v4l.org/BccPi and it just works. Without any problems.. But now
> I understands what you meant by documentation,
> if you want to describe interface JsonSerializable with PHP. Yes then if
> you do it like that it causes BC problems.
> If you just leave it like it is, it works. Granted it's kind of weird
> then. With BC problem I don't see a problem seeing this in some
> future PHP version that has that BC change.
>

Even if you leave the interface as is and just pass the parameter
additionally, it could have been extended by users just like in your
example and pass a wrong value then.

Regards, Niklas
Jani Ollikainen
RE: [PHP-DEV] Exposing jsonSerialize's depth
January 16, 2017 05:00PM
Hi,

> Even if you leave the interface as is and just pass the parameter additionally, it could have been extended by users just like in your example and pass a
> wrong value then.

Yes, it might. I don't come up an idea why, but maybe someone has had a reason, as the json_encode calling won't pass any variable there.
So this has BC issue and as it has BC you won't like it. What are then the options? I'm not familiar how this PHP thing is developed.

- Do a BC incompatible change in future, will probably need a voting and most probably people won't see the case for it and will not accept it.
(as I've already seen the "you're doing it wrong" comments, but I'm not so sure that the dynamic object-relational-mapping that loads
references is so stupid, as if it wouldn't do that, then we would have to tell, case by case what to relations load and when. Now it just loads
everything but stops at the depth needed. Also somehow we would probably want to say load the first 'parents' reference but not from
the next ones, which would mean knowing the depth.).

- Do it another way? So that it wouldn't be BC incompatible? How would one do that smart and so that it would be accepted? I don't see
good way of going it.. Having some jsonSerializeDepth that gets called if it exists, not jsonSerialize being pretty. Any other ideas?

- Fixing json_encode, as there wouldn't be a problem if json_encode's depth would be useful, like limiting the depth to given depth and not going
too deep and getting into trouble.. Now the depth is just pretty useless, oh, it's too deep, throw error, not like just limit to this given depth.
But changing that would also be BC incompatible. And after thinking why the depth came there https://bugs.php.net/bug.php?id=62369 it just
fixed stack overflow. And then if one tries to fix that from json_encode's side then without BC issues it would need some new parameter
like boolean $cut = false, which would then make it cut at the depth not like it does now. But that parameter for start seems pretty stupid and
don't know how big thing that would be to implement.

- Continue using patched json, or just do the whole json encoding thing itself so that the depth is accessible.

- Some other way that I didn't think of?
Sorry, only registered users may post in this forum.

Click here to login