Maintainability 101

Maintainability — Handling exceptions.

Let’s see together why it’s important and how it can change the way you code.

This is an article I wrote a few years ago on my personal blog after stumbling on a Pull Request that was accepted on Akeneo. I wanted to revisit this article, as I still see this error often in code I review and even in software I use. I have tried to improve upon my first variation.

Let’s first see the Pull Request at the origin of this article; PIM-7915: Display a modal on deletion error

This pull request adds a nice catch try.

try {
    $remover->remove($group);
} catch (\Exception $exception) {
    return new JsonResponse(['message' => $exception->getMessage()], 500);
};

This merge request dates from the 6th of January 2019; and the code is still present in the 4.0 version of Akeneo.

Why am I bothered with this try-catch, what is wrong with it?

• The catch doesn’t catch a particular exception but \Exception so all exceptions will be caught.
  I suspect that the purpose is to catch exceptions such as “Group couldn’t be deleted because it’s in use”. But the way it’s been coded we might end up catching a Framework exception or a database connection exception.
  First, issue: We do not know what we are catching.
• The exception message is also sent to the user; this can be done but only when we are aware of the messages we are going to show our users.
  By catching all exceptions we might very well show him a database error such as “Access denied for user ‘fakeuser’@’localhost’ (using password: YES)”
  There is way too much information in that message for someone with bad intentions.
  Second issue: We do not know the possible error messages we are showing to the users and what information we might leak to users.
• Finally, there is no logging. This is acceptable if catching specific exceptions; but if it’s an unexpected situation we need to log the exception in order to be able to debug the issue.
  The message displayed to the user has no value because the user might choose not to share it; and even if he does share it there is little context and no trace. Maybe just as we saved it the database server failed and it was an isolated situation. Maybe there is an error in our code that’s hard to reproduce there is no way to know (nginx/apache logs can be of help here).
  Third issue: We do not know how to fix the error.

So what should be done?

• Only catch exceptions you wish to catch, avoid catching \Exception as much as possible.
• Show user messages from specific exceptions for which you know the messages. For other exceptions send the user a generic message.
• Low-level framework exceptions should be handled by the Framework. They should return a generic error page and the necessary information should be logged but not displayed on a production environment.

If we wanted to fix the example above it would rather look like;

try {
    $remover->remove($group);
} catch (MyGroupUserException $exception) {
    // We know this exception is safe to be sent to the user.
    return new JsonResponse(['message' => $exception->getMessage()], 500);
} catch (MyGroupException $exception) {
    // This exception is not safe and should not happen. So we log and send a generic exception. 
    $this->logger->error("There was an error while deleting the user group : {$exception->getMessage()}", ['exception' => $exception]);
    return new JsonResponse(['message' => "The grouop coudln't be deleted"], 500);
};
// All other exceptions will be handle by the framework. 

Of course, it’s is not always possible to control what we need to catch. Even some very popular and very good php libraries, throw generic exceptions instead of defining their own. In this case, we are obliged to catch \Exception. In this case, we need to :

• Always log the exception message. This will allow us to have the necessary information to debug the issue if needed.
• Always show a generic error message to the user and not the exception message. This allows us to be sure no sensitive information can be leaked to the users.

So if we needed to rewrite our example for this scenario it would rather look like this.

try {
    $remover->remove($group);
} catch (\Exception $exception) {
    $this->logger->error("There was an error while deleting the user group : {$exception->getMessage()}", ['exception' => $exception]);
    return new JsonResponse(['message' => "The group couldn't be deleted"], 500);
};

This has of course the downside of possibly generating a lot of unnecessary logs. We will talk about exploiting logs in another article.

For now, let’s rather talk about logging exception.

Logging

Logging properly and having your logs parsed and easily available is a subject on its own. One for which I will write another article. The really important part of a log is

• To be easily identifiable. A message such as “An error happened” is not an identifiable message. When you read the message you must be able to quickly determine where it might come from.
• It needs to contain all the necessary data to debug. For example for an api, the url’s called and the parameters, as well as the response, should be logged.

