There are a lot of extension modules that need backend-specific code to support their functionality: autocompletion, facets, grouping, spellcheck, location, …
Currently, we just let each of those define a feature and then implement that feature within the backend's code. This allows different backends (even those we don't know about) to support that module's functionality and keeps all code specific to a certain backend contained in that backend's module. On the other hand, though, this also means that a backend's code will get more and more bloated with each additional feature it supports, which makes the code harder to understand even if all the features aren't even used.
It also means that if someone creates a module using a certain new feature, it will have to get the maintainers of the backends it wants to support to commit the necessary code, which the backend module maintainer would then have to maintain as part of their module. This previously worked pretty well since both the backends and extension modules were mostly my own, but it's not a recipe for a future-proof ecosystem, where more and more modules (both new features and backends) will pop up.

Therefore, while we should still definitely allow all backends to support the functionality of extension modules, we should discuss moving some of those implementations into the extension modules themselves.
The only technical hurdle for that would be being able to alter a backend's supportsFeature() return value. (If we want to make an alter hook for it, though, changing it to getSupportedFeatures() and making it return an array of all supported features would probably make more sense.)

Of course, features like autocomplete, which use dedicated additional methods on the backend class, would need to put a bit more thought into it if they want to be able to implement the functionality for a specific backend. But since it's their decision anyways if they want that, it's not really a problem.

If we want to go through with this, we should probably also define a common way for extension modules to add such backend-specific implementations, so that it's easy (or easier) for others to see what's going on. In general, the problem of including hooks (or, even worse, classes) depending on whether a certain module is installed isn't such a simple one.

But in any case, we should first discuss if that's even something we want to do.

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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drunken monkey’s picture

Nick_vh’s picture

It is indeed a very complex problem and I appreciate that you have put some thought into it. I think that it should always be possible for a new module that wants to implement a new feature to control the whole flow and not be dependent on the maintainer of the backend to implement the feature for the certain backend.
Also, I'm not sure in which matter we need a backend to support all possible extensions. A backend should at least support the base Search API, with as many features as the backend claims to support using the getSupportedFeatures() and perhaps with a minimum of a couple enforced features such as basic search and filters?

An extension module could then alter each separate feature of a specific backend, as you mentioned or add a specific feature. In Drupal 7 this would be something like

function search_api_location_search_api_feature() {
  return array(
     'search_api_solr' => array(
       'feature' => 'search_api_location'
       'query callback' => 'search_api_location_query'
     ),
    'search_api_db' => array(
       'feature' => 'search_api_location'
       'query callback' => 'search_api_location_solr_query'
     ),
  );
}

And the alter could happen in the same way. If we make each feature a class? of its own, it would even be easier for the alteration, custom or contrib to subclass it and then alter or add to the feature definition list. We did similar things for the Apache Solr module to support arbitrary entities, a register with callbacks for each entity, and if the array of callbacks was present, the UI showed a checkbox that enabled that "feature" to index that specific entity.

For example, search_api_location is then responsible for choosing which backends it supports and which features it supports. The hook that adds the feature can decide which backends it adds the support for and where the implementation lives.

I'm not entirely sure how this thinking maps to Drupal 8 but I'm sure we're not the only ones with this architecture issue.

Nick_vh’s picture

Oh and also, in the query of the backend, I think it should take the Query object and all other possible parameters and pass it to those implementation callbacks for each feature the query is trying to accommodate to. Since we are passing objects to all the callbacks it should be possible to add query details.

The ordering might be tricky as some query implementations depend on other features. So perhaps we need a dependency chain where a feature depends on other features. Eg, location depends on the filters to already be there so the example would be:

function search_api_location_search_api_feature() {
  return array(
     'search_api_solr' => array(
       'feature' => 'location'
       'query callback' => 'search_api_location_query',
      'dependencies' => array('filters'),
     ),
    'search_api_db' => array(
       'feature' => 'location'
       'query callback' => 'search_api_location_solr_query',
       'dependencies' => array('filters'),
     ),
  );
}
drunken monkey’s picture

