In #2130819: UI improvements for the "View" tabs, we are just adding a feature and associated optional getExtraInformation() method to the service class for providing service-class specific information in a nicer format than is currently possible with viewSettings(). In D8, we should make this method a required part of the service class interface (but it can of course just return an empty array, which the abstract plugin base would take care of).

Another idea in that direction would be to integrate a method like Solr's ping() directly into the service class, which could be used for checking whether the server is correctly set up and available at the moment in a compatible way. We could then use that information, e.g., to improve the Search API overview, by coloring the rows accordingly, and/or add a hook_requirements() checking all enabled servers. (Disabled servers should probably never be checked.)

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 : 5
Story Points: 5

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Nick_vh’s picture

Title: Allow service classes to provide more specific feedback/information » Allow backend plugins to provide more specific feedback/information
Issue summary: View changes
Issue tags: +beta blocker
borisson_’s picture

I started implementing this, but noticed that the \Drupal\search_api\Backend\BackendSpecificInterface already specifies a method called viewSettings.
According to the description of that method, this is intended for the same this as this issue.

Returns additional, backend-specific information about this server.

I think this is already sufficient and we can close this issue? I attached a patch with my work to make sure we don't lose the work I started on.

drunken monkey’s picture

Yes, the first part is already done. The second suggestion, with the dedicated isAvailable() (or something, names are hard) method and integration into the Search API overview (and the admin status report) might still be an idea, though, if you want to implement that.

borisson_’s picture

Status: Active » Needs review
FileSize
3.43 KB

No relation to the previous patch, so no interdiff.

I added an isAvailable method to the backendSpecificInterface and the code to show it in the overview. I added a check in the Integration test for this as well.

Patch is pretty straightforward.

drunken monkey’s picture

Status: Needs review » Needs work

With "Search API overview" I meant the initial admin/config/search/search_api page, not the server overview. For the server overview page, there is already viewSettings() (which we might want to rename, actually, come to think of it) for showing additional information, so backends can (and do) display the server's availability.
So the new isAvailable() would probably just do something like switch the status graphic to "error" and maybe color the table row red, or something like that. And it could be included in a hook_requirements('runtime') implementation (for all enabled servers).

Otherwise, this looks pretty good, thanks! As you say, rather straight-forward.

borisson_’s picture

Status: Needs work » Needs review
FileSize
2.99 KB
6.42 KB

Maybe we should add a test with a test backend that always returns FALSE for the isAvailable method. Other suggestions fixed in the patch.

drunken monkey’s picture

Status: Needs review » Needs work

Great work, thanks!

Regarding the test, we could just adapt the existing TestBackend with an implementation that returns something like \Drupal::state()->get('search_api_test_backend.available', TRUE). Then we could easily test both cases.

Some further remarks on the latest patch:

  1. +++ b/search_api.install
    @@ -4,6 +4,7 @@
      */
    +use Drupal\search_api\Entity\Server;
    

    This should have an empty line in between.

  2. +++ b/search_api.install
    @@ -116,6 +118,25 @@ function search_api_requirements($phase) {
         }
    +    $servers = Server::loadMultiple();
    

    This, too.

  3. +++ b/search_api.install
    @@ -116,6 +118,25 @@ function search_api_requirements($phase) {
    +      );
    +
    +    }
    

    This not.

  4. +++ b/search_api.install
    @@ -116,6 +118,25 @@ function search_api_requirements($phase) {
    +      if (!$server->isAvailable()) {
    

    This check should only be made for enabled servers. Either add the check there, or only load enabled ones (probably better, regarding performance).

  5. +++ b/search_api.install
    @@ -116,6 +118,25 @@ function search_api_requirements($phase) {
    +          'value' => t('The servers: @servers are down.', array('@servers' => implode(',', $unavailable_servers))),
    

    I would use format_plural() (or whatever that's called now), implode with a space in between (', ') and phrase it a bit differently: "The search server "@servers" is currently not available."/"The following search servers are not available: @servers."

  6. +++ b/src/IndexListBuilder.php
    @@ -132,8 +137,9 @@ class IndexListBuilder extends ConfigEntityListBuilder {
    +        $entity instanceof ServerInterface && !$entity->isAvailable() ? 'color-error' : '',
    

    Better not do that inline, adding an empty class doesn't seem very clean and the line is in general too confusing, I'd say.

    Also, apart from that, I think it would be good if the status label would also change for unavailable vs. disabled.

    Finally, isAvailable() should only ever be called on enabled servers, I would say. I don't think we want to, e.g., ping a disabled Solr server. (And the current code would in that case also display it as an error, even though the server is disabled.)

  7. +++ b/search_api.theme.inc
    @@ -137,6 +137,15 @@ function theme_search_api_server($variables) {
    +  // Add backend availability information.
    +  $info = $server->isAvailable() ? t('Server is available.') : t('Server is not available.');
    +  $class = $server->isAvailable() ? 'ok' : 'error';
    +
    +  // Append the row and reset variables.
    +  $label = t('Backend availability');
    +  $rows[] = Utility::deepCopy($row);
    +  $class = '';
    +
    

    As said, I wouldn't include the availability there, that's something the backend can do themselves, if appropriate. For the database backend, e.g., this would have no value whatsoever, while the Solr backend includes the ping delay together with that information.

borisson_’s picture

Status: Needs work » Needs review
FileSize
6.37 KB
8.43 KB

Fixed #7.

drunken monkey’s picture

Status: Needs review » Fixed

Excellent, looks great. Thanks!
Committed.

Status: Fixed » Closed (fixed)

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