Remove error suppression operator (@) and add proper error handling#1741
Remove error suppression operator (@) and add proper error handling#1741jordikroon wants to merge 2 commits into
Conversation
…handling Replace error suppression operator (@) with explicit file existence and readability checks. Add new helper functions in include/file.inc (file_get_size, file_get_mtime, file_get_contents_if_exists) to safely handle file operations. This improves code reliability by catching actual errors rather than silencing them, and makes error handling explicit and testable throughout the codebase.
Add comprehensive documentation to the file header explaining the purpose of helper functions and their safety checks. Remove trailing stray braces that appear to be leftover code.
jordikroon
requested review from
derickr,
saundefined,
sgolemon and
sy-records
as code owners
| return false; | ||
| } | ||
|
|
||
| return file_get_contents($filename); |
There was a problem hiding this comment.
There is a race condition here, if the file gets removed between the file_exists/is_readable, and the file_get_contents. This is quite possible, as the rsync process to put new data on the servers is not atomic. I would recommend keeping the @ here.
| // Open the note file for reading and get the data (12KB) | ||
| // ..if it exists | ||
| if (!file_exists($notes_file)) { | ||
| // if (!file_exists($notes_file) || !is_readable($notes_file)) { |
There was a problem hiding this comment.
This introduces a parse error, as the { is now gone matching the } on line 106.
|
|
||
| // Open events CSV file, return on error | ||
| $fp = @fopen("backend/events.csv",'r'); | ||
| $fp = fopen($path,'r'); |
There was a problem hiding this comment.
There are race conditions here too for similar reasons.
|
@jordikroon Would you have some time to look at my comments? Otherwise I think we should close this and just leave the |
|
For now I will close this. Given the redesign it's not a good moment to push this through anyway. |
2 participants
At some places the
@symbol was still used. This PR changes that behaviour with proper handling.Since those checks are always (correct me if I am wrong) for internal paths. A simple file_exists & is_readable check is sufficient. If not we could override the error handler instead.