Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » Exposing jsonSerialize's depth »

Exposing jsonSerialize's depth

php.internals

Jani Ollikainen

9 years ago
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

Jakub Zelenka

9 years ago
On Wed, Jan 11, 2017 at 8:06 AM, Jani Ollikainen <jani.ollikainen@valve.fi> 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

Jani Ollikainen

9 years ago
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.

Niklas Keller

9 years ago
2017-01-16 9:19 GMT+01:00 Jani Ollikainen <jani.ollikainen@valve.fi>:
> 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

Jani Ollikainen

9 years ago
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

9 years ago
> > >> 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

9 years ago
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?

Jakub Zelenka

9 years ago
On Mon, Jan 16, 2017 at 8:19 AM, Jani Ollikainen <jani.ollikainen@valve.fi> 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

Stas Malyshev

9 years ago
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

Jani Ollikainen

9 years ago
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.