Fixed recursion on payload too large

[woutersamaey]

Goal

Fixed the endless recursion when the payload to submit is too large. The PHP max function nesting will be reached.

Review

For the submitter, initial self-review:

• Commented on code changes inline explain the reasoning behind the approach
• Reviewed the test cases added for completeness and possible points for discussion
• A changelog entry was added for the goal of this pull request
• Check the scope of the changeset - is everything in the diff required for the pull request?
• This pull request is ready for:
  • Initial review of the intended approach, not yet feature complete
  • Structural review of the classes, functions, and properties modified
  • Final review

For the pull request reviewer(s), this changeset has been reviewed for:

• Consistency across platforms for structures or concepts added or modified
• Consistency between the changeset and the goal stated above
• Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
• Usage friction - is the proposed change in usage cumbersome or complicated?
• Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
• Concurrency concerns - if components are accessed asynchronously, what issues will arise
• Thoroughness of added tests and any missing edge cases
• Idiomatic use of the language

[Cawllec]

Hi @woutersamaey, it seems this would interfere with our retrying logic that would generally trigger to split up large payloads. While I understand having a singular payload event that's too large could be problematic, and will recommend a fix for that to our products team, I don't think this approach will be usable.

If you could describe the particular issues that led you to create this PR, that would help us narrow down and approach and create a fix much more easily.

[woutersamaey]

We had an exception with a huge message that could not be sent to Bugsnag.
This made debugging the issue on production very hard.

The idea is that if bugsnag can't take the exception, then at least it should help me out instead of failing with another error, masking the original.

The underlying issue was easy to fix for me, but because bugsnag could not handle it, it became a huge thing. So that's why I wrote the message out.

[Cawllec]

I agree, that doesn't sounds ideal at all. I think adding a failure condition with full logs would be a practical way to approach this as suggested, and I'll suggest we prioritise this to get a fix as soon as we can.

[woutersamaey]

@Cawllec What about creating a fallback LoggerInterface where we can attach our own logic to, so that will be used in case Bugsnag can't handle it?

I'd be able to implement per-application logic and log the error to the regular application error log, which did not happen before as it could not get to that point.

[GrahamCampbell]

What about creating a fallback LoggerInterface where we can attach our own logic to, so that will be used in case Bugsnag can't handle it?

For now, I think it's best to do what the library already does, which is to fallback to php's own global logging functions. Changing how the library deals with internal exceptions is a separate discussion to be had I think, if we want to change that. One of the reasons for the current design is to prevent the possibility non-termination due to bugsnag being the psr logger itself.

[GrahamCampbell]

For now, I think it's best to do what the library already does

Ah, looking at your changes, I see that's what you've done. Perfect! :)

[woutersamaey]

Huh?