Added checkIgnore config option. (#45)

[bytestream]

Copied over the checkIgnore functionality from https://github.com/rollbar/rollbar.js/blob/a072cb78a9056ebded63879e6eec473d6ff1752d/release/rollbar-1.8.5.js

[nocive]

👍

[Crisfole]

Hm. So RollbarException seems to be duplicating a lot of stuff that's already present in $payload. Is there a notable difference?

[bytestream]

I literally just copied the functionality and tried to ensure it was consistent with the JS version. I agree it doesn't really make sense; I wasn't sure how to handle the two cases where there is not an exception:

• Logging a message
• Logging a PHP error

[Crisfole]

On a more general level, would this function allows users to tweak the payload before it gets sent?

It's not necessarily a bad thing, but mutating can have annoying side effects. Is there an easy, fast deep clone mechanism in PHP? That could make this 'pure' (apart from its arguments) by passing a cloned version of the $payload to the checkIgnore function.

Note: this is why there's a separate transform config function in Rollbar.js.

[bytestream]

At the moment they can't tweak the payload no.

[Crisfole]

@bytestream what prevents them from tweaking it? Couldn't I just say $payload["data"] = "LOOKIE HERE!!!" in my callback? (Here I may be revealing my PHP ignorance).

[bytestream]

@Crisfole PHP passes function args by value not by reference :) http:https://php.net/manual/en/functions.arguments.php

[Crisfole]

Right, but the 'value' of most object types (arrays and maps included) in most languages is simply the reference to the object. Does PHP not work this way?

[nocive]

@Crisfole only for objects, otherwise you have to explicitly say you want to use them by reference with &
Arrays and all scalars are not considered objects and as such are not taken by reference by default

[Crisfole]

👍 I'd say this is close to good to go once we make sure that the Exception, Error data, and message are availabe in the second argument in some way shape or form. (From IRC channel)

[bytestream]

@Crisfole done ^

[Crisfole]

👍 That's great! I didn't know ErrorException existed. That's a nice solution.

[Crisfole]

K, last thought is that we probably don't need the new RollbarException($e->getMessage(), $e) (the getMessage portion in particular.

If people want to call getMessage they can be our guest but they may not need it, and it makes it just the tiniest bit harder to tell if a RollbarException is for a report_message or report_exception.

It might be nice to have a third parameter for ErrorException instead of using a single exception parameter. It might change how you want to handle the ignoring.

Thoguhts?

[bytestream]

You can differentiate between a message and an exception by just checking if getException() returns null. Changing it to how you suggest is really just the same thing, you still have to check which params are set. This is how you would do it at the moment:

public function ignore($isUncaught, RollbarException $exception, $payload)
{
    if (is_null($exception)) {
        // we have a message
    } else {
        if ($exception instanceof ErrorException) {
            // we have a php error
        } else {
            // we have a normal error
        }
    }
}

The OO way is more long winded, arguably nicer, but would be this (untested):

interface RollbarException {
    public function getMessage();
}

class RollbarPhpError extends ErrorException implements RollbarException
{
    //
}

class RollbarMessage implements RollbarException
{
    public function getMessage()
    {
        //
    }
}

class RollbarUncaughtException extends Exception implements RollbarException
{
    //
}

public function ignore($isUncaught, RollbarException $exception, $payload)
{
    if ($exception instanceof RollbarPhpError) {
        // 
    } elseif ($exception instanceof RollbarMessage) {
        //
    } elseif ($exception instanceof RollbarUncaughtException) {
        //
    } else {
        // getMessage()
    }
}

[Crisfole]

I think I'm good with it the way it is, then. @brianr what do you think?

@bytestream Once we get a second pair of eyes on this we can merge and up the version number.

[dlsteuer]

Looks good to me, merging in.