getConnectionClass() should not be setting the config variable search_api_solr_connection_class

[herbdool]

Reported by Oriol Roger

Yes, we found that the function getConnectionClass() is setting the config variable search_api_solr_connection_class. That function is called in a lot of places, and it's aim should be only to get the property connection_class from the class SearchApiSolrService.

So we don't really understand why the setting of the config variable is done in here; probably would make more sense to be set in the next function, setConnectionClass().

Anyway, commenting just the line config_set('search_api_solr.settings', 'search_api_solr_connection_class', $this->connection_class);, all our problems have gone away.

Actually, we don't know what is the config variable search_api_solr_connection_class used for. We can't find any place where this variable is relevant.

Thanks everybody for your help!

[herbdool]

@argiepiano got a PR. Looked in the commit history and it previously was using variable_get and that got switched to config_set. Oops!

[argiepiano]

Looking at the Backdrop code, one might think that it's possible to use method setConnectionClass() to change the connection class. However, this has no effect, since getConnectionClass always returns the default value stored in search_api_solr.settings.json. With the way things are written, it just doesn't make sense to have setConnectionClass() since it doesn't have any effect in Backdrop.

In D7, the code in setConnectionClass() does have a way to change the class within a single page request. You would call setConnectionClass() to set the class property connection_class, then delete the variable search_api_solr_connection_class. So, when you call getConnectionClass(), the line variable_get('search_api_solr_connection_class', $this->connection_class); will return the value you stored earlier.

So, if we want to mimic the D7 code, this would have to look like:

  public function getConnectionClass() {
    $connection_class = config_get('search_api_solr.settings', 'search_api_solr_connection_class')
    return !empty($connection_class) ? $connection_class : $this->connection_class;
  }

Thus, if for some reason the config value search_api_solr_connection_class is empty, we return the value stored in the property.

[herbdool]

I'm now thinking we change this to settings_get('search_api_solr_connection_class', $this->connection_class);. This is a setting that had no UI to set anyway and it's something that you don't want to expose to the UI because it could potentially cause a fatal error if the class doesn't exist. So that's the kind of rare change that only a developer would make and best to put it in the settings file. Then it would behave the same way as in D7.

[herbdool]

@argiepiano this is ready for another review.

[argiepiano]

This makes sense. LGTM

[herbdool]

I'm going to merge this now. There's no active maintainer and this bug is a real pain for those using the module.