</code>Steps to replicate:

Use case 1:
1. Make sure Solr server is *unreachable*
2. Create a Solr server + index

Expected: Error validating server connection
Actual output: WSOD

Use case 2:
1. Make sure Solr server is *reachable*
2. Create Solr server + index
3. Close Solr
4. Visit index page

Expected: Index page with error message
Actual output: WSOD


<code>
The website encountered an unexpected error. Please try again later.

Solarium\Exception\HttpException: Solr HTTP error: HTTP request failed, Failed to connect to localhost port 8983: Connection refused in Solarium\Core\Client\Adapter\Curl->check() (line 248 of modules/search_api_solr/vendor/solarium/solarium/library/Solarium/Core/Client/Adapter/Curl.php).
Solarium\Core\Client\Adapter\Curl->getResponse(Resource, )
Solarium\Core\Client\Adapter\Curl->getData(Object, Object)
Solarium\Core\Client\Adapter\Curl->execute(Object, Object)
Solarium\Core\Client\Client->executeRequest(Object)
Drupal\search_api_solr\Plugin\search_api\backend\SearchApiSolrBackend->search(Object)
Drupal\search_api\Entity\Server->search(Object)
theme_search_api_index(Array)
Drupal\Core\Theme\ThemeManager->render('search_api_index', Array)
Drupal\Core\Render\Renderer->doRender(Array)
Drupal\Core\Render\Renderer->doRender(Array, )
Drupal\Core\Render\Renderer->render(Array, )
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}()
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object)
Drupal\Core\Render\MainContent\HtmlRenderer->prepare(Array, Object, Object)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object, Object)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object, 'kernel.view', Object)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.view', Object)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1)
Drupal\devel\StackMiddleware\DevelMiddleware->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1)
Stack\StackedHttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)

Estimated Value and Story Points

This issue was identified as a Beta Blocker for Drupal 8. We sat down and figured out the value proposition and amount of work (story points) for this issue.

Value and Story points are in the scale of fibonacci. Our minimum is 1, our maximum is 21. The higher, the more value or work a certain issue has.

Value : 8
Story Points: 2

CommentFileSizeAuthor
#15 search_api_index-wsod_server_unavailable-2575651-15-D8.patch2.23 KBjurcello
#13 search_api_index-wsod_server_unavailable-2575651-13-D8.patch2.71 KBtlyngej
#12 search_api_index-wsod_server_unavailable-2575651-12-D8.patch2.68 KBtlyngej
#9 search_api_index-wsod_server_unavailable-2575651-9-D8.patch2.24 KBdevd
#7 search_api_index-wsod_server_unavailable-2575651-7-D8.patch1.77 KBcristiroma
#5 search_api_index-wsod_server_unavailable-2575651-5-D8.patch1.93 KBdevd
#4 search_api_index-wsod_server_unavailable-2575651-2-D8.patch1.93 KBdevd
#2 search_api_index-wsod_server_unavailable-2575651-1-D8.patch1.71 KBcristiroma

Comments

cristiroma created an issue. See original summary.

cristiroma’s picture

cristiroma commented
Status: Active » Needs review
StatusFileSize
new search_api_index-wsod_server_unavailable-2575651-1-D8.patch1.71 KB

Attaching a patch which shows server error instead of the count of items from the index.

Status: Needs review » Needs work

The last submitted patch, 2: search_api_index-wsod_server_unavailable-2575651-1-D8.patch, failed testing.

devd’s picture

devd commented
StatusFileSize
new search_api_index-wsod_server_unavailable-2575651-2-D8.patch1.93 KB

Attached patch is working fine but we have HttpException type object, So it can we catch further.

devd’s picture

devd commented
StatusFileSize
new search_api_index-wsod_server_unavailable-2575651-5-D8.patch1.93 KB

Attached patch is working fine but we have HttpException type object, So it can we catch earlier.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented

Thanks for reporting this issue and providing a patch!
Two things, though:

  1. +++ b/search_api.theme.inc
@@ -297,13 +297,18 @@ function theme_search_api_index($variables) {
+      catch(Exception $e) {

    First off, it should be \Exception, according to the Drupal coding standards.
    More importantly, though, we should more specifically only catch the exception that the code is actually allowed to throw – i.e., SearchApiException.

    That the Solr backend apparently doesn't properly catch its internal exceptions to wrap them in a SearchApiException object is a different issue, and should be reported and fixed in that module.

    In any case, though, catching the possible exception there is correct and very important, so thanks a lot for the patch! 

  2. +++ b/search_api.theme.inc
@@ -297,13 +297,18 @@ function theme_search_api_index($variables) {
+        $info = t('The Solr server could not be reached. Further data is therefore unavailable.');

    It doesn't have to be a Solr server, this could be any other kind of server (and any other kind of error cause) as well. The message should be much less specific. And probably we should log the exception message somewhere – or, alternately, include it in the displayed information.

    Also, we should change $class to 'error' in this case.

cristiroma’s picture

cristiroma commented
StatusFileSize
new search_api_index-wsod_server_unavailable-2575651-7-D8.patch1.77 KB

Thank you for the comprehensive review!

I have uploaded a patch that addresses your observations, but doesn't solve the problem unless #2577041: Catch internal exceptions and throw them as SearchApiException is also fixed. This would be a first time submiting a non-working patch intentionally :D

cristiroma’s picture

cristiroma commented

@devd you are trying to catch Symfony\Component\HttpKernel\Exception\HttpException, while here is thrown Solarium\Exception\HttpException. This is not the same Exception, therefore your first catch statement is ineffective.

devd’s picture

devd commented
Status: Needs work » Needs review
StatusFileSize
new search_api_index-wsod_server_unavailable-2575651-9-D8.patch2.24 KB

Thanks Thomas & Cristian.

Hi Thomas as per you instruction, I did fixed the issued in attached patch.

Hi Cristian, I agree with you but we should the caught the exception at the end in catch block.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Issue summary: View changes
Status: Needs review » Needs work

@ cristiroma: Thanks for the revised patch, looks better now! However, still some issues:

  1. +++ b/search_api.theme.inc
@@ -297,13 +297,19 @@ function theme_search_api_index($variables) {
+      catch(\Drupal\search_api\SearchApiException $e) {

    Please use the import here instead. (Unless I've missed a change in the coding standards?) 

  2. +++ b/search_api.theme.inc
@@ -297,13 +297,19 @@ function theme_search_api_index($variables) {
+        $info = t('The underlying search server could not be reached. Further data is therefore unavailable.');

    This is still too specific. The problem doesn't have to be that the server could not be reached, it's just that an exception occurred during search. Just write that, and then maybe include the exception message to clarify things.
    (I.e., 'Error while checking server index status: @message.')

@ devd: Please stop rolling your own patches here, cristiroma's are quite good already, you're just adding confusion. If you want to help, please improve on cristiroma's last patch!

devd’s picture

devd commented

@Thomas thanks to correct me.
@Cristian thanks to good work.

tlyngej’s picture

tlyngej commented
Status: Needs work » Needs review
StatusFileSize
new search_api_index-wsod_server_unavailable-2575651-12-D8.patch2.68 KB

I've updated the patch based on the comments in #10.

tlyngej’s picture

tlyngej commented
StatusFileSize
new search_api_index-wsod_server_unavailable-2575651-13-D8.patch2.71 KB

Right, something went wrong with the other patch. Here's a new one...

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
Issue summary: View changes
Issue tags: +beta blocker
jurcello’s picture

jurcello commented
StatusFileSize
new search_api_index-wsod_server_unavailable-2575651-15-D8.patch2.23 KB

I reviewed the patch, and I have some remarks:

- There is still a use of \kint in the code.
- Why should the coding style of the header be changed? It is not of importance to this issue I think.
- I find the display of the line number confusing. This adds no information to the error message if the stacktrace is not displayed.

I created a new patch containing the corrections for these remarks.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented 
+++ b/search_api.theme.inc
@@ -297,13 +298,26 @@ function theme_search_api_index($variables) {
+        $info = t('Error while checking server index status: @message', array('@message' => $e->getMessage()));
...
+        $info = t('Error while checking server index status: <em>%message</em>.', $error);

We shouldn't translate error messages (see #2055851: Remove translation of exception messages). Use FormattableMarkup instead.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

My comment in #16 was bogus, this is only when we're throwing exceptions. Patch from #16 is ok.

tlyngej’s picture

tlyngej commented

@jurcello

- Ups. Didn't notice from the former patch.
- Array exceeded 80 characters, so I re-formatted it, according to the coding standards.
- Agreed, line number doesn't make much sense.

Good job.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

@tlyngej:

Both contrib and core code ask not to include unrelated changes in a patch, especially in unrelated hunks.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Fixed

Thanks for all your work here!
However, I think you all worked on the wrong patch. As said in #10, I ignored #9, my comments were directed at the patch in #7 (which was already pretty good).
I now just fixed the remaining two small problems myself and committed this.
Still, thanks again!

However, there's also still #2577041: Catch internal exceptions and throw them as SearchApiException which is the real problem here for Solr users.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.