feat: remove unnecessary $isUncaught param

[bishopb]

Description of the change

When the application logging handles an otherwise uncaught error, exception, or fatality, the logging path enters the "uncaught" flow. The implementation for the uncaught flow went through the usual logging mechanism (PSR-3, log()) but appended the uncaught state value to the log() signature.

While not a violation of PHP LSP rules, it nonetheless could be an error when Rollbar is used as the handler, but a user-defined logger with a different log() API is shimmed in-front of the Rollbar logger.

For broadest compatibility, and keeping with the principal of least surprise, we remove that extra parameter and instead pass the state information as part of the exception object itself.

BREAKING CHANGE

Type of change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Related issues

• Fixes Add isUncaught to context array, remove fourth parameter to log in RollbarLogger #499

Checklists

Development

• Lint rules pass locally
• The code changed/added as part of this pull request has been covered with tests
• All tests related to the changed code pass in development

Code review

• This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
• "Ready for review" label attached to the PR and reviewers mentioned in a comment
• Changes have been reviewed by at least one other engineer
• Issue from task tracker has a link to this pull request

[bishopb]

I'm on the fence about this method.

It makes for an easy upgrade path: code doing Rollbar::log($level, $data, $context, true) just gets rewritten to Rollbar::logUncaught($level, $data, $context), provided that the $data is an instance of Throwable (which it must be, since the only "uncaught" things derive logically from Throwable). However, if no-one is using Rollbar::log with the fourth parameter, then this is just code bloat.

Without metrics, I'm erring on the side of caution and providing an upgrade path for the breaking change.

[waltjones]

There was at least one person who intended to use it 3 years ago. #314

In general the SDKs allow specifying "uncaught", and it's a common pattern to use this flag in a check ignore handler, if not elsewhere. It makes sense to continue to provide a way to set it.

I'd agree this should only apply to Throwable. Hopefully no one had been using it differently.

[waltjones]

Looks good, on the basis that this is treated as a breaking change.

[jamesaspence]

@bishopb I didn't notice this PR until now (10 days too late!), but the one comment I'll have is that we're adding a new property, isUncaught, to ErrorWrapper but not actually defining the property anywhere. Does it make sense to create a dedicated exception decorator (UncaughtExceptionDecorator) that has isUncaught defined and a way to retrieve the base exception, so you can document the property and how it's meant to work? I guess it'd look something like this:

<?php

namespace Rollbar;

use Throwable;

class UncaughtExceptionDecorator
{
    private $isUncaught;
    /**
     * @var Throwable
     */
    private $exception;

    public function __construct(Throwable $exception, bool $isUncaught = true)
    {
        $this->exception = $exception;
        $this->isUncaught = $isUncaught;
    }

    public function getWrappedException(): Throwable
    {
        return $this->exception;
    }

    public function isUncaught(): bool
    {
        return $this->isUncaught;
    }
}

[bishopb]

we're adding a new property, isUncaught, to ErrorWrapper but not actually defining the property anywhere... [should we]
create a dedicated exception decorator (UncaughtExceptionDecorator) that has isUncaught defined

@jamesaspence True! It's kind of gross as implemented, but I did consider the decorator.

I decided against it for two reasons. First, concision: this property is an internal detail, whose line-of-code existence is very short and well-defined. Where it touches the user, it's by a well-documented API (Rollbar::logUncaught). Second, safety: the implementation to log the actual exception would have been complicated with a wrap/unwrap dance, and if the wrapped version somehow found its way to the log, it would've masked the true exception.

Your proposed implementation looks good, and we're open to all changes that improve the code, so feel free to carry it forward if you'd like.