[2.0] Merge up to 1.9

[Jean85]

This is a continuation of the work of #526, continuosly porting all the new stuff from the master branch into the 2.0. This is the list of merged PRs on master that I've ported here (by merge order on master):

• Fix gzipCompress function name #543: docs fix
• Adding application/json input handling. #546: adding application/json input handling
• Use correct class name for default processor #547: docs fix bis
• fixing AnnotationException when Raven_Client appears in the stack trace of being serialized Exception #548: fix Client PHPDoc
• Added syntax highlighting to README.md #551: fix syntax highlighting in the README.md
• Add use statement for HttpException class #556: docs fix ter
• Change badges with badge poser. #557: use badge/poser in the README.md
• Cleanup the 'site' value if it's empty #555: Cleanup the 'site' value if it's empty site is a deprecated field, no longer present
• Runtime information #564: add runtime info already covered by [2.0] Adding the PHP version #562
• Update docs config #570: fix official docs definition
• Handle and add non default port to host (#558) #572: handle non-default host port
• Update changelog for version 1.8.4 #573: prepare 1.8.4 release
• Fix x-forwarded-proto detection #578: Fix x-forwarded-proto detection will be handled with [2.0] Drop the trust_x_forwarded_proto option #594
• Add excluded_exceptions that is compatible with 2.0 #583: exclude_exceptions with child types already handled in [2.0] Refactor how the events are captured and stored #487
• Allow $data['request'] to be 5 levels deep #554: allow serialization 5 levels deep
• Make it possible to overwrite message limit of 1024 #559: make string limit customizable
• Fix for exceptions throwing exceptions #587: avoid looping when exception throws exception
• Fix monolog handler not accepting Throwable #586: Fix monolog handler not accepting Throwable
• Update Serializer.php #553: fix serializer to account for non-UTF-8 chars
• Fixed undefined variable #588: fix for undefined variable not applicable, CurlHandler no longer present
• Releasenotes 1.9.0 #590: update for 1.9.0 release
• Prepare master for next release cycle #591: prepare 1.10.x release not applicable

I will handle those PHPT regression tests separately:

• Revert ignoring fatal errors on PHP 7+ #571: add regression tests for fatal handling
• Avoid double reporting due to ErrorExceptions #574: avoid double reporting
• Force event send after fatal error with curl_method async #575: force send with curl_method async
• Apply other fix for #391 #576: more regression PHPT tests and fixes
• Fix regression test #589: fix regression test

[Jean85]

@ste93cry while trying to port #555 I discovered that the site field got lost during the development of the 2.x. Is it a custom field that we are ditching since we already have the standard server_name?

[ste93cry]

It was removed because it's a deprecated field

[Jean85]

@ste93cry looking at #578 I discovered that the trust_x_forwarded_proto option is handled but never used... what should we do?

[ste93cry]

According to the security issues that could arise by using the X_FORWARDED_* headers I would remove the configuration option entirely and not take any action regarding support of this particular feature in the PHP SDK

[Jean85]

This is so funny! It's the Google-translated version of Stop hoping that your users will report bugs in russian

[stayallive]

I couldn't just add some random text, needs to make a little bit sense at least 😉

[Jean85]

This is no longer WIP, I will handle the PHPT tests separately, please review!

[ste93cry]

I know that this argument has already been discussed many times and widely, but I strongly disagree with gathering the POST data and deserialize it if it's JSON. Now that we have a well organized way to collect all information we and users need through middlewares everyone can easily add whatever they want to the event data, and the SDK specs also says:

This data should not be provided by default as it can get quite large

This is the main reason I did not collect POST data by default in the RequestInterfaceMiddleware middleware. If we really want to keep going this way, we should at least use the PSR-7 methods to get the data instead of using file_get_contents directly.

[stayallive]

It's a large PR so I skimmed over it but looks like it's all the changes from 1.9.0 and in the right places, so 👍

[Jean85]

This data should not be provided by default as it can get quite large

Oh well, then I think we should create a separated middleware to do that!

This is the main reason I did not collect POST data by default in the RequestInterfaceMiddleware middleware. If we really want to keep going this way, we should at least use the PSR-7 methods to get the data instead of using file_get_contents directly.

What's the correct method to do it?

[ste93cry]

I think you should use the getBody() method of the ServerRequestInterface interface

[Jean85]

Ok I've reverted the commit about #546, I will handle it in a separate PR where I will create a separated middleware to handle POST body, with JSON handling too.

[ste93cry]

#587 should not apply to 2.x as we don't set anymore the event_id property

[Jean85]

#587 should not apply to 2.x as we don't set anymore the event_id property

Yep, but I've preferred to port the regression test anyway, so we will never have issues like that one in the future, for whatever reason.

[ste93cry]

I can't see where the event_id property is set in the test, maybe you forgot it?

[Jean85]

The event_id was set in the code of 1.x. The core of the issue is an exception that throws an other exception on __set, which would be triggered in a number of ways; in 1.x was the event_id.

[ste93cry]

What I meant is that I don't see anything in the test that tries to set the event_id property on the exception object (which in turn should throw another exception), so how can the test work? By the way, are you sure that the same case (throwing an exception while already handling one) is not covered by my PR that refactors the error handler?

[Jean85]

It's possible that is already covered by your PR, yes.