[FEATURE] Add support for filtering collections

More tests for the EntityResource class will be needed in a follow up issue.

Re-kicking tests.

Fixing test namespaces for testbot.

Merged.

I like where this is going!

I have a couple of questions/requests.

1. Groups

Can we have:

filter[$index][group][name]=<group label> // optional.

Instead of:

filter[$index][group][group]=<group_index> // optional.

2. Exists

Do we need this if we can support the IS NOT NULL as an operator?

3. Shortcuts

I'd like to keep simplified versions for simple queries, since those will be the common case. Maybe:

filter[0][field]=<public_field_name>&filter[0][value]=<value>

expands to:

filter[0][condition][field]=<public_field_name>
filter[0][condition][value]=<value>
filter[0][condition][operator]=%3D
filter[0][condition][group]=asfdf
filter[asfdf][group][conjunction]=AND

How would you feel about filter[$index][group][index]=$index.

Did you mean: filter[$index_0][group][index]=$index_1?

Or am I missing something.

This patch is amazing! I am so excited. It is also very big so the review is going to take quite some time.

First review pass:

1. +++ b/jsonapi.services.yml
   @@ -48,3 +48,6 @@ services:
   +    arguments: [ '@entity.query' ]
   

   This service will disappear. We should be injecting the entity_type.manager and using getStorage() from there.

2. +++ b/src/Query/ConditionOption.php
   @@ -0,0 +1,96 @@
   +   * Value of the field.
   

   Value of the condition for the given field.

3. +++ b/src/Query/ConditionOption.php
   @@ -0,0 +1,96 @@
   +    $this->field    = $field;
   

   I believe that we should not have the extra spaces to align values, according to the Drupal Coding Standards.

4. +++ b/src/Query/ExistsOption.php
   @@ -0,0 +1,79 @@
   +class ExistsOption implements QueryOptionInterface {
   

   There's some duplication with the ConditionOption class. Should we have a common parent (abstract) class to put the duplicated code there?

5. +++ b/src/Query/GroupOption.php
   @@ -0,0 +1,140 @@
   +      if ($child_id) return $child_id;
   +      return ($groupOption->hasChild($target_id)) ? $condition->id() : NULL;
   

   We need the curly braces and new lines according to the DCS. Maybe:

   return $child_id ?: ($groupOption->hasChild($target_id)) ? $condition->id() : NULL;

6. +++ b/src/Query/GroupOption.php
   @@ -0,0 +1,140 @@
   +    if ($this->id() == $target_id && $option instanceof GroupOption) {
   +      $this->childGroups[$option->id()] = $option;
   +      return TRUE;
   +    }
   +    elseif ($this->id() == $target_id) {
   +      $this->childOptions[$option->id()] = $option;
   +      return TRUE;
   +    }
   

   What about:

   if ($this->id() == $target_id) {
     $property = $option instanceof GroupOption ? 'childGroups' : 'childOptions';
     $this->{$property}[$option->id()] = $option;
     return TRUE;
   }
   </li>
   
   <li>
   &#10;+++ b/src/Query/GroupOption.php&#10;@@ -0,0 +1,140 @@&#10;+    elseif ($proper_child = array_reduce($this-&gt;childGroups, $find_proper_id, NULL)) {&#10;
   
   This array reduce will keep looping even after an ID is found.
   
   Maybe we need a helper protected method with an early return when the ID is found.
   </li>
   
   <li>
   &#10;+++ b/src/Query/GroupOption.php&#10;@@ -0,0 +1,140 @@&#10;+    case &#039;OR&#039;:&#10;...&#10;+    case &#039;AND&#039;:&#10;+    default:&#10;
   
   Indentation for the cases is off by 2 spaces.
   </li>
   
   <li>
   &#10;+++ b/src/Query/GroupOption.php&#10;@@ -0,0 +1,140 @@&#10;+    if (!isset($this-&gt;childOptions) || empty($this-&gt;childOptions)) return FALSE;&#10;
   
   Same comment about inline if statements.
   
   https://www.drupal.org/coding-standards#controlstruct says:
   
   <blockquote>Always use curly braces even in situations where they are technically optional. Having them increases readability and decreases the likelihood of logic errors being introduced when new lines are added. </blockquote>
   </li>
   
   <li>
   &#10;+++ b/src/Query/GroupOption.php&#10;@@ -0,0 +1,140 @@&#10;+    return array_reduce($this-&gt;groupOptions, function ($hasChild, $group) use ($id) {&#10;
   
   This keeps looping even after we know what the return value will be.
   </li>
   
   <li>
   &#10;+++ b/src/Query/QueryBuilder.php&#10;@@ -0,0 +1,212 @@&#10;+  public function addFilter() {&#10;
   
   I'm not sure what this is doing, but there is no docs or implementation code there.
   </li>
   
   <li>
   &#10;+++ b/src/Query/QueryBuilder.php&#10;@@ -0,0 +1,212 @@&#10;+    ¶&#10;
   
   Nit: Extra blank space.
   </li>
   
   <li>
   &#10;+++ b/src/Query/QueryBuilder.php&#10;@@ -0,0 +1,212 @@&#10;+    $applied_query = array_reduce($this-&gt;options, function ($query, $option) {&#10;+      $query = $option-&gt;apply($query);&#10;+      return $query;&#10;+    }, $query);&#10;
   
   Can we just do:
   <?php
   return $option->apply($query);
   

7. +++ b/src/Query/QueryBuilder.php
   @@ -0,0 +1,212 @@
   +    case 'filter':
   

   Indendation for 'case'.

8. +++ b/src/Query/QueryBuilder.php
   @@ -0,0 +1,212 @@
   +   * @param Drupal\jsonapi\Routing\Param\JsonApiParamInterface $param
   

   FQDN needs a leading backslash in the docblocks.

1. +++ b/src/Query/ConditionOption.php
   @@ -0,0 +1,96 @@
   +  /**
   +   * Value of the field.
   +   *
   +   * @var string
   +   */
   +  protected $value;
   

   What if we have a multivalue operator? Should we make this an string|string[]?

2. +++ b/src/Query/GroupOption.php
   @@ -0,0 +1,140 @@
   +class GroupOption implements QueryOptionInterface, QueryOptionTreeItemInterface {
   

   We'll need docblocks for classes as well.

3. +++ b/src/Query/QueryBuilderInterface.php
   @@ -0,0 +1,26 @@
   +interface QueryBuilderInterface {
   

   Docblocks for class needed. (This happens in several other places, I won't report every one)

4. +++ b/src/Resource/EntityResource.php
   @@ -87,6 +95,27 @@ class EntityResource implements EntityResourceInterface {
      /**
   +   * Gets a basic query for a collection.
   +   */
   +  protected function getCollectionQuery($entity_type_id, $params) {
   

   Needs docs for the parameters.

5. +++ b/src/Routing/Param/Filter.php
   @@ -16,8 +16,6 @@ class Filter extends JsonApiParamBase {
   -  protected static $multivalue_operators = ['IN', 'NOT IN', 'BETWEEN'];
   

   Are we losing the ability to use multivalue operators?

I'm done with the review. We will definitely need some extensive functional tests for the filtering but in order to increase development velocity, I'd say that those can be a follow up.

I'm doing tests as part of #2726265: [BUGFIX] Add test coverage for EntityResource, Routes and ResourceManager, which will include testing for lists (without filters). Maybe you can base off that.

Will you create the follow up issue for the tests as part of the acceptance criteria for this one?

Finally, I always forget to add interdiffs but I will need it to review your updates to this patch.

Did I say how excited I am about this patch? :-D

This was merged. Thanks!