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

Fix language negotiation exception to be standard #24764

Merged
merged 1 commit into from
Oct 20, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Oct 18, 2022

Overview

Fix language negotiation exception to be standard

Before

CRM_Core_Locale::negotiate throws a RunTime exception when provided bad data

After

CRM_Core_Locale::negotiate throws a CRM_Core_Exception

Technical Details

`RunTime exceptions are defined as
"Exception thrown if an error which can only be found on runtime occurs."

I guess you could argue that any place in the code that fails when data is bad / fails validation is a runTime exception - but in practice in our codebase historical bad database data (e.g having an invalid value in civicrm_contact.preferred_language or an invalid email would generally throw a CRM_Core_Exception and most code that calls CiviCRM function expects and handles this). Where I see runTime exceptions in Civi code is mostly where a site is misconfigured - so not something that an extension would need to know about or handle.

This function is a utility function that is frequently called from sending messages & it seems wrong to have an exception thrown that bypasses 99% of civicrm exception handling

Side note - the type of exception thrown should have been documented in the docblock & we should possibly document the reason whenever it isn't a derivative of CRM_Core_Exception

Comments

@totten

@civibot
Copy link

civibot bot commented Oct 18, 2022

(Standard links)

@civibot civibot bot added the 5.55 label Oct 18, 2022
Copy link
Contributor

@wikimediaWfan wikimediaWfan left a comment

Choose a reason for hiding this comment

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

Ha right, this error exception is not about runtime error

@totten
Copy link
Member

totten commented Oct 20, 2022

Side note - the type of exception thrown should have been documented in the docblock & we should possibly document the reason whenever it isn't a derivative of CRM_Core_Exception

Heh - The main reason to use RuntimeException is that it doesn't require a docblock. 🙃

(I kid, but it's true. You can play with it in the PhpStorm to see -- Exceptions will follow you around with docblocks and warnings, but RuntimeExceptions won't. PhpStorm's behavior makes much more sense if you interpret Exception/RuntimeException from Java perspective - ie checked vs unchecked. Granted, in PHP's runtime, they don't really have "checked" exceptions -- so it only makes a difference in how the IDE presents it.)

In any event, you're right that everything else is CRM_Core_Exception - so this might as well be too.

@eileenmcnaughton
Copy link
Contributor Author

@totten ok to merge then?

@totten totten merged commit d4e3a1c into civicrm:5.55 Oct 20, 2022
@eileenmcnaughton eileenmcnaughton deleted the runtime branch October 21, 2022 06:34
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.

3 participants