But let’s concentrate on logging an exception message.

• Prefix your log message before logging the exception message. This will make it easier to find the origin of the log and allow the log to have more meaning without having to look into the code.
• Always add the exception to the context [‘exception’ => $exception]. This is the way PSR-3 says exceptions must be logged.
  If an Exception object is passed in the context data, it MUST be in the ‘exception’ key. Logging exceptions is a common pattern and this allows implementors to extract a stack trace from the exception when the log backend supports it. Implementors MUST still verify that the ‘exception’ key is actually an Exception before using it as such, as it MAY contain anything.
• Add additional information to the context such as the url of the page. If your code can be accessed from multiple pages this might make it easier to pinpoint the issue. Frameworks such as Symfony does it automatically. But Magento for example won’t do it.

To wrap it up.

There are basically 3 points when handling exceptions.

• Know what you are catching.
• Know what you are displaying to your user.
• Know how to debug your exceptions when they are triggered.

This way you know that

• You are not leaking sensitive data to the user
• You have user user-friendly messages displayed to your users
• You have all the details for easy troubleshooting.

Another example

I have taken an example from Akeneo; not because Akeneo is a bad product; it’s not the case, it actually works very well and it’s very well coded. Every piece of software will have small “errors” and I was surprised to see it in Akeneo.

I could have used a Magento example; Magento is full of such errors as well. One of my favorite examples is on the product pages controller.

try {
    $page = $this->resultPageFactory->create();
    $this->viewHelper->prepareAndRender($page, $productId, $this, $params);
    return $page;
} catch (\Magento\Framework\Exception\NoSuchEntityException $e) {
    return $this->noProductRedirect();
} catch (\Exception $e) {
    $this->_objectManager->get(\Psr\Log\LoggerInterface::class)->critical($e);
    $resultForward = $this->resultForwardFactory->create();
    $resultForward->forward('noroute');
    return $resultForward;
}

At first, this looks good. What does it do this :

• If the entity does not exist then displays a 404.
• If any other exception displays a 404 & log the exception.

So it looks like, it checks all 3 boxes; right?

• Know what you are catching. ✔️ We are catching NoSuchEntityException.
• Know what you are displaying to your user. ✔️ A 404 page.
• Know how to debug your exceptions ✔️ The error is logged.

This one is actually tricky. If we pay attention, we actually don’t know what we are catching. The issue is all Magento entities throws NoSuchEntityException. So if in any of the blocks on the product page try to load any other entity besides the product; we can end up with a 404. It could also be trying to load additional products for a cross-sell for example. In those cases, we have no information to debug why we got a 404.

Simply looking at the NoSuchEntityException’s message we can figure out that it’s not the product that can’t be loaded but for example a category. But we don’t have that information. It’s actually possible to get into such a scenario by just messing up contributions in the back office on a native Magento.

So reviewing this try-catch again with this knowledge; would rather say:

• Know what you are catching. ✖️ We are catching all NoSuchEntityException, we don’t know which entity was not found. The exception is too generic.
• Know what you are displaying to your user. ✔️ A 404 page.
• Know how to debug your exceptions ✖️ We have no logs about which type of entity was not found when catching the NoSuchEntityException.

Conclusion

I have spent more time then I would like to maintain old e-commerce websites. Handling your exceptions properly will make your code more maintainable for yourself, but also for all the other developers that will have to maintain the code at some point.

Even on projects I know I won’t maintain I am of the opinion that the top priorities are.

• Maintainability
• Customer satisfaction / Cost

Yes, maintainability is more important. Even if at first it might cost slightly more it will pay for itself in the long term. On quickly evolving websites with redesigns sometimes done every year, features added daily; the long term might be just in less than a year.

You must of course sometimes do compromises, but maintainability needs to be always kept in mind. And when you do a compromise you must know where you are possibly adding maintenance issues in order to add if possible documentation (this can just comment in the code in some cases).

We have seen how to handle exceptions, in the newt article I would like to talk a bit more on how to log and why logs are important.

Maintainability — Logs & Their use

Thank you for reading.