dev/core#560 Update Cancel Billing & update billing to use status bounce rather than fatal

[eileenmcnaughton]

Overview

Minor stdisation on error handling

Before

After

(It's slightly weird that the box starts to open & then closes & the error msg pops up but IMHO it's still better)

Technical Details

Per https://lab.civicrm.org/dev/core/issues/560 we have resolved to get rid of fatal & move
to throwing Exceptions.

However, I'm leaning towards making statusBounces the rules for when we just deem a form inaccessible.

In this case we have 3 parallel forms - cancel, update & a similar update but slightly different just cos.

One does a status bounce - 2 spit out fatals when access not available.

Just looking at Cancel these are the places that still have fatal

• The recurring contribution looks to have been cancelled already
• Required information missing

I'm not sure the value of a fatal as opposed to the gentler statusBounces in either of those cases.

Throwing an exception looks very much like a fatal except

1. it doesn't hurt tests or cli tools as much
2. IF display backtrace is enabled THEN the back trace displays - ie

so, it's generally better BUT I kinda feel like the only time when it's actually better than a status
bounce is when a bug has occurred (eg. an unexpectedly failed api call) - anything we anticipate
seems like a bounce to me....

Comments

@colemanw @seamuslee001 @monishdeb @mattwire @jitendrapurohit @pradpnayak @totten
I feel like it would be good to have a generalised agreement on when to use statusBounce as opposed to throwing an exception. Per above I would say almost always at the form layer

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

[mattwire]

@eileenmcnaughton This looks simple and I agree that switching fatal to statusbounce is almost always the correct thing to do at the form layer. My only concern is that this is making the same change on both forms with identical code that could instead be extracted out into a shared parent class per your comments on #13285. My preferred approach is actually to align the code on each of the forms before extracting the common functionality but I feel we should do one or the other and not both!

[eileenmcnaughton]

@mattwire the process of aligning the code feels very risky & error prone to me - lots of renaming variables to the 'wrong' thing in order to make them the 'same' in order to be able to compare them. I feel much more comfortable extracting a function to a trait or shared parent & then cleaning it up & then working through the other forms to ensure they can use the function & keeping each step small (& obviously looking to where we can add tests).

Regarding this PR - I think using it as a discussion piece on when we use statusBounce & when we use Exceptions is probably more important that whether it actually gets merged

[mattwire]

@eileenmcnaughton That makes sense, I'd suggest we do the same with this PR as the two code blocks are identical except for the text in the message.
Re switching to statusbounce I thought we already had consensus that it was the right thing to do eg. https://github.com/civicrm/civicrm-core/pulls?q=is%3Apr+statusbounce+is%3Aclosed

[eileenmcnaughton]

@mattwire well issue 560 is about replacing fatal with throwing exceptions - but I'm wondering if that is what we should be doing

[mattwire]

There's a related issue somewhere that came up recently @mfb? because fatal() causes issues with the PEAR error scope and suppresses errors in some cases.

[mfb]

Not sure which specific issue you're referring to, maybe #13682? The push for nicer error notifications in the UI sounds great (as does throwing exceptions, in non-UI code).

[mattwire]

@mfb thanks yes it was #13682