Add Views filters

Here is a first try, feedback would be very welcome!

I've tested a bit and everything seems to work fine, but I'm sure there are a lot of options that are still unnecessary or will break things.

For D8 I've changed the approach taken for this. In Drupal 7 we had one base filter for all Search API filters, which the others would inherit from and implement their specific functionality on top of. This worked pretty well, but lead to a lot of differences between Views' "native" filters and the equivalent Search API fitlers.
In Drupal 8, due to the availability of traits, we now have a much cleaner way of letting each Search API filter inherit from the one in Views that corresponds most closely to it, leading to much cleaner code and, hopefully, better working and more feature-complete filters.
One more change I made to make things a lot easier is just implementing the addWhere() method in the Search API query plugin (even though it doesn't really make sense, conceptually), and translate from the SQL-specific format used in Views' default query plugin to our own syntax. This also seems to work quite well, and reduces boilerplate code by a lot. Most filter classes, and even the trait, are now pretty empty, actually.

One thing that would still be great would be updating the Views test to test more of these handlers.

Second try.
Didn't know that the test bot fails on strict warnings, but great that it does! (Though I'm confused that I didn't get those locally.)

Also, while testing the fulltext search filter locally again, I ran into an error with its config schema definition and realized for the first time that it even has one. Does that mean we should add similar definitions for all other filter plugins?

If i understand your question right: Absolutely and enable strict checking in tests. It helps us to maintain long term consistency / compatibility of configuration.

How does one enable strict checking in tests? I thought this was automatically done, but this doesn't seem to be the case.
Anyways, thanks for clearing that up!

... strange it didn't identify your issue?

Ah, probably because we don't have any filters (except for "Search: Fulltext search") enabled in our test view. So once we'd add tests for those, that would pop up in any case.
Anyways, thanks for the information!

Fatal error: Drupal\search_api\Plugin\views\filter\SearchApiFilterNumeric has colliding constructor definitions coming from traits in .../src/modules/contrib/search_api/src/Plugin/views/filter/SearchApiFilterNumeric.php on line 41

I just tried it out myself, but couldn't reproduce the problem – for me, all of this works fine.
A quick internet search revealed that this is/was indeed a bug in PHP itself: Constructor from trait conflicts with inherited constructor.

Regrettably, it seems this bug wasn't fixed until 5.5.20/5.6.4, and since Drupal only requires 5.5.9 I guess this means we really have to change this code. Darn …
I'll try to provide an updated patch in the next few days.

Here is an updated patch, working aroung the PHP 5.5.9 bug. Luckily, the constructor in the trait was almost pointless anyways, so it was easy to replace.

Config schema and extended tests are still needed, but it would still be great to get some feedback.

This hook has no real namespace and could so be problematic when core introduces something similar for example, which we kinda want.

It has a namespace, it starts with search_api. Of course, I forgot the hook_ in front, so that might be what you mean?
Anyways, thanks for pointing me to it. The attached patch fixes this.

Its also add that this example talks about my_entity_type but it looks more like a field type for me ...

It determines the handlers for fields that reference a my_entity_type entity. I thought that might be something that's useful for lots of people.

Would also be good to list this in the list of beta blocker issues, I guess, even if the parent is already declared a beta blocker.

The changes to the readme file are unrelated, should go in #2268867: Review README.txt files.

I'm not sure if dumping more stuff into Utility is a good idea.

I think this is a definitely not related to this patch though. We should probably open a new issue for that.

You're right, of course, got carried away once again. (Also, this was in the time before such fast progress and thorough reviews as now, where it would have probably made little difference.)

I posted the README.txt changes to the linked issue, and created #2637712: Provide helper methods for getting our services for the other one.
And while I agree that Utility should be split up, as long as #2230907: Split up Utility into one or more services is still unresolved we might as well put the methods there, and devise a coherent system for where to put them afterwards. But I'd say the methods are very good to have, since those magic strings you need to remember or manually look up each time are just terrible DX. (And with the methods, you also have one more way to quickly look up all of those IDs.)

Is there an issue about this bug? We should link to that.

Doesn't seem like it was reported before, I now created one: #2637674: Remove unused fields from value form based on used operators.
But it's a Core issue, so it's reasonable to assume it will just lie there for the next decade.

In any case, thanks a lot for your review, as thorough as always! The attached patch contains all your suggested changes (as well as being re-rolled to apply to the latest dev).
And some of the changes are really hard to understand, they just came from inheriting from Views' own filter plugins and then fixing whatever broke. There is no greater scheme behind it (except inheriting from Views' own filter plugins, and using a trait to re-use as much code as possible).

Still needed: tests and config schema (which the tests will hopefully show).

Thanks for your comments, I incorporated all of them in the attached patch!
Setting to "Needs review" purely for the test bot, this still has the problems mentioned in #11.

For the empty class, I now changed it to what I also used for SearchApiException – a single space. As you say, completely a matter of taste.

In any case, great to hear you're beginning to understand the patch! ;) That probably means it can't be that bad.

Here is a current re-roll, with (hopefully correct) config schemas.

I now just need to figure out how to best add filters to the existing test view.

Also, using the "Configuration inspector" module I still see several schema errors – however, I don't really understand most of them ("missing schema" for filters which should have a schema), and some seem to be just wrong in Views itself (e.g., "Reduce" has type boolean, but is an integer due to the Form API internals).

Resubmitting with a $strictConfigSchema=FALSE to skip schema checks for search_api_test_views.

Shouldn't it be enough to just manually edit the exported view and change the data type of the offending settings? Or is it not possible for us to fix the other errors? I have to admit, I'm pretty confused about some of them, but I'd have thought that would be solved with a bit of debugging.

This one should pass, but still needs additional (exposed) filters in the test view.
(Interdiff compared with my last patch, #25 – Miro's addition doesn't seem necessary after all.)

I also discovered #2641434: New views are always created with query type "views_query", which lead to some additional errors when creating a view via the UI.

And some more tests. Pass fine for me locally, if the test bot agrees this could be RTBC.

Excellent, great to hear!
Fixed that and committed.

The change was due to the issue described in the @todo above: I added a debug statement to show me the query but it killed the page request. Therefore, I commented out that line, and then forgot to revert that before creating the patch.