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

Serialize arrays in a readable format instead of just counting the items. #697

Closed
wants to merge 4 commits into from
Closed

Conversation

ArSn
Copy link

@ArSn ArSn commented Nov 16, 2018

We do currently have the problem that we are getting an exception where we can not really reproduce the issue but some of our users are running into the issue. Even though we are able to fail gracefully here, we'd like to resolve the issue.

However, the raven client for PHP serializes arrays in a not very helpful way by just counting the array items, which leads to event data like this:

With the proposed changes this changes to (amongst other points where arrays show up):

Which is evidently much more helpful as we can see the user input and reproduce the problem now.

Would be awesome if we could put that into one of the next releases for the official client so we can switch back to that from our forked one.

I adjusted the tests accordingly, let me know if I should change anything else and keep up the good work!

@ArSn
Copy link
Author

ArSn commented Nov 16, 2018

I ran all the things on PHP 7 and it worked fine, as it does in travis. However I see all the failing tests, it appears to me though that the dependencies do not even resolve in e.g. a 5.6 like scrutinizer is running:

root@cdc95543d5c2:/app# php composer.phar install
Do not run Composer as root/super user! See https://getcomposer.org/root for details
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Installation request for doctrine/instantiator 1.1.0 -> satisfiable by doctrine/instantiator[1.1.0].
    - doctrine/instantiator 1.1.0 requires php ^7.1 -> your PHP version (5.6.38) does not satisfy that requirement.
  Problem 2
    - Installation request for myclabs/deep-copy 1.8.1 -> satisfiable by myclabs/deep-copy[1.8.1].
    - myclabs/deep-copy 1.8.1 requires php ^7.1 -> your PHP version (5.6.38) does not satisfy that requirement.
  Problem 3
    - Installation request for phpdocumentor/reflection-docblock 4.3.0 -> satisfiable by phpdocumentor/reflection-docblock[4.3.0].
    - phpdocumentor/reflection-docblock 4.3.0 requires php ^7.0 -> your PHP version (5.6.38) does not satisfy that requirement.
  Problem 4
    - Installation request for phpunit/php-token-stream 2.0.2 -> satisfiable by phpunit/php-token-stream[2.0.2].
    - phpunit/php-token-stream 2.0.2 requires php ^7.0 -> your PHP version (5.6.38) does not satisfy that requirement.
  Problem 5
    - Installation request for symfony/debug v4.1.7 -> satisfiable by symfony/debug[v4.1.7].
    - symfony/debug v4.1.7 requires php ^7.1.3 -> your PHP version (5.6.38) does not satisfy that requirement.
  Problem 6
    - Installation request for symfony/yaml v4.1.7 -> satisfiable by symfony/yaml[v4.1.7].
    - symfony/yaml v4.1.7 requires php ^7.1.3 -> your PHP version (5.6.38) does not satisfy that requirement.
  Problem 7
    - doctrine/instantiator 1.1.0 requires php ^7.1 -> your PHP version (5.6.38) does not satisfy that requirement.
    - phpunit/phpunit-mock-objects 3.4.4 requires doctrine/instantiator ^1.0.2 -> satisfiable by doctrine/instantiator[1.1.0].
    - Installation request for phpunit/phpunit-mock-objects 3.4.4 -> satisfiable by phpunit/phpunit-mock-objects[3.4.4].

How shall we handle this?

@Jean85
Copy link
Contributor

Jean85 commented Nov 16, 2018

There are already numerous attempts at refactoring this issue. We currently are working on the 2.0 branch for a new release, and we would like to address this issue thoroughly.

This approach seems dangerous and not practical to me, since it doesn't take into account the fact that an array can contain any number of things, especially objects, so this PR doesn't seem feasible to me.

@ArSn
Copy link
Author

ArSn commented Nov 16, 2018

@Jean85 You have a valid point, this is however the reason that I am using print_r and not var_export so one does not run into problems with circular references. I appreciate that object content may be serialized here as well, but how would this be a problem? Is your point that the request might be too big?

Regardless, do you have any advice on how we could analyze our issue differently in the meantime before 2.0 is finished?

@Jean85
Copy link
Contributor

Jean85 commented Nov 16, 2018

The first thing that you can track down is which version of the client are you using, since #554 should have reduced this issue a bit. For anything else, there's #632 which would make the limit customizable.

@ArSn
Copy link
Author

ArSn commented Nov 19, 2018

We were in fact using an old version and the level was just cut off too early. Bummer I did not see this before trying to cook my own soup. Well, you never stop learning I guess.

Thanks for the hint @Jean85 - I'm closing this PR as I agree it is nonsense then.

@ArSn ArSn closed this Nov 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants