local_vars_dump with recursive data structures causes infinite recursion

[jlesueur]

Unfortunately some of our parameters are recursive. Here is a super simplified example:

$obj = [
  'key1' => 'value',
  'key2' => 'value'
];

$obj['key3'] => &$obj;

This causes Utility::serialize($obj) to enter an infinite recursion. So if local_vars_dump is on, errors can cause infinite recursion.

I kind of think a simple fix is to implement a maximum depth for the serialize method. Maybe a configurable one, with a default of somewhere between 5 and 10? If that solution sounds ok, I'd be willing to put together a PR for it.

[jessewgibbs]

@jlesueur thanks for reporting this issue.

@ArturMoczulski what do you think about the proposal to set a maximum depth for the serialize() method? I imagine we'd want to have it be a configurable setting...

[coryvirok]

We also need the serialization code to detect loops. See the other sdks for a reference implementation.

[ArturMoczulski]

@jessewgibbs yeah I think that's good

@jlesueur if you want to prepare a PR that would be awesome

@coryvirok I'll check out other SDKs, although I believe introducing default max depth should solve the problem

[coryvirok]

It might solve it in terms of the specific bug but the serialization will be incorrect due to loops. Also, it will have another effect for nested objects of increasing the payload size.

…

On Fri, Sep 28, 2018, 12:33 PM Artur Moczulski ***@***.***> wrote: @jessewgibbs <https://github.com/jessewgibbs> yeah I think that's good @jlesueur <https://github.com/jlesueur> if you want to prepare a PR that would be awesome @coryvirok <https://github.com/coryvirok> I'll check out other SDKs, although I believe introducing default max depth should solve the problem — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#410 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAu9K_PxKDgcFV6-ZcAd2mZ4L7D-3kAvks5ufk9ZgaJpZM4W7iqC> .

[JohnKendhammer]

Issue has been resolved with PR #414