Report fatal errors potentially caused by Site Kit if opted in to event tracking

[felixarntz]

In order to be aware of any fatal errors related to Site Kit that happen in the wild (e.g. to more quickly and proactively discover something like #3830 in the future), we should collect fatal errors for those Site Kit users that have opted in to tracking. This will bring parity with what we already have in JS, which has helped us identify and fix breaking JS errors in the wild.

Catching and recording PHP errors is slightly more complex, but in a way even more crucial than JS errors as in the worst case it can bring down the entire site.

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• Site Kit should track PHP fatal errors in the following circumstances:
  • The current user has opted into tracking.
  • The fatal error has at least one Site Kit file in its stack trace.
• Every such fatal error should be sent to Analytics as follows:
  • Event category: php_error
  • Event name: TBD
  • Event label: error message, followed by the list of file and line number pairs from the stack trace (in a comparable format to how react_error events are sent)
• In order to catch fatal errors, Site Kit should, as early as the plugin code is loaded, integrate with WordPress core's fatal error detection mechanism:
  • It should use the wp_should_handle_php_error filter to get the error, passing through the filter value. The error should only be handled if WordPress considers it a fatal error (i.e. filter value is true) and if the above conditions are satisfied.
  • TBD

Implementation Brief

Test Coverage

Visual Regression Changes

QA Brief

Changelog entry

[felixarntz]

@aaemnnosttv @tofumatt @eugene-manuilov Let's collaborate to define the rest of the ACs here. It goes a little bit towards IB, but we should still specify as part of the ACs how to actually get the event to Analytics (storing and sending later? sending immediately?).

Also, any thoughts on a meaningful event name? We use the context for React errors, but not sure whether there's an equivalent that would be valuable on the PHP side.

[felixarntz]

FYI @bethanylang It might take us a bit to figure out a solid approach here, but we're working on a way to identify fatal errors hopefully before people need to report them to us.

[eugene-manuilov]

how to actually get the event to Analytics (storing and sending later? sending immediately?).

I think we need to send it immediately because if the fatal error happens during the plugin activation flow, we will not have a "later" chance to send it. Another question is how to do it... I think there is no legit way to send a custom event using Google SDK, right? I have seen that people use different hacks like this one to send custom events, but it doesn't look reliable to me :thinking:

[eugene-manuilov]

Actually, after thinking a little bit more, I think we can't track fatal errors at the plugin activation stage anyway because we haven't got consent for tracking from the user yet.

[eugene-manuilov]

It should use the wp_should_handle_php_error filter to get the error, passing through the filter value. The error should only be handled if WordPress considers it a fatal error (i.e. filter value is true) and if the above conditions are satisfied.

Unfortunately, the wp_should_handle_php_error filter is not applied for E_ERROR, E_PARSE, E_USER_ERROR, E_COMPILE_ERROR, and E_RECOVERABLE_ERROR error types, thus we won't be able to track these errors if we rely on this filter.

protected function should_handle_error( $error ) {
    $error_types_to_handle = array(
        E_ERROR,
        E_PARSE,
        E_USER_ERROR,
        E_COMPILE_ERROR,
        E_RECOVERABLE_ERROR,
    );
 
    if ( isset( $error['type'] ) && in_array( $error['type'], $error_types_to_handle, true ) ) {
        return true;
    }
 
    /**
     * Filters whether a given thrown error should be handled by the fatal error handler.
     *
     * This filter is only fired if the error is not already configured to be handled by WordPress core. As such,
     * it exclusively allows adding further rules for which errors should be handled, but not removing existing
     * ones.
     *
     * @since 5.2.0
     *
     * @param bool  $should_handle_error Whether the error should be handled by the fatal error handler.
     * @param array $error               Error information retrieved from error_get_last().
     */
    return (bool) apply_filters( 'wp_should_handle_php_error', false, $error );
}

https://core.trac.wordpress.org/browser/tags/5.8/src/wp-includes/class-wp-fatal-error-handler.php#L96

[felixarntz]

@eugene-manuilov

I think we need to send it immediately because if the fatal error happens during the plugin activation flow, we will not have a "later" chance to send it.

That's probably correct and make sense anyway, so let's work towards that. In certain cases it's probably still possible to store something in the DB and then send later, but sending it immediately should be more reliable.

I have seen that people use different hacks like this one to send custom events, but it doesn't look reliable to me 🤔

I mean technically the JS client also makes a POST request with that data right? We should look into any public documentation on how to actually do this, but the approach that post is describing generally makes sense to me.

Actually, after thinking a little bit more, I think we can't track fatal errors at the plugin activation stage anyway because we haven't got consent for tracking from the user yet.

It's not only about activation errors, we want to track fatal errors in general. Of course we can't track them if the user hasn't granted consent (yet). I think we should do the following:

• Check the option as soon as Site Kit loads.
• If opted in, hook in / initialize the logic which will take care of reporting any potential fatal errors. In other words, we definitely should not only check for the opt-in when a fatal error occurs, as that would be unreliable due to a potential DB query.