Views is broken with non-Sql query plugins

Mh, so yeah views sucks, especially in that regard.

The handlers are currently all bound to SQL, which is kind of okay. What about introducing an SqlAwareQueryInterface implemented by Sql and check this here?

I'm upping this to major:

A PHP error which is only triggered under rare circumstances or which affects only a small percentage of all users.

Is there any workaround for this? Could Search API implement these methods itself?

Is there any workaround for this? Could Search API implement these methods itself?

In order to work searchapi has to implement all those handlers anyway, if I understand it correctly. Handlers are bound to their underlying query implementation, I don't see
how they could not.

I doubt that is a contrib blocker, the entire lifetime of d6/d7 was able to deal with it

I doubt that is a contrib blocker, the entire lifetime of d6/d7 was able to deal with it

I also don't think it's a blocker per se. However, with field handlers using these methods that are only on that specific query plugin, it becomes unnecessarily harder (and much more verbose, since you manually need to override these instances) to provide Views integration with a different query plugin (e.g., Search API). If the field handlers could be changed to only use the methods on the interface, this would greatly simplify things for new query plugins, because they have proper documentation on the methods they need to provide in order for the query plugin to work.

In D7 Search API we were "able to deal with it", yes, but it required an awkward workaround with boilerplate code like this for working around all this hard-wiring to SQL backends.

Also, since there was no proper separation between the SQL query plugin and the general definition of "that's how a query plugin should look" (i.e., a missing interface), contrib modules were always using the SQL-specific methods when dealing with Views queries leading to numerous instances where things would break horribly when trying to use such contrib modules' functionality with Search API views.
Having a proper QueryPluginInterface would be the best solution here, of course, but in lieu of that having code in the field handlers that shows that not all query plugins are SQL-based, and how to deal with that fact (SqlQueryInterface might really be a good option), would surely help a lot by providing a good example to contrib modules.

Having a generic query plugin interface and a more specialized sql query interface sounds like a great approach. I would really like to see proper work in that area.

@drunken monkey
Do you want to take over this initiative, especially given that you have the usecase in contrib?

I fear I don't have the time for that, sorry, at least not now. If no-one else steps up, maybe I'll give it a shot when I have time, but I'd really be grateful if someone else could find the time to work on this.
tstoeckler, maybe?

Would you then also be open to changing the field handlers (and maybe filters, etc., too) to explicitly check for the SqlQueryInterface before calling the SQL-specific methods, to make adaption easier for non-SQL query plugins? Or are you just for providing interfaces?

Also, how do you rate the chances this can be committed still for 8.0.0? If there's no chance it will be considered unfrozen, then I guess working on it doesn't make much sense anyways.

I think just putting the methods of the plugin base class on the interface would be the most natural. After all, it seems those methods were previously determined to be backend-independent. Deciding to move other methods to the interface, too, could also be done in follow-up issues.
Also, first you should probably wait for Daniel's opinion on whether this can even still make it into Core. I don't think adding the interface would be possible after 8.0.0 stable? (Don't really understand what will or won't be allowed for 8.1.0, etc., though.)

If Daniel thinks it would be a good idea to already move possible common methods to interface and base class, then these are the ones which I think would still be useful/applicable in the Search API (and probably most other backends):

• addField() (could just be ignored if not applicable, but would make field handlers easier to implement generically)
• clearFields()
• addWhere() (maybe renamed to condition())
• addOrderBy() (should maybe also be renamed)
• addTag() (could be ignored for now, but we might add query tags later)

Since renaming methods would be even more disruptive than the rest, though, I don't think that part will be possible.

In any case, thanks a lot for volunteering for this!

It would be great if the generic query interface could be kept as simple as possible and then have some interface with sane things like fields, where order by etc.

+1 to #15

I would also aim onto an interface which looks kinda similar to EQ. This would make it easier to move to a EQ based view in the future,
as every handle implementing just a simple query interface could be seen as EQ-able.

Any progress here, maybe on github?
I ran into this issue today while working on apachesolr_multilingual 8.x which is based on search_api now.

Any updates here?

Also Acquia Search using the Acquia Connector contrib module is affected by this bug.

 diff --git a/docroot/core/modules/views/src/Plugin/views/HandlerBase.php b/docroot/core/modules/views/src/Plugin/views/HandlerBase.php
  index 82a4a92..1b12184 100644
  --- a/docroot/core/modules/views/src/Plugin/views/HandlerBase.php
  +++ b/docroot/core/modules/views/src/Plugin/views/HandlerBase.php
  @@ -549,7 +549,7 @@ public function setRelationship() {
  	* {@inheritdoc}
  	*/
 	public function ensureMyTable() {
  -	if (!isset($this->tableAlias)) {
  +	if (!isset($this->tableAlias) && method_exists($this->query, "ensureTable")) {
     	$this->tableAlias = $this->query->ensureTable($this->table, $this->relationship);
   	}
   	return $this->tableAlias;

Would changing this in Drupal core really break things so horribly? If we can proceed with this patch or if someone can explain me what the better work-around is, I'm happy to document and implement that. This patch was provided to me by users that tried certain combinations of contrib modules and found this to break. I'm not submitting this as a patch to solve all the things but I do want to show this is a real issue that needs to be solved sooner rather than later.

I think we need a decision here. Either this gets fixed now, or is pushed back to Drupal-Next.
Other modules are in limbo over this.

I think we need a decision here

Alright, let me decide that @christianadamski will work on that :P
So yeah, the problem is that we need to design a more generic query interface and well, time is limited.

Regarding the patch in #20, what about adding an @internal interface with #8 and document that this is just a temporary workaround ?

@dawehner I'm just a pretty face with a Drupal T-Shirt, you don't want me to work on important stuff :)

No worries, just wanted to explain with a bit hurmor what is going on, or what isn't, in this issue.

Does this really block anything? Add an empty ensureMyTable and move on. The question rather is, what are you doing that this method is called at all? For example, random pick, BooleanOperatorString has $where = '(' . $where . " OR $this->tableAlias.$this->realField IS NULL)"; you need to override the whole plugin if you are not SQL anyways. It's rather odd to pick at this method, it's the least of your problems.

Well for sure it doesn't block anyone, people worked around that problem since Drupal 6, but in general having a more sane architecture would be nice, but guess, noone came up with something.

This issue is not really a bug per se since we did not support this before either, but the core committers and Views maintainers agreed that this agreed it should be a major issue since it definitely interferes with non-SQL Views support in a way that is at least really frustrating from a DX perspective.

To move this forward, I think we should (a) address @dawehner's feedback in #22 and (b) see if there are any reasons @chx's proposed workaround in #25 does not work as a temporary fix.

Wait, what "workaround"? If ensureMyTable is called very often (always?) SQL calls follow so you can't use the plugin as it is. If there's a major issue it is "Views is SQL bound". You are trying to untangle a gordian knot by pulling at a single thread. This is not conductive to success. Cutting through the knot with a sword is.

You need a way to describe data structures and then manipulate them without ever writing a lick of SQL.

So the real question is: how can you enhance entity query to achieve all that Views does? Then get efq_views in core, convert all core views to efq views, mark old ones deprecated, and when D9 comes remove those.

Edit: normally I do not post any more to the issue queue but since I was pinged, somewhat reluctantly, I am. If anyone has a problem, let me know and I will immediately delete this.

Wait, what "workaround"? If ensureMyTable is called very often (always?) SQL calls follow so you can't use the plugin as it is.

That's not true. What follows is often, e.g., a call to addWhere() – and since that passes a field, value and operator separately, it will also work just as well for a lot of other backends.
In the Search API we now managed, by just implementing a few methods from the SQL query plugin in our query plugin (e.g., addWhere()) and with the help of a few traits (1, 2, 3) to re-use all Views plugins with relatively little trouble – e.g., see the date field and filter.
It's just pretty ugly, I'd say, and I think it could be a lot less work and complexity with a little more help from Views itself – but it basically works. We just need to explicitly override those instances where a concrete SQL expression is used (as a condition or a column) – and that's very few places, actually.

So, to summarize (and answer xjm): yes, just implementing a few methods from the SQL query plugin and maybe doing some automatic conversions will go a long way to making the plugins reusable – but it's a lot more work than it needs to be. Definitely not a bug, though – you're right.

Any news or discussion on this which maybe hasn't been reported back here? Is this still an issue or should we close it in favour of other routes on the map? Just humbled over this one here be reading thru the issues and thought it looks important enough to wake it up or to discuss closing it with links to the new places.

Lets see what happens when we remove the method.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Think instead of removing it should be deprecated first
#41 applies but had a bunch of failures

Definitely think it will need a change record.