WordPress - Display site-theme/decorations on error screens

[braders]

Overview

This builds on the previous work completed by @seamuslee001 in #20206, but addresses some concerns with the original approach.

This new approach keeps the use of CRM_Core_Exception, ensuring that existing logging and hooks are not lost. As WordPress does not have a generic theme() function to theme a generic error message we need to ensure that the execution does not exit to allow the error message to be dropped into the page content - demonstated with a couple of default themes:

Before

Previously a completely unbranded, and minimally styled error page was used:

After

The error message is embedded in the post content, as demonstrated in the screnshots above.

Technical Details

There may be some (legitimate) concerns about not calling exit after an error, which is fair - consideration should be given to whether I'm missing anything when reviewing this.

I did wonder if the $civicrm_wp_title variable should be set within the CRM_Core_Error method to avoid leaking information when exceptions happen which don't pass through the permissionDenied method. Ultimately I decided this probably wasn't a big risk, but it would be good to check no one else disagrees here.

This can be tested in two ways:

• Manually trigger an exception on the page you are viewing by adding throw new CRM_Core_Exception('This is a test exception') to the relevant form. (unless you know of a specific route which causes a legitimate exception).
• Visit a page you don't have permissions to view in order to trigger the permissionDenied method.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[christianwach]

This looks much better than previous solutions 👍

@braders Quick question (I haven't time today to r-run) I assume the backtrace isn't shown when:

define('WP_DEBUG_DISPLAY', false);

[braders]

This looks much better than previous solutions 👍

@braders Quick question (I haven't time today to r-run) I assume the backtrace isn't shown when:

define('WP_DEBUG_DISPLAY', false);

I'd need to review what Civi does in some more detail to be certain, but I think the display (or not) of the backtrace would be completely down to whether Civi is configured to display debug information - I'm fairly sure there is a system settings page where is is set. I don't think CiviCRM reads WP_DEBUG_DISPLAY by default, and I've not added any logic to read this constant.

[totten]

That does look like a nicer error screen!

Here are some random thoughts. These are not all blockers - just general feedback/observations/suggestions.

• Naming:
  • CRM_Utils_System::exitAfterFatal(); returns a boolean to describe a policy -- it should have a name that sounds like a boolean or policy. (Ex: isExitRequired() or doesFatalRequireExit()).
  • CRM_Core_Error::abend(...bool $exit) - The function is called "end" (as in "abnormal end of execution"; as in "the system is crashing"). bool $exit reads as a little redundant. (As in: "Do you really want the function to do what it says?")
• Mark functions as @internal: Support for CRM_Utils_System::* ranges widely - some functions are "absolutely critical, public-facing APIs that could only break with major Civi v6.0 bump"; others are "totally quirky, internal-only, could change next month". The @internal annotation would remove ambiguity and make it easier/safer for future iterations.
• Target Environment: I feel a little ambivalent about targeting by CMS/UF (WordPress).
  • The code-paths around page-rendering and around errors can be sometimes weird and sometimes CMS-dependent. r-runing is complicated, and focusing on one CMS keeps the scope-of-work narrower.
  • If you talked to a dozen site-builders about error-handling behaviors, I bet the desired behaviors would not fall neatly along CMS lines
• Root Problem: The root problem is that the contract around invoke()/runItem()/throw $exception/PEAR_Error/fatal() feels a bit like a porcupine. The ideal is to grok+improve that contract. (Though it may be a heavy lift.)
• Test Coverage: E2E test-coverage would be great. Maybe something like:

  class E2E_Core_ErrorReportingTest extends CiviEndToEndTestCase {
    use Civi\Test\HttpTestTrait;
    public function testErrorChrome() {
      if (CIVICRM_UF !== 'WordPress') $this->markTestIncomplete('Not yet adapted to Drupal/Joomla/etc');
  
      $http = $this->createGuzzle([
        'authx_user' => $GLOBALS['_CV']['ADMIN_USER'], // Send HTTP requests as admin
        'http_errors' => FALSE, // We're expecting errors. It's easier to inspect if Guzzle doesn't its own exceptions.
      ]);
      $response = $http->get('frontend://civicrm/some/page?with=baddata&to=provokeError');
      // Assert that $response has HTTP code 404 or 500 or somesuch.
      // Assert that $response has some error message
      // Assert that $response has some kind of site-specific chrome
    }
  
    // NOTE: May need to borrow E2E_Extern_AuthxRestTest::setUpBeforeClass
  }

[braders]

@totten Thanks for the feedback. My latest push addresses:

• Naming
• Mark functions as @internal
• I also realised that following this change, the HTTP 500 code was not being used. That has also been addressed.

[demeritcowboy]

jenkins retest this please. Known intermittent timestamp-rollover fail, which is touchy to fix because was part of a security release.

CRM_Core_BAO_CustomFieldTest::testFileDisplayValueNoDescription
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'              <a href="/index.php?q=civicrm/file&amp;reset=1&amp;id=1&amp;eid=3&amp;fcs=51f30f984798b52fad8a796708bd26e8fe38ced0434f743c67369d07ec3db20d_1648379262_168" title="test_file.txt">\n
+'              <a href="/index.php?q=civicrm/file&amp;reset=1&amp;id=1&amp;eid=3&amp;fcs=51f30f984798b52fad8a796708bd26e8fe38ced0434f743c67369d07ec3db20d_1648379263_168" title="test_file.txt">\n

[eileenmcnaughton]

@seamuslee001 @christianwach @totten - is this mergeable now?

[kcristiano]

@christianwach The debug display is controlled by CiviCRM's debug handling. I've done some r-run on this and it looks good to me. I am OK to merge.

[eileenmcnaughton]

I've added merge-ready based on @kcristiano ^^ - if we don't get further feedback we can merge

[totten]

Yeah, I think the updates from @braders look like a good improvement. 👍

I've written an end-to-end test to check the quality of error screens across different environments (#23257).

(@braders) I also realised that following this change, the HTTP 500 code was not being used. That has also been addressed.

Something seems off:

• ⚠️ In my testing of a CRM_Core_Error::fatal() and throw new \Exception() on WordPress (with or without the current PR), it incorrectly returns HTTP 200. (You would expect HTTP 500.)
• ✔️ The PR title references the "permission denied" scenario - for that scenario, you'd expect HTTP 403. In my testing, it does give HTTP 403. (So sometimes it does it does set an HTTP status-code.)

But I think the status-code is maybe tangential:

• The focus of the PR description seems to be more about showing the site's chrome/decorations around the error. I confirmed that my WP site displays the chrome/decorations. Tested via r-run and testErrorChrome.
• The status-code seems to be a secondary thought in the PR. In my testing (testErrorStatus), it is neither better nor worse.

I think it would be great to also resolve discrepancies in the error-status (and remove this opt-out), but that doesn't need to block improvements on the chrome/decoration.