Allow optional disabling of connection exceptions #16

Merged
merged 3 commits into from Jun 11, 2015

Projects

None yet

3 participants

@michaelmoussa
Contributor

Metrics gathering on my project is a welcome bonus, but not a critical feature, so I don't want a page load to fail if it can't connect to StatsD. I also don't want to wrap every invocation with:

try {
    // ... do statsd stuff
} (catch ConnectionException $e) {
    // do nothing
}

I could just write a wrapper to do all that transparently, but I think "just log a warning and keep loading the page" is a common enough use case, and making the exception throwing configurable would be a helpful contribution.

The throwConnectionExceptions configuration setting is completely optional, and it will throw exceptions by default like it does now (so there is no BC issue).

@philsturgeon
Member

Can you do a git pull --rebase origin master on this?

@philsturgeon
Member

I don't think trigger errors or more ideal that catching exceptions. I don't know right now the best approach, but a wrapper could be one.

@michaelmoussa
Contributor

@philsturgeon rebase done.

I made it optional since I think how one wants to deal with these types of failures is an implementation detail. Etsy's blog post on why they chose UDP in particular says:

The application doesn’t care if StatsD is up, down, or on fire; it simply trusts that things will work. If they don’t, our stats go a bit wonky, but the site stays up.

Since trigger_error(...) won't halt the application, I think that fits the above sentiment better. When my code throws an exception, I intend for it to either (a) not be caught and cause the request to fail gloriously or (b) be caught and handled. We obviously don't want (a) in this case, but in the case of (b), how would it typically be "handled" other than logging a message?

I was just hoping to avoid...

try {
    // ... do statsd stuff
} (catch ConnectionException $e) {
    // do nothing
}

.. because it gives me nightmares about how we used to write Java at university. :)

@michaelmoussa
Contributor

@philsturgeon @marcqualie

ping? :) Or should I close this w/o merge?

@michaelmoussa
Contributor

ping?

@marcqualie marcqualie merged commit ee55378 into thephpleague:master Jun 11, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@marcqualie
Member

Huge apologies for the delay on getting this merged @michaelmoussa, great contribution

@michaelmoussa michaelmoussa deleted the unknown repository branch Jun 11, 2015
@michaelmoussa
Contributor

@marcqualie would you mind tagging a new release with this change please?

@marcqualie
Member

I've now released version 1.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment