debugphpMinor
Error Handler in PHP
Viewed 0 times
handlererrorphp
Problem
I am currently developing a Content Management System, and I have just completed the Error Handling system, which handles errors according to the user's settings.
I want to know every single thing I could improve to make it more flexible, robust, light weight and fast processing. If there are better ways of doing things, I am ready to re-do stuff. I just want comments on
Global.php
I have 3 classes and 1 interface.
class.ErrorHandlerController.php:
class.ErrorHandlerDatabaseMapper.php:
```
database = $database;
}
public function log($error) {
$query = $this->database->prepare("INSERT INTO errors VALUES('', ?);");
$query->bind_param('s', $error);
$query->execute();
$query->close();
return true;
}
public function returnErrorList() {
$query = $this->database->prepare("SELECT error FROM errors");
$query->execute();
$query->bind_result($error);
while($query->fetch()) {
$errors[] = $error;
}
$query->close();
return $errors;
}
public function truncateErrorList() {
$query = $this->database->prepare("TRUNCATE TABLE errors");
$query->execute();
$query->close()
I want to know every single thing I could improve to make it more flexible, robust, light weight and fast processing. If there are better ways of doing things, I am ready to re-do stuff. I just want comments on
ErrorHandling System, in this global.php you can see how I instantiate them. It would also be helpful if you can suggest renames to variables, etc to make more sense. This project is going to be up on GitHub soon. Please be as strict as possible, and point out even a slight flaw in it.Global.php
connect_errno) {
echo $database->connect_error;
}
}I have 3 classes and 1 interface.
class.ErrorHandlerController.php:
errorHandlerMapper = $errorHandlerMapper;
}
public function handleErrorLogging($error) {
return $this->errorHandlerMapper->log($error);
}
public function handleReturningErrorList() {
return $this->errorHandlerMapper->returnErrorList();
}
public function handleErrorListTruncating() {
return $this->errorHandlerMapper->truncateErrorList();
}
}class.ErrorHandlerDatabaseMapper.php:
```
database = $database;
}
public function log($error) {
$query = $this->database->prepare("INSERT INTO errors VALUES('', ?);");
$query->bind_param('s', $error);
$query->execute();
$query->close();
return true;
}
public function returnErrorList() {
$query = $this->database->prepare("SELECT error FROM errors");
$query->execute();
$query->bind_result($error);
while($query->fetch()) {
$errors[] = $error;
}
$query->close();
return $errors;
}
public function truncateErrorList() {
$query = $this->database->prepare("TRUNCATE TABLE errors");
$query->execute();
$query->close()
Solution
Errorhandler or ErrorLogger?
As far as I can see, you wrote an ErrorLogger, not an error handler. You are not registering an error handler. The only thing these classes seem to do is Logging and giving access to the errors - with the option to truncate them.
So in fact, you created a Logger that can delete all errors (now why would you do that?), so call it a Logger.
The good peeps over at psr - standards have create a really nice interface for a Logger. Implementing that interface would be a big plus since you could easily change it with a complete different logging engine if needed.
Code "errors"
You are using namespaces, that's good. But the location of your files is just illogical. Stick to psr-4 or psr-1. Standards exist because they are proved to be good. And instead of using all those requires. Use an auto loader instead. Write a custom one or use a complete solution like Symfony loader or even composer.
In your database you are only saving the error, nothing more. Some extra information would be useful. Like when did it occur for instance.
If every method starts with the same word and if that word can also be found in the class name, you probably don't need it. Writing:
It feels weird. You say that it should handle logging errors. But you need an error to handle error logging. But the only thing it does is log an error. So what does the
Way more readable.
Then your method name:
The same goes for
When using the local error mapper problems will arise if the error message contains ';'.
And as a last remark. If you are creating an
Your class failed. It is not handling errors.
Disclaimer: everything I wrote is here to help you; If you feel offended, I apologise. This is not what I intended.
As far as I can see, you wrote an ErrorLogger, not an error handler. You are not registering an error handler. The only thing these classes seem to do is Logging and giving access to the errors - with the option to truncate them.
So in fact, you created a Logger that can delete all errors (now why would you do that?), so call it a Logger.
The good peeps over at psr - standards have create a really nice interface for a Logger. Implementing that interface would be a big plus since you could easily change it with a complete different logging engine if needed.
Code "errors"
You are using namespaces, that's good. But the location of your files is just illogical. Stick to psr-4 or psr-1. Standards exist because they are proved to be good. And instead of using all those requires. Use an auto loader instead. Write a custom one or use a complete solution like Symfony loader or even composer.
In your database you are only saving the error, nothing more. Some extra information would be useful. Like when did it occur for instance.
If every method starts with the same word and if that word can also be found in the class name, you probably don't need it. Writing:
$errorHandler = new ErrorHandlerController;
$errorHandler->handleErrorLogging($error);It feels weird. You say that it should handle logging errors. But you need an error to handle error logging. But the only thing it does is log an error. So what does the
ErrorHandlerController do? It passes everything to the ErrorMapper. So why not use the error mapper?$errorHandler = new ErrorHandlerDatabaseMapper;
$errorHandler->log($error);Way more readable.
Then your method name:
returnErrorList. Return to where? The client? What you are doing is 'getting'. So getErrors or getErrorsAsList. But what is a list? It's just an arrayThe same goes for
truncateErrorList. What is the List keyword doing there? And why is it returning something? This means that every piece of code that truncates all the errors needs to check for success, so throw an Exception.When using the local error mapper problems will arise if the error message contains ';'.
And as a last remark. If you are creating an
ErrorHandler and after you have created your error handler you write:if($configuration['errors']['enable_error_logs'] == 1) {
if($database->connect_errno) {
echo $database->connect_error;
}
}Your class failed. It is not handling errors.
Disclaimer: everything I wrote is here to help you; If you feel offended, I apologise. This is not what I intended.
Code Snippets
$errorHandler = new ErrorHandlerController;
$errorHandler->handleErrorLogging($error);$errorHandler = new ErrorHandlerDatabaseMapper;
$errorHandler->log($error);if($configuration['errors']['enable_error_logs'] == 1) {
if($database->connect_errno) {
echo $database->connect_error;
}
}Context
StackExchange Code Review Q#61624, answer score: 5
Revisions (0)
No revisions yet.