Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert fatals to statusBounces in case forms #17212

Merged
merged 1 commit into from
May 5, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Replaces calls to fatal with statusBounce where they are in the case forms

Before

CRM_Core_Error::fatal

After

CRM_Core_Error::statusBounce

Technical Details

In general users should experience the bounce message whereas code should catch exceptions. So in the form layer it's CRM_Core_Error::statusBounce and in the BAO throw new CRM_Core_Exception and CRM_Core_Error::fatal is oh-so-2014

I'm just assuming form layer due to the folder these are in

Comments

@demeritcowboy

@civibot
Copy link

civibot bot commented May 1, 2020

(Standard links)

@civibot civibot bot added the master label May 1, 2020
}
if (count($form->_caseId) != 1) {
CRM_Core_Resources::fatal(ts('Expected one case-type'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the what?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds familiar. It comes from here as copy/paste - eb20fbf, but sort of makes sense. The actual issue is that _caseId became an array, so it was part of a larger change to only use the first one, and then a sanity check that there is only one when having more than one made no sense. But the wording is wrong since it has nothing to do with case types.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also they got the class wrong. CRM_Core_Resources::fatal() doesn't exist!

*/
public function preProcess() {
$this->_activityId = CRM_Utils_Request::retrieve('activityId', 'Positive');
if (!$this->_activityId) {
CRM_Core_Error::fatal('required activity id is missing.');
CRM_Core_Error::statusBounce('required activity id is missing.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They all seem ok I'm just not sure about this one. This form is a form but not a form - as far as I know it's only reached via an ajax snippet, and when I try to force a failure it gives me a weird screen with the error squished inside an unnecessarily small textbox. But I get the same thing before the patch so that's ... good?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@demeritcowboy I've switched it to throw an Exception since there is no clear case for a statusBounce and throwing an exception is closer to no change

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!

@eileenmcnaughton
Copy link
Contributor Author

test this please

1 similar comment
@seamuslee001
Copy link
Contributor

test this please

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy is this good to merge?

@demeritcowboy
Copy link
Contributor

Yes!

@eileenmcnaughton eileenmcnaughton merged commit b2e10b3 into civicrm:master May 5, 2020
@eileenmcnaughton eileenmcnaughton deleted the fatal branch May 5, 2020 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants