GibbonEdu / core

Gibbon is a flexible, open source school management platform designed to make life better for teachers, students, parents and leaders.
https://gibbonedu.org
GNU General Public License v3.0
460 stars 299 forks source link

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

Closed SKuipers closed 8 months ago

SKuipers commented 8 months ago

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 commented 8 months ago

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

rossdotparker commented 8 months ago

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 commented 8 months ago

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.