silent failure on missconfiguration

[alwex]

in the Mailer class line 130, all exceptions are catch and never re-thrown causing silent error.

        } catch (\Exception $e) {
            // Close and remove any temp files created for attachments, then let the exception bubble up
            $this->closeTempFileHandles();
            return false;
        }

Could be a good idea to re throw the exception instead of returning false in the catch block, it will be very useful for debugging purpose.

[kinglozzer]

Agreed, unfortunately throwing an exception here breaks SilverStripe’s core Mailer API 😞 (see #6).

@dhensby any thoughts on this one? user_error($e->getMessage(), E_USER_WARNING) maybe?

[dhensby]

I don't think a warning helps too much either - how about just logging it using SS_Log?

[kinglozzer]

That relies on people setting up a log writer though, and even our installer doesn’t do that. I’m leaning towards raising a warning - it doesn’t need any extra configuration, you can’t miss it unless you develop in live mode, and it should be suppressed in live mode by default.

[dhensby]

Doesn't it write to syslog by default?

[kinglozzer]

Nope, it doesn’t log anything anywhere unless you set it up 😕. Either way, you’d be completely unaware that you’d done anything wrong unless you happened to check those logs

[dhensby]

Yep. Whether you trigger a warning or log the error it will be hidden on a prod server until someone checks.

This isn't really a problem for this module to solve because it's an issue with how SS mailer works... It never throws any error about whether emails are sent or not.

Perhaps storing the last exception and allowing devs to access it $mailer->getLastError() could be a solution to providing some kind of debugging support?

[kinglozzer]

How about this?

if (Director::isDev()) {
    user_error($e->getMessage(), E_USER_ERROR);
} else {
    SS_Log::log($e->getMessage(), SS_Log::ERR);
}

So it’s guaranteed to show up in dev mode, and anyone who wants to log errors in test/production envs can do so

[dhensby]

Sure.. I don't know how much a dev error would be as I imagine emails probably won't actually be sent on dev??

Maybe I'd log on both and error on dev only...

SS_Log::log($e->getMessage(), SS_Log::ERR);
if (Director::isDev()) {
    user_error($e->getMessage(), E_USER_ERROR);
}

Though I'm not really a big fan of such big differences in behaviour on dev vs non-dev

[dhensby]

How about Debug::message()?

[kinglozzer]

Looks good, I’ve gone for that: 87f8e00. Tagged as 2.0.1