Added typing / comments to the FilterInterface and removed the token argument

[danielmorell]

There are six interfaces for custom classes that may be passed into the configs to change how the Rollbar package works. They are:

1. DataBuilderInterface handled in Added required public methods to the DataBuilderInterface #586
2. FilterInterface handled in this PR
3. ResponseHandlerInterface
4. ScrubberInterface
5. SenderInterface
6. TransformerInterface

The others will be addressed in future PRs (coming soon).

Description of the change

Implementing the FilterInterface is the lesser used filtering alternative to the check_ignore function.

I removed the $accessToken argument from the FilterInterface::shouldSend() method. This does not seem necessary, or helpful. And passing any sort of secret around where not needed seems like a poor idea. Additionally, if the implementation desperately needs it, calling $payload->getAccessToken() will provide it from the Payload instance.

Design Question

The check_ignore function accepts a boolean $isUncaught argument that is used to designate the log item as an uncaught error or exception. FilterInterface::shouldSend() method could likely possibly benefit from this additional argument. Should I add it? Any input would be helpful.

If we did not add the $isUncaught argument to FilterInterface::shouldSend(), there is a simple workaround that would likely work...

class MyCustomFilter implements FilterInterface {
    private static bool $isUncaught;
    public static function checkIgnore(bool $isUncaught, string|Stringable $toLog, Payload $payload) {
        self::$isUncaught = $isUncaught;
        // Other logic
    }
    public function shouldSend(Payload $payload): bool {
        $isUncaught = self::$isUncaught
        // More logic
    }
}
Rollbar::init([
    'check_ignore' => MyCustomFilter::checkIgnore,
    'filter' => MyCustomFilter::class,
]);

I appreciate any input on this question.

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)
• Maintenance
• New release

Related issues

None

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 assigned
• Issue from task tracker has a link to this pull request
• Changes have been reviewed by at least one other engineer

[waltjones]

@danielmorell Thank you for the thoughtful PR and design notes.

Adding $isUncaught to the filter interface makes sense. I'm wondering though whether both interfaces are needed if they do the same thing. Or if either one can be improved to cover all the use cases of the other.

About the access token: I don't know how users of rollbar-php are using it, but in a multi-project runtime, it might be used to detect which project the message belongs to. Regardless, I agree $payload->getAccessToken() should be sufficient.

[danielmorell]

@waltjones I appreciate you questioning the need for the two redundant filtering mechanisms. Having thought about it, I think we should keep them both for now. Let me first explain the difference between the two, and then I will explain why I think this.

The check_ignore function

The check_ignore function is non stateful function that should look like the following...

function myCustonIgnore(bool $isUncaught, string|Stringable $toLog, Payload $payload): bool {
    // logic goes here...
}

Rollbar::init([
    'check_ignore' => myCustonIgnore,
]);

Alternatively, as pointed out above, it could be a static method in a class. This is much more probable in most instances where autoloading is being used. However, because PHP has good support for programs written in a procedural style, such as much of WordPress, it may also be common to see a standalone function being used.

PHP does not at this time support function typing beyond the callable type. This means that function arguments and return types cannot be type checked.

The check ignore function is documented and seems to be the most widely used filtering method.

The FilterInterface

On the other hand, a class implementing the FilterInterface can be both stateful and explicitly typed, however the 'filter' argument is currently undocumented.

For example...

class MyCustomFilter implements FilterInterface {
    private string $tenant;

    public function __construct(array $options) {
        $this->tenant= $options['tenant'] ?? 'none';
    }

    public function shouldSend(Payload $payload, bool $isUncaught): bool;
        // filtering logic...
    }
}

Rollbar::init([
    'filter' => MyCustomFilter::class,
    // The options are passed to the filter constructor
    'filterOptions' => [
        'tenant' => 'my_tenant',
    ],
]);

During the initialization the instance of MyCustomFilter is created with the $tenant property set to 'my_tenant'. Keeping track of state like this with the check_ignore function would be difficult.

Additionally, the shouldSend() method must comply with the interface. This helps keep return types and arguments in sync and reduces bugs.

Unfortunately, the filter config option is not in our documentation. And does not appear to be widely used.

My Opinion

The filter option is the better way of filtering. Unfortunately, it is the less used. We should document it and promote it as the default filtering mechanism.

Removing check_ignore would be a pretty serious breaking change. One that we would probably get a lot of backlash for removing. It may be worth deprecating and possibly removing in a future v5. This would give us plenty of time to encourage our users to implement move to filter.

A short note on multi-projectp runtimes

Thank you for pointing out the use case of access token being used to distinguish between projects in a multi-project runtime. This is not something I had considered, so thank you.

I spent a few minutes thinking about how to set up a multi-project runtime, and what I found might be worth noting. Because of the way the Rollbar class is it is not possible to have multiple access tokens in the same PHP process without throwing out the Rollbar class entirely. This is because the init method is static and the error handlers and logger are both private and static.

Calling Rollbar::init() will not create a new instance if one already exists but reconfigure the existing instance.

To get around this a user would need to create an alternative to the Rollbr class that could be initialized with each project as a separate instance.

Alternatively, Rollbar::init() could be called late enough in the process that the project is known, and it can be initialized with the project details. However, this puts us back in the position where some state is needed at that point in the process to identify the project, and that state would likely be available to the filtering function as well.

It may be worth considering how to make multi-project runtimes a little easier.

Sorry for the long-winded reply.

[waltjones]

@danielmorell Thank you for the detailed reply. No apology needed. :)

I think the reason for both the existence and the popularity of check_ignore has to do with it existing in nearly the same form in many of the other SDKs. (Even including the isUncaught flag.) Your comments about why the filter option is better make sense, and I agree it makes sense to keep both.

Thanks for the explanation on multi-project configurations. It sounds even more reasonable to remove the $accessToken argument.