Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow optional disabling of connection exceptions #16

Merged
merged 3 commits into from
Jun 11, 2015
Merged

Allow optional disabling of connection exceptions #16

merged 3 commits into from
Jun 11, 2015

Conversation

michaelmoussa
Copy link
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
Copy link
Member

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

@philsturgeon
Copy link
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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@philsturgeon @marcqualie

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

@michaelmoussa
Copy link
Contributor Author

ping?

marcqualie added a commit that referenced this pull request Jun 11, 2015
Allow optional disabling of connection exceptions
@marcqualie marcqualie merged commit ee55378 into thephpleague:master Jun 11, 2015
@marcqualie
Copy link
Member

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

@michaelmoussa michaelmoussa deleted the disable-exceptions branch June 11, 2015 13:42
@michaelmoussa
Copy link
Contributor Author

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

@marcqualie
Copy link
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants