We implemented a payment extension for Montonio for Shopware 6 and while incorporating the feedback from the first Shopware code review I stumbled upon a weird bug in our unit test suite.

It turns out the error I saw was not the error which happened.

The problem

For the payment extension we add a couple of media files, so we can attach them to the payment methods. To do this we have something like this:

try {
    // Install Media files
} catch (DuplicatedMediaFileNameException|MediaException|\Exception $e) {
    // find the already installed file and return the id
}

You see the problem already?

Because I reworked that part of the code one of my tests fail, but because PHPUnit is throwing an exception in a mock, when a method fail their expectation it was caught here – and of course not properly handled, because it is no case we have in the code.

The consequence

We caught a \PHPUnit\Framework\ExpectationFailedException with a message telling me, that the filename is wrong (I removed the file extension, before that it was saved as .png.png).

It took a moment to realise that problem, it could have been hours to be honest, thankfully not this time. The fix itself was removing .png – so 5min.

But why does that matter?

I was a big fan of „better catch too much than too less“, but that showed me I though think that through – and what I learned is, that not only PHPUnit might break my code, but even worse: If I catch ANY exception and don’t handle it properly.

Why? You ask?

Because if you don’t know what exception you caught, you don’t know the use case and unfortunately we still can’t write code which handles unknown problems in the future.

Better approach

I’m not sure it is a better approach, but NOT handling generically any exception means, it bubbles up to the top and shows in the worst case a 503 to the visitor of your side. This is not cool and we don’t want that, but imagine you set an order as paid and ship it, but something happened, you caught it and didn’t handle it properly.

So better have proper monitoring of your servers, applications and log files, so you spot unhandled exceptions early. In the base case on stage 🙂

Other articles from this category