Currently, if you call apachesolr_get_solr('foo'); and there isn't an environment called foo it will silently fall back to the default environment, as if I had called apachesolr_get_solr();
If I'm passing an environment id, then that means my code is multi-environment aware and there may well be significant differences between environments, so I want to know if my environment isn't available.
The function already throws an exception if it can't find any environment. It should also do this if it can't find YOUR environment.
Patch to follow.
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | apachesolr-invalid_environment_exception-1708166-1.patch | 689 bytes | ianthomas_uk |
Comments
Comment #1
ianthomas_ukComment #2
tim.plunkettThe module should provide and throw a named Exception, not the base class itself.
Comment #3
alanmackenzie commentedI think the ideas behind the patch makes sense. Supporting broken code with a nasty/confusing side effect is something we should aim to avoid.
However to do this properly will require a more significant rework. If this patch was applied and a non default the solr service was to go down it would bring down the Drupal instance as well.
To repoduce:
----
tim.plunkett is correct: http://drupal.org/node/608166
Given that there's 12 calls to the base Exception class it would probably be a good idea to create a separate issue for dealing with this (there's also some missing calls to t()). I'll let the module maintainers make a decision about this.
Comment #4
ianthomas_uktim: I agree that a named exception would be better, but this function already throws an unnamed exception. I'm just making this happen in an additional case. I agree with alan that naming the exceptions (including this new one) deserves it's own issue.
See http://drupalcode.org/project/apachesolr.git/blob/a5972f30f7b22847bf19b8... line 1064 for more context.
----
alan: I'm not sure I understand what you're trying to say. Is it:
a) There may be existing sites calling apachesolr_get_solr() with a bogus environment id, and if we released this patch as part of an update (e.g. 7.x-1.0-rc3) it could cause those sites to have fatal errors on their homepage after the update? or
b) Sites may be configured with two Solr instances. If one becomes unavailable, fall back to the other instead of producing a fatal error.
If it's the former, that's a valid concern. Is there a standard way of dealing with issues like that? Maybe we should set a variable in an update hook, and continue falling back to the default instance if that variable is set? That way new installs would get the safe behaviour, but existing, already broken installs wouldn't break even more than they are already. We could log a watchdog error to encourage broken sites to get fixed.
If it's the latter, then I don't really see that this patch changes much, particularly as the environments are likely to be on the same server and therefore either both up or both down. The documentation already tells you it might throw an exception (see my reply to tim).
Comment #5
alanmackenzie commentedIgnore my objection about this causing a 500 while a solr environment was unavailable. The exception generated it caught else where, this became obvious when I tested it with a full stack.
That's what I get for trying to be too smart :)
Comment #6
ianthomas_ukYour right. My original patch was based on the docblock, but I've checked and all calls in apachesolr and apachesolr_panels are wrapped in try-catch (or at least the ones that pass $env_id). This could still cause problems if you're calling apachesolr_get_solr() from custom code, but if you're doing that and passing $env_id then I imagine you care about the environment. Given that, is the original patch OK if we add a release note telling people to check any custom calls to apachesolr_get_solr()?
Comment #7
pwolanin commentedThis function is already annotated as throwing an Exception, so I don't think a release note is even needed per-se.
I agree with Tim that a named exception would potentially be a better design, but that should be a follow-up issue since that would require a lot of other changes.
Comment #8
nick_vhThanks for the contribution! It has been committed to 7.x-1.x, with authorship of your name Thomas!
Comment #9
pwolanin commentedpatch applies to 6.x also - committed.
Comment #10
pwolanin commentedSo, in writing some new tests I see a downside to this - it made it harder to test (and potentially harder to use) function apachesolr_search_run() because I have to have an environment defined rather than potentially creating an ad-hoc solr object.
I thin kthis is minor and likely few people are using it this way, but I'm setting this back to active for more discussion.
Comment #11
ianthomas_ukI don't follow. You're presumably talking about the call to apachesolr_drupal_query which in turn calls apachesolr_get_solr() (line 1513 of apachesolr.module in current 7.x-1.x head).
This change only affects the case when you call apachesolr_get_solr with a parameter and that parameter is not empty, so either I've not understood what you're getting at, or the problem already existed before this change.
Comment #12
pwolanin commentedOk, I was mis-remembering where the exception was thrown. I was thinking apachesolr_environment_variable_get() would lead to an exception with an invalid env, but i see it does not.
Comment #13
ianthomas_ukComment #14
ianthomas_ukComment #15
nick_vhWas already committed a while ago