Fix loop

[eileenmcnaughton]

Overview

It turns out that if you 'break' advanced search (in my case
I applied a patch that caused invalid sql) it enters a loop.

I really can't see why 'while' would have ever made sense here.

There is nothing to cause it to 'move along'

Before

Loop

After

error

Technical Details

Comments

@demeritcowboy I got a loop testing #19619 - wondering if you did or if not what is different

[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).

[demeritcowboy]

I don't remember but it looks like it logs it to a file before hitting this point so I probably just looked in the file and didn't think more about it.

The loop is probably there because of nested exceptions, so the loop would allow outputting the nested messages. So maybe rather than change the loop to an if, break out if it's a DB_Error? Or yeah maybe just the first one is fine since I suppose you could have infinite nesting, which would then need a depth limit.

[eileenmcnaughton]

test this please

[demeritcowboy]

Tested and works ok.
One thing to note is this only comes up if you have civi debugging settings set to show a backtrace on screen.

And another way to reproduce this without faking a query error is go to the case dashboard and sort the upcoming list by type. There's already a ticket about the underlying problem but it triggers this - before the logo just spins, now you get the datatables error same as if you didn't have the backtrace setting.

[eileenmcnaughton]

oh progress - a datatables error :-)

Still I think loops are especially bad so this is better IMHO

[demeritcowboy]

Fun sidenotes:

• You know how sometimes when you run the upgrade using the UI and it hangs? I had always assumed it was just that the communication between the progress bar and the server gets cut off when there's an error, and since the error is in the log you can just look there. But what's actually happening is that first it hits formatTextException because that's what the queue uses to log the error, but that DOES exit its version of the loop because it checks for DB_ERROR. Then where it hangs is when the UI is trying to display the exception, which calls formatHtmlException. So this fixes that too.
• When running cli scripts, I can't see where the call to formatTextException actually ever happens, and looking at the history the call to it was maybe just added into handleUnhandledException for completeness. It seems to be only used by the queues.
  • It doesn't come up using cv since symfony gets in the way and handles it first.
  • It doesn't come up for standalone scripts since they don't usually call CRM_Core_Invoke::invoke().

[eileenmcnaughton]

@seamuslee001 @colemanw I think this should be merged based on above

[colemanw]

Agreed. Change makes sense.