Add the concept of query extenders

Results of my first visual review of this patch:

+++ b/contrib/search_api_views/includes/query.inc
+++ b/contrib/search_api_views/includes/query.inc
@@ -133,6 +133,9 @@ class SearchApiViewsQuery extends views_plugin_query {

+++ b/includes/query_extender.inc
@@ -0,0 +1,210 @@
+   *   TRUE, iff the field should be processed.

Typo

+++ b/contrib/search_api_views/includes/query.inc
@@ -144,12 +147,145 @@ class SearchApiViewsQuery extends views_plugin_query {
+    // Extenders

Inline comments must end in full-stops, exclamation marks, or question marks.

+++ b/includes/query_extender.inc
@@ -0,0 +1,210 @@
+  protected function testField($name, array $field) {

Missing function doc comment

+++ b/includes/query_extender.inc
@@ -0,0 +1,210 @@
+   *   TRUE, iff the type should be processed.

Typo

+++ b/includes/query_extender.inc
@@ -0,0 +1,210 @@
+  protected function testType($type) {

Missing function doc comment

+++ b/search_api.module
@@ -1369,6 +1369,29 @@ function search_api_get_processors() {
 /**
+ * Returns a list of all available query extenders.
+ *
+ * @see hook_search_api_query_extender_info()
+ *
+ * @return array
+ *   An array of all available query extenders, keyed by id.
+ */
+function search_api_get_query_extenders() {

Would be nice to have a documentation of this in search_api.api.php

+++ b/search_api.module
@@ -1369,6 +1369,29 @@ function search_api_get_processors() {
+    // Initialization of optional entries with default values

Inline comments must end in full-stops, exclamation marks, or question marks.

For me this idea is tempting, as I need #1721262: Field collapsing for a feature I'm building. However, for me it's not fully clear yet what the advantage of the query extenders is compared to hook_search_api_query_alter() and the processors of the index itself ($this->index->preprocessSearchQuery($this);, ($this->index->postprocessSearchResults($results, $this);).
As far as I see the Goal from the summary could be reached with the existing hooks / processors:

To be able to use Solr field collapsing when doing SearchAPI queries, being able to choose when creating a view which compatible field to group on within the index.

The main advantage of query extenders seems to be this:

only executed at query time, and configurable per-query.

I'm afraid this strict separation would lead to duplicated work/code.
In your use case the collapsing (grouping) seems to be needed just in certain queries but I think it's also a valid use case to say all queries of an index need to use collapsing.
That said, wouldn't it be better to try re-use existing preprocessors on a per query basis?

My idea was to provide another abstract class for processors to extend which provides a method similar toSearchApiProcessorInterface::configurationFormSubmit().
The new method just needs to store the stuff query related and not globally if applicable.

Maybe the field collapsing is not a representative processor but I ended up copying the major parts of your query extender (#1721262: Field collapsing) into my processor, which seems like a lot of duplicated code.

Like Peter, I don't really see the added value of query extenders. Why can't we just use options like up to now? Adding them would work equally well (or probably even better) as adding an extender in all cases I can think of.
As far as I can see, this would just be a comparatively large API change without any benefits.

I'm also not sure why a processor would be needed here. Of course, a processor could set the options, but if we want to do this per-query, it would probably fit better into the Views/Page admin UI.

@ Peter: “iff” is not a typo, it means “if and only if”. But I've already had native speakers not know that, I think it's only common in information theory, formal logic and the like. I don't have any problems with its use here, though.

Oh, I see … Sorry, it seems I misunderstood the concept before, thinking of this as more of an analogy to DB query extenders. But in this form I do see their added value – providing common configuration for additional per-query options regardless of search type (Views, page or some other). That is an interesting concept, thanks for proposing that!
It also seems that there is hardly any API change at all, just one addition which should have zero effect on existing code.

What is certainly missing is the API documentation for the hook, and probably also an alter hook.
I'll take a closer took in the next few days. Other things I already spotted.

+++ b/contrib/search_api_views/includes/query.inc
@@ -144,12 +147,145 @@ class SearchApiViewsQuery extends views_plugin_query {
+    // Ensure that the search API admin include is around.
+    form_load_include($form_state, 'inc', 'search_api', 'search_api.admin');

Why?

Also, the whole code for creating the extender form should probably go to a Search API helper method, for this to be more helpful. Otherwise, individual search modules will have to duplicate this code after all.
A good way to judge this would be to also write a patch for the Pages module and use the same function there. ;)

+++ b/search_api.module
@@ -1369,6 +1369,29 @@ function search_api_get_processors() {
+      $query_extenders[$id] += array('enabled' => TRUE, 'weight' => 0);

We don't really want all of them enabled by default, do we? I'd rather say the default should be FALSE.

Other than that, this looks rather good. Thanks a lot for proposing and coding this!

coming here from #1721262: Field collapsing

drunken monkey already adressed this in #9:

+++ b/contrib/search_api_views/includes/query.inc
@@ -144,12 +147,145 @@ class SearchApiViewsQuery extends views_plugin_query {
+    // Ensure that the search API admin include is around.
+    form_load_include($form_state, 'inc', 'search_api', 'search_api.admin');

in my case this leads to

PHP Fatal error: Only variables can be passed by reference in /search_api/contrib/search_api_views/includes/query.inc on line 158

Is this still on your radar? Will you provide an updated and tested patch?
I'm really interested in this, it seems like a great idea.

Here's at least a re-roll for current dev, along with a fix for the form_load_include() call.

Hi there! I am going to follow this patch and provide (as far as I will be able to) patches and updates.
I think that the actual implementation that forces the grouping at the Index level is too strict, and one of the main disadvantages is that you will not be able to re-use the same index for un-grouped searches.

I'll keep you updated!

Thanks for picking this up!
However, I think at this point we should definitely first implement this in Drupal 8 (or in D7 and D8 in parallel) and then backport to Drupal 7 (if possible). And for that, the approach of re-using the existing processors, but allowing them to be overridden on a per-query basis seems the best solution.

If you'd also be available for working on that, please let me know and we can discuss it in detail.
Otherwise, I'm sorry, but you'll have to use a hackier solution in Drupal 7 for now (i.e., alter queries). Just working off the existing patches here is not an option anymore.

I've started some works in such direction: the module in my sandbox: https://www.drupal.org/sandbox/thepanz/2566785 allows processors to be configured and run on a per-query basis.

All the code has been taken from the latest patch provided here by DrunkenMonkey ( #1720348-15: Add the concept of query extenders) and completely moved to an external module, thus no patches are required for search_api, nor new hooks/code needs to be defined at Processors level.

Waiting for your feedback!

Looks great, thanks a lot!
I posted a review here: #2581151: Module review.

So, for D8 we can probably just port and include your sandbox's code.

I don't know anymore, what did we say about D7? In my opinion, it would make sense to leave this as a separate module for now and see if it gets enough users to warrant adding it to the Search API itself. (But it's possible that we've already agreed on something else?)

Regarding the "plan": initially we agreed about having something 'usable' for D8 with Class overrides, that would be back-ported to D7.

*Update*: the "SearchAPI Exteded processors" module is available here: https://www.drupal.org/project/search_api_extended_processors thus I think we can postpone this issue for D8.

Marked as postponed, see comment #21

Why postpone it in D8? There, I'd say, we still (might) want to add this in the framework itself, and we could do so at any time.
For D7, though, probably seeing how well the add-on module works and how popular it is is the best step.

I'm not certain, but this might easily be the largest remaining beta blocker, and I'm pretty sceptic about whether we should really do this. Normally, I'm the first to suggest (and mark as beta blocker) features adding incredible flexibility only five people in the world will ever need, but somewhere we have to draw the line.

What does anyone else say, can you really think of enough good use cases for this?

The thing is, there is little query-related stuff processors can do which Views filters can't, so that's a huge potential use case already taken care of. Other than Views there is really only Pages, at least it was that way during the whole D7 lifecycle, and Pages was always meant as a slim alternative – being able to override the processors there would be a bit overkill.
Lastly, there's always alter hooks for the more niche use cases.

I admit, having a common way for configuring processor overrides per query sounds good, and cleaner than the alternatives for those few use cases, but in my estimate there just aren't enough of those to make this worthwhile.

But I'm open for other arguments. (Also, as we say with the D7 patch, this might not even need any API changes to work, so we might as well add it after going stable, if we do discover enough demand.)

Would this cover things like being able to add location querying functionality to search backends outside of the base module?
It does seem a bit weird architecturally that the backends support location searching in SearchAPI, but the integration with the rest of Drupal is in another contrib module.

Discussed this with Nick_vh and borisson_, and we agreed to not implement it at this point. It doesn't add enough value to warrant the change – and it seems like we can also implement this after beta/stable if there proves to be enough interest.

Hi, this feature was made originally on D7 however on the last version has been a necesary update that client needs, in case anyone need it too i will leave it here.