Improve initialization crash message

[artfulrobot]

Overview

If there's an initialization crash, Civi will bring the whole site down and output an error message. This is rare, but a lot of eyes see the error on a busy website if it does happen.

Before

The output included dumped variables and error messages.

After

The output is simply an error reference which can be looked up wherever error_log outputs.

Technical Details

This changes the behaviour of CRM_Core_Error::debug() which is deprecated anyway.

It's really about avoiding public vardumps rather than a sensible logging improvement.

Using error_log because we might not have Civi::log() available in these weird/bad/early crashes.

[civibot]

Thank you for contributing to CiviCRM! ❤️ We will need to test and review the PR. 👷

Introduction for new contributors

• If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
• A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
• A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
• If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
• Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
• A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
• To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
• For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers

• ➡️ Demo site for this PR
• 📖 Review standards
• 🗒️ Review template (brief or verbose)

[artfulrobot]

Sorry only meant to include the one commit . will remove the other tomorrow

[eileenmcnaughton]

As a deprecated function this is probably safe but giving merge-ready in case @totten has thoughts

[totten]

Yeah, I've read this a couple times -- and on the surface, it looked good. But when I tried to grok the overall effect (to figure out if it might change some other use-case in a problematic way), then my brain exploded. This is not because of the patch itself; rather, the surrounding context is messy.

(Aside: I agree that debug() should be regarded as deprecated, but fwiw it doesn't have the annotation.)

OK, so trying again to analyze it... I suppose that CRM_Core_Error::debug() is used to mean three different things:

1. debug() is a formatting function ("Given value X, format it as a string Y"). There is only one code-path which does this. It looks like an important one, but it's only one.
   • The patch does not affect this usage.
2. debug() is an unguarded print statement ("Given value X, send it to the web-user"). It is only used this way one time (via simpleHandler).
   • I believe this was the use-case which motivated the patch. This scenario seems bad.
3. debug() is a permission-guarded print statement ("Given value X, send it to the web-user, but only if they permission"). This is the default mode.
   • In a "Find Usages" analysis, almost all callers use it this way. This includes ~10 direct calls in core and ~15 direct calls in universe.
   • This is where it gets messy.
   • I spot-checked a few of core's direct calls, and it's been very tricky to run them in an organic way. The first few I tried seemd to get cut-off by higher-level factors. But the code is still there (probably out of concern that they might be used in some hard-to-see way).
   • For most of these, I think the original/pre-patch behavior (enforce the permission-guard; print if it passes) would be fine/better.
   • Take CRM_Core_DAO::debugPrint() as an example. This is never called. Presumably, it's a utility that someone hacks-in if they want to print extra info. The name suggests it will print debug info. But with this patch, it prints a placeholder message. In the fact, the permission view debug output becomes kinda meaningless with this patch, doesn't it? (The permission no longer gives you access to debug output. It gives you access to a reference-code that you can check in the log. You can safely give that reference code to anyone.)

If the goal is "fidelity to CRM_Core_Error::debug()", then maybe the better approach is like:

• If the permission subsystem is not available, then print a reference-code.
• If the user has permission, then show debug info.
• If the user lacks permission, then... show the reference-code? Or show nothing? (I think "Show nothing" is more consistent with how the permission view debug output has been understood. But sometimes a reference-code might be safe+useful. It's just PITA to track down the usages and evaluate them.)

OTOH, I'm not sure that "fidelity to CRM_Core_Error::debug()" is important... when what I really want is to finish switching it over to PSR-3... but that's a different scope for a patch...

Recapping... in terms of contracts, it's maybe a bit regressive. But in terms of user-impacts, does it matter? Eh...

[artfulrobot]

Great work @totten!

See what you think of this.

Improvements, I think are:

• without permissions system we don't output internal gubbins to the world. (original purpose of this PR). Just a reference, as you suggested.
• we never pass HTML content to error_log().
• without permission: you see nothing, but in a slight nod to your PITA, if HTML is requested then it spits the reference in a <!-- comment --> (though you'd have to know to look for it...)
• with permission: should behave as before.

I agree that this is a messy function. e.g. I have assumed that $html means you want HTML output, which is different to just whether htmlspecialchars should be applied to $variable.

[totten]

@artfulrobot Yeah, I think that updates makes more sense.

I played around with it a bit... and while the overall murkiness of error-handling makes it hard to be sure, I think the patch had the appropriate affects.

Test failure is unrelated/temporal.