search

amateescu / search_api_solr

11 stars 14 forks source link

Add exception handling #37

Closed drunken-monkey closed 9 years ago

drunken-monkey commented 9 years ago

See #2414347: when deleting items but Solr isn't running, it seems the Solr backend throws an exception from deleteItems() that is not a SearchApiException and, therefore, not permitted there. The exception should be caught and re-wrapped instead.

However, upon fixing this I saw that the code currently is never catching any Solarium exceptions (except in ping()), so this change is just a start, this needs to happen everywhere we use Solarium methods that can throw exceptions.

drunken-monkey commented 9 years ago

Where are we catching \Exception? I think, unless you really need to make absolutely sure, you always just catch the exceptions you expect to be thrown. So, in this case, any Solarium exception (it seems they aren't very reliable when it comes to @throws documentation, so no real telling which of them can be thrown by a given method), and they all implement \Solarium\Exception\ExceptionInterface.

amateescu commented 9 years ago

Where are we catching \Exception?

Everywhere in core where catching a specific exception is not enough. And in case you want to catch \Solarium\Exception\ExceptionInterface specifically, don't we need a use statement at the top of the file?

drunken-monkey commented 9 years ago

Ah, sorry, of course you're right! Sorry, I'm completely new to GitHub – I had a working patch already, but then didn't know how to get that up here as a pull request. (Now I know, but only after creating this one, and I obviously missed the import when re-doing the change here.)

And I'd say catching all Solarium exceptions here is enough.

amateescu commented 9 years ago

And I'd say catching all Solarium exceptions here is enough.

I agree :)