Adding breadcrumbs to sanitize / serialize.

[ryantology]

Breadcrumbs are not being sanitized. When the inputs contain binary data (binary UUID for instance) the json_encode fails and no report is sent.

[Jean85]

Thanks for the contribution!

Have you tried this on a live setup?
Can you please add tests to avoid regressions (and help the porting of this onto the 2.x branch)?

[ryantology]

Yes, it has been tested in live setup. Using this fork in production now.

I had some questions when adding this code:

• Some of the sanitize data has a limit on the depth. Is there a reason for these limits? My breadcrumb arrays are only 3 levels deep but I don't have a ton of test data to know what other breadcrumbs may look like.
• I will add a test for the data that I have as an example. My use case was that there is binary UUID in sql breadcrumb that fails. Would a test that replicates the context test: https://github.com/getsentry/sentry-php/blob/master/test/Raven/Tests/ClientTest.php#L1255 be adequate for this code?

I didn't see the 2.x version! Will check on that and test this bug against the new branch.

[Jean85]

The 2.x branch is still in development, but we should be near to something usable as an alpha, probably!

As per the data limitation, the reason is that the Sentry server has some hard limits on that, see docs. Breadcrumbs shouldn't have those.

As for the test, yep, that case is good ti imitate.

[ryantology]

It would be ideal if these tests also tested to see if mb_convert_encoding($value, 'UTF-8'); is working properly. I'm not sure how to include binary data in the test.

[Jean85]

Maybe this could help? https://blog-en.openalfa.com/how-to-work-with-binary-data-in-php

Also, temporarely commenting your fix should make the test fail...

[ryantology]

The CI tests fail if there are PR comments? Thats kinda cool. I am going to add the binary check in the test and then rebase / squish and remove these comments.

[Jean85]

Nope, I meant that if you locally comment the piece of code that you applied as a fix, your test has to fail; it's just a practical test to see if your test is properly written and does his job correctly.

[ryantology]

Unrelated, but would love to see styleCI added as a CI here to enforce PSR formatting and coding standards. It looks like 2.x dropped the old array() syntax <3

[ryantology]

I think travis is failing because the encoding might be different for some php versions?
https://github.com/getsentry/sentry-php/blob/master/lib/Raven/Serializer.php#L89

My tests locally pass and the new code creates a passing test. Not sure how to proceed. The serializer property on client is protected making it hard to use the build in encoding detection.

[Jean85]

As for CS, 2.x is under PHPCSFixer and it's checked in the Travis build, so we're covered.

For the build failures, it seems that there are some differences for PHP <5.6. Maybe you should try to use some different bytes?

[ryantology]

I found a different binary data that seems to work! Pretty random but does work. Squishing commits now.