Fix handling of invalid sql query during import

[eileenmcnaughton]

Overview

Fix handling of invalid sql query during import

To replicate this be sure to log in as admin rather than demo

Attempt an import with an invalid sql query

Before

After

Page reloads as

Technical Details

UPDATE - the exception stuff is being moved to a separate PR

The main thing going on in this PR is trying to think about what is going on with Exceptions. My general thoughts on Exceptions is that any anticipated Exception that arises from within CiviCRM that is handled in a different subsystem than it was generated in should be a version of CRM_Core_Exception. This would mean that all CiviCRM code or DB errors arising from CiviCRM sql calls would be caught by catching CRM_Core_Exception. From there we can have more nuanced exceptions.

This is not currently the case. Any code that runs CRM_Core_DAO::executeQuery() indirectly or directly may have to deal with an uncaught PEAR_Exception.

This PR doesn't change that but starts to propose what an alternative that implements some / most of what PEAR_Exception provides - ie

• a way to get the mysql error code getErrorCode
• a way to get the PEAR error code getPEARErrorCode
• a way to get a message suitable for display to users getUserMessage
• a way to get the erroneous sql
• a way to get the debug info

One thing it doesn't do is handle protecting the usefulness of the backtrace - I figured I'd worry about that if it became the blocker

A couple more things I realised in the process

• the API Kernel has helpers to get info out of the error object - I copied these in since getting db error out of exceptions is kinda tricky
• CRM_Core_Exception extends PEAR_Exception - I'm not a fan.

Comments

This is an alternative to #25171 - although I suspect it will stall so I'm going to put up the context part as a more limited patch to get that merged

[civibot]

(Standard links)

• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

test this please

fatal: unable to access 'https://lab.civicrm.org/extensions/angularprofiles.git/': Failed to connect to lab.civicrm.org port 443 after 130015 ms: Connection timed out

[eileenmcnaughton]

Note here is another case where code fell into the trap of not catching the PEAR_Exception #25633

[totten]

OK, the PR is doing a bit of trick - in its title, it approaches a narrow problem (the importer fails on SQL exception; go figure), but the discussion proposes a wide-ranging change (rework the class-hierarchy vis-a-vis exceptions and PEAR). I suspect you're looking for feedback on the latter.

My first reaction was: "That's crazy." But... the universe will guide us, and... it just might be doable.

• It should be crazy, generally speaking. This exception has been around forever. It's a very basic kind of problem. We see it all the time in backtraces. Surely, there are downstream commitments to PEAR_Exception, and changing it will cause a chaos from mismatched try/catches.
• But in the universe... I only found two pieces of live contrib code that refer to PEAR_Exception. Namely, Systopia's sqltasks and Veda's mailchimp. Additionally, Compucorp's membershipextras has some test-only references. Each reference is small. Fixing 2-3 extensions is not major lift.

Trying to rationalize this... I suppose there are a few reasons why downstream would not have taken to PEAR_Exception...

• Historically, PEAR didn't actually throw Exceptions. So there was no point.
• Most folks consume via APIv3/APIv4, and the kernel already coerces the exception to another format.
• SQL errors usually arise from malformed requests or bad mysqld circumstances, and there's not much point in catching either. The former should usu be displayed+fixed during dev/QA. The latter is usu non-recoverable. Displaying them makes it easier to fix. It's really only oddballs (like importer, sqltasks, db upgrader) that get value from catching them.

So on conceptual level, there should be path that makes it possible to replace PEAR_Exception.

I don't have my head fully around what the exception should look like, but I wonder if the framing in this branch might be a bit too narrow? Like... it means that most use-cases still throw the same kind of exception, but this one throws something different. Since there doesn't seem to be a huge systemic commitment to PEAR_Exception, perhaps we can find a simpler way to change it? IIRC, PEAR's error handling provides some access points that can give you more leverage over the range of errors/exceptions.

[eileenmcnaughton]

@totten one thing to bear in mind is that CRM_Core_Exception extends PEAR_Exception - so any Exception that extends CRM_Core_Exception will still be caught when catching PEAR_Exception & still give off those methods...

I think the obvious place that should through something that inhetirs CRM_Core_Exception AND by virute of that PEAR_Exception is CRM_Core_DAO::executeQuery

[eileenmcnaughton]

@totten Also - based on testing with #25633 and specifically by toggling the exception handling in the bit of code from SearchAdmin you reference - there are at least some cases functions within apiv4 bypass any exception coercion

[eileenmcnaughton]

@totten the last point is - yes - this branch only adds the new exception in one place - the focus being on defining what the new exception class might look like rather than the adding of it in - although as I say CRM_Core_DAO::executeQuery is the obvious place to take a PEAR_Exception and rethrow a CRM_Core_Exception_DBQueryInvalidException (or perhaps a Civi\Exception\DBQueryException

[totten]

@eileenmcnaughton I tried a SQL import with select * from foo, and it failed like this:

Changing getUserMessage() to getMessage() worked locally...

[eileenmcnaughton]

@totten ah yeah - that fell out when I split - OK - will move that part out of this

[eileenmcnaughton]

@totten ok fixed - I'll fix up the follow on PR once this is merged

[totten]

Confirmed it works better 👍

Merge on pass