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
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=72073For 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
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.
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
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.
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.
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/L1u7qAnd 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
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?
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
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
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.