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

System: remove raw exception message output from the interface #1767

Conversation

SKuipers
Copy link
Member

@SKuipers SKuipers commented Dec 13, 2023

Currently, there's many areas of the code where exception messages are output directly to the user-facing interface. In particular, when handling SQL queries. This is problematic for two reasons, one in that the Connection class already internally handles exceptions so this is a duplication of code, and another because exception messages shouldn't be displayed raw to the user except when in Development mode. Instead, in Production systems they should be logged, which is another set of refactoring we have planned for the future.

Description
Removes all raw $e->getMessage() output from try/catch blocks. Scripts that use $e->getMessage() internally and properly handle the exception message have not been changed.

Motivation and Context
Refactoring and security.

How Has This Been Tested?
Locally and CI.

@yookoala
Copy link
Member

Should the error message be logged somewhere instead of just ignored?

@rossdotparker rossdotparker merged commit 1e63a11 into GibbonEdu:v27.0.00 Dec 13, 2023
4 checks passed
@rossdotparker
Copy link
Member

I've merged this for now, but @yookoala raises an interesting point. Would these errors not show up in the MySQL or PHP logs?

@SKuipers
Copy link
Member Author

Thanks Koala and Ross. Yes, the Connection class already catches and handles exception output for queries, so these were for the most part duplicates from old code. For any of the other ones, on our list is to go through and clean up all these try/catch statements, and will add proper logging to the cases that need it. Focused on cleanup only for this PR.

@SKuipers SKuipers deleted the feature/Refactoring_Exception_Errors branch December 14, 2023 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants