Comments

drunken monkey created an issue. See original summary.

drunken monkey’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
84.01 KB

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.

Status: Needs review » Needs work

The last submitted patch, 2: 2601224-2--views_filters.patch, failed testing.

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
85.33 KB
5.66 KB

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?

miro_dietiker’s picture

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.

drunken monkey’s picture

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!

miro_dietiker’s picture

Yeah, it is defined in TestBase:

  /**
   * Set to TRUE to strict check all configuration saved.
   *
   * @see \Drupal\Core\Config\Testing\ConfigSchemaChecker
   *
   * @var bool
   */
  protected $strictConfigSchema = TRUE;

And yes it is on and tested by ConfigSchemaChecker::onConfigSave()
... strange it didn't identify your issue?

drunken monkey’s picture

Status: Needs review » Needs work

... 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!

ChristianAdamski’s picture

As mentioned elsewhere:

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

Might be caused by changes in API upstream

drunken monkey’s picture

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.

drunken monkey’s picture

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.

drunken monkey’s picture

dawehner’s picture

+++ b/search_api.api.php
@@ -98,6 +98,34 @@ function hook_search_api_field_type_mapping_alter(array &$mapping) {
+ */
+function search_api_views_handler_mapping_alter(array &$mapping) {
+  $mapping['entity:my_entity_type'] = array(

This hook has no real namespace and could so be problematic when core introduces something similar for example, which we kinda want. ... Its also add that this example talks about my_entity_type but it looks more like a field type for me ...

drunken monkey’s picture

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.

drunken monkey’s picture

Issue tags: +beta blocker

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.

borisson_’s picture

  1. --- a/README.txt
    +++ b/README.txt
    

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

  2. +++ b/src/Form/ServerForm.php
    @@ -77,7 +78,7 @@ class ServerForm extends EntityForm {
    -    return $this->backendPluginManager ?: \Drupal::service('plugin.manager.search_api.backend');
    +    return $this->backendPluginManager ?: Utility::getBackendPluginManager();
    

    There's a truckload of changes like this one, that change a \Drupal::service call with a Utility::get*Manager call.

    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.

  3. +++ b/src/Plugin/views/filter/SearchApiFilterTrait.php
    @@ -0,0 +1,92 @@
    +   * This is done since this is not necessary for Search API queries.
    

    I would like it if this comment describes why it's not necessary, something like:
    "Because the result from a Search API query doesn't have to come from a entity inside drupal, we don't have to check if the table really exists."

  4. +++ b/src/Plugin/views/filter/SearchApiFilterTrait.php
    @@ -0,0 +1,92 @@
    +  public function ensureMyTable() {
    +    // Do nothing.
    +  }
    

    Can we change this to public function ensureMyTable() {}. The docblock should describe the why.

  5. +++ b/src/Plugin/views/filter/SearchApiFilterTrait.php
    @@ -0,0 +1,92 @@
    +   * Overridden to remove fields that won't be used (but aren't hidden either
    +   * because of a small bug/glitch in the form code.
    

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

  6. +++ b/src/Plugin/views/filter/SearchApiUser.php
    @@ -20,68 +18,52 @@ use Drupal\Core\Form\FormStateInterface;
    +  function operators() {
    

    This needs a visiblity: public function ...

This is a really big patch and I don't understand everything that is going on. These are things I noticed on the first pass though.

drunken monkey’s picture

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).

borisson_’s picture

Status: Needs work » Needs review

Setting to needs review for the testbot.

Status: Needs review » Needs work

The last submitted patch, 17: 2601224-17--views_filters.patch, failed testing.

The last submitted patch, 17: 2601224-17--views_filters.patch, failed testing.

borisson_’s picture

I think I almost understand what is going on in the patch, that's great. I found a couple of really small things that could be better.

  1. +++ b/src/Plugin/views/filter/SearchApiFilterDate.php
    @@ -16,87 +16,19 @@ use Drupal\Core\Form\FormStateInterface;
    +    // @todo Enable "(not) between" again once that operator is available in
    +    //   the Search API.
    

    Is there an issue we can link this @todo to?

  2. +++ b/src/Plugin/views/filter/SearchApiFilterOptions.php
    @@ -18,279 +17,17 @@ use Drupal\Core\Form\FormStateInterface;
    +    unset($form['reduce_duplicates']);
    

    Should we add the reason we can't do the reduce duplicates with this this filter?

  3. +++ b/src/Plugin/views/filter/SearchApiFilterString.php
    @@ -0,0 +1,24 @@
    +class SearchApiFilterString extends SearchApiFilterNumeric {
    +
    +}
    

    I don't think there's an explicit recommendation for this in the style guide but for empty classes I prefer to remove the empty lines.

    class SearchApiFilterString extends SearchApiFilterNumeric {}

  4. +++ b/src/Plugin/views/filter/SearchApiFilterTrait.php
    @@ -0,0 +1,92 @@
    +    // @todo Use "IN"/"NOT IN" instead, once available.
    

    Is there an issue we can link this @todo to?

  5. +++ b/src/Plugin/views/query/SearchApiQuery.php
    @@ -111,13 +112,16 @@ class SearchApiQuery extends QueryPluginBase {
    +    // @todo Instead use Views::viewsData() – injected, too – to load the base
    +    //   table definition and use the "index" (or maybe rename to
    +    //   "search_api_index") field from there.
    

    Is there an issue we can link this @todo to? Or should this be fixed in this issue?

drunken monkey’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 22: 2601224-22--views_filters.patch, failed testing.

The last submitted patch, 22: 2601224-22--views_filters.patch, failed testing.

drunken monkey’s picture

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).

Status: Needs review » Needs work

The last submitted patch, 25: 2601224-25--views_filters.patch, failed testing.

The last submitted patch, 25: 2601224-25--views_filters.patch, failed testing.

miro_dietiker’s picture

Core has a bunch of schema errors for views features (that are not test covered).

miro_dietiker’s picture

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

Status: Needs review » Needs work

The last submitted patch, 29: search_api_2601224_filters_29.patch, failed testing.

The last submitted patch, 29: search_api_2601224_filters_29.patch, failed testing.

drunken monkey’s picture

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.

drunken monkey’s picture

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.

drunken monkey’s picture

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

borisson_’s picture

Status: Needs review » Needs work

I found one thing I don't quite understand in the interdiff, I didn't check the patch itself again but I don't think that's needed.

  1. +++ b/src/Query/Query.php
    @@ -476,7 +476,7 @@ class Query implements QueryInterface {
    -    $ret .= 'Options: ' . str_replace("\n", "\n  ", var_export($this->options, TRUE)) . "\n";
    +//    $ret .= 'Options: ' . str_replace("\n", "\n  ", var_export($this->options, TRUE)) . "\n";
    

    Any reason this is commented out?

Settings this back to NW to fix that, when that is fixed I'm happy with the patch.

  • drunken monkey committed abfc7dd on 8.x-1.x
    Issue #2601224 by drunken monkey, borisson_: Added Views filters for all...
drunken monkey’s picture

Status: Needs work » Fixed

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.

Status: Fixed » Closed (fixed)

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