I don't think anything so complicated is necessary. I would have solved it as follows (in D8, a possible backport could be discussed afterwards):

class Server {
  public function getSupportedFeatures() {
    $features = $this->hasValidBackend() ? $this->getBackend()->getSupportedFeatures() : array();
    \Drupal::moduleHandler()->alter('search_api_server_features', $features, $this);
    return $features;
  }
}

And then any module that adds features to a server could just alter the query to behave accordingly (e.g., for Solr, using hook_search_api_solr_query_alter()). I don't think anything else is really needed.
If a feature is more complex than can (easily) be achieved with a simple alter hook, the module defining the feature can either define some custom workaround itself (e.g., the autocomplete module could check whether the necessary additional methods are present on the backend plugin and, if not, use some custom mechanism for finding callback functions/methods/whatever) or just require the backend plugin to implement the feature, like is currently the case.
Especially things like the search_api_location feature would really be tough to move out of the backend plugin, I think, since there are so many parts to this – when the option is present, it changes the facets, the filters, the sort and even the field (in Solr 4). Implementing this in a separate module, even if we'd provide a full-fledged dependency-aware backend feature plugin system, would be unnecessarily complicated, I think.

(Btw: I guess at some point soon we should discuss which hooks should rather be events. I think I'll need someone to slowly explain to me again how events work in D8 before that, though.)

Nick_vh’s picture

Issue summary: View changes
Issue tags: +beta blocker
drunken monkey’s picture

This implements the suggested solution from #4. As discussed, I think it should be flexible enough for most use cases.
Would be great to get feedback from other backend maintainers, though.
Also, we might want to add a second, backend-specific hook? Not sure about that, could be handy. (But, on the other hand, I don't think the current way will really cause problems either. Most sites don't have more than one server anyways.)

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

We should get a review from @mkalkbrenner as well if possible, but I feel this is a good improvement.

mkalkbrenner’s picture

Status: Reviewed & tested by the community » Needs review

The patch looks good and I'm basically fine with that API change. But in real life I see a lot of problems with this approach for more sophisticated extensions:

  1. The maintainer of an extension could not be familiar with all backends. At the moment we have three "first class" backends, DB, Solr, Solr Multilingual. But there is Elastic Search and others. Therefor other maintainers input will be required nevertheless. And then I prefer to maintain the code within the backend as to be forced to provide patches for extension modules.
  2. Some of these extensions might require Solr schema adjustments, for example geo location. There's no API for that and it is not supported by most Solr service providers.

Therefor I'm not completely sure, if that patch should be committed or not.

drunken monkey’s picture

As explained already, it's up to each maintainer on whether they want to implement support for any backends in their modules, and of course it won't be possible in all cases anyways. Adding support for a feature in the backend itself will, in any case, always remain possible, exactly for the two reasons you mention. Otherwise, the whole feature system would become pointless, which would definitely be a step backwards.

This is meant merely as an addition to that system. In the past, there were several cases of modules that wanted to integrate with Solr, and they always had to get their patches into the Solr module, meaning we now have integration code for a lot of modules there, some of which you or I aren't even that familiar with. Providing at least the possibility, in those cases where it is possible, to add Solr integration from outside the Solr module would be an advantage in those situations.

drunken monkey’s picture

@ mkalkbrenner: Does this sufficiently alleviate your concerns?

mkalkbrenner’s picture

ok, convinced

mkalkbrenner’s picture

Status: Needs review » Reviewed & tested by the community
drunken monkey’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
8.18 KB

OK, good to hear, thanks!
Needed a re-roll, but if the test bot consents I'll commit it right away.

  • drunken monkey committed 707c0c5 on 8.x-1.x
    Issue #2352475 by drunken monkey: Added a hook for altering a backend's...
drunken monkey’s picture

Status: Needs review » Fixed

Committed.
Thanks again for everyone's input!

Status: Fixed » Closed (fixed)

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