[FEATURE] Implement validation of filter params

In trying to grasp https://www.drupal.org/docs/8/modules/json-api/collections-filtering-sor... I tried to translate that back into https://en.wikipedia.org/wiki/Backus–Naur_form

For me this helped a little. But I guess if we first spec the filter we can write tests for it easier.

Below a EBNF of the canonical version: filter[{filter id}][{filter type}][{filter property}]={filter value}

<filter> :== <filter-item> | <filter-item> '&' <filter>

<filter-item> :== <condition-filter> | <group-filter>

<condition-filter> :== <filter-id> '[condition]' <condition-filter-property>

<condition-filter-property> :== '[path]' | '[value]' | '[operator]' | '[group]' | '[memberOf]'

<group-filter> :== <filter-id> '[group]' <group-filter-property>

<filter-id> :== '[' <alpha-with-dashes-or-integer> ']'

<group-filter-property> :== '[conjunction]' | '[group]'

<alpha-with-dashes-or-integer> :== <letter-dash> | <integer>

<letter-dash> ::= <letter> | <letter> <letter-dash> | <letter> '-' <letter-dash>

<integer> ::= <digit> | <digit> <integer>

filter-id is not completely clear to me. "it can be a string or an integer". What are the string chars? How long can it be?

Both (optional) group is unclear to me in:

- Condition Properties: path, value, operator, (optional) group
- Group Properties: conjunction, (optional) group

memberOf is mentioned on doc page but not in the list of

filter[filter-id][group][group] = "parent-group-id"

why not make that parent instead of that weird repeating [group][group]

filter[filter-id][group][parent] = "parent-group-id"

As http://jsonapi.org/format/#fetching-filtering is agnostic I'd love to have our specs for filtering in a rigid way.

I hope we can derive the short forms from the long version.

What I've found interesting is that devs can make "condition" query without specifying the "condition" key in the filter query.

Drupal core devs or JSON API devs?

So we are not define / specify the filter options? We could say the docs are the specs but my 'bnf' was just a first stab to a specification. I hoped someone would point to a PHP spec tool :p

1. +++ b/tests/src/Unit/Routing/Param/FilterTest.php
   @@ -14,72 +14,110 @@ class FilterTest extends UnitTestCase {
   +   * @dataProvider validFiltersDataProvider
   

   I hoped for a canonical data provider and a function to create the short version(s) from it.

2. +++ b/tests/src/Unit/Routing/Param/FilterTest.php
   @@ -14,72 +14,110 @@ class FilterTest extends UnitTestCase {
   +      // Test case:
   +      // filter[foo][value]=bar.
          [
            ['foo' => ['value' => 'bar']],
            ['foo' => ['condition' => ['path' => 'foo', 'value' => 'bar', 'operator' => '=']]],
          ],
   -      // Tests filter[0][path]=foo&filter[0][value]=bar.
   +      // Test case:
   +      // filter[0][path]=foo
   +      // filter[0][value]=bar.
          [
            [0 => ['path' => 'foo', 'value' => 'bar']],
            [0 => ['condition' => ['path' => 'foo', 'value' => 'bar', 'operator' => '=']]],
          ],
   

   Why not use the filter URL parameter strings instead of PHP arrays? Makes the comments obsolete.

I'm sorry it took too long for this review.

+++ b/src/Routing/Param/Filter.php
@@ -74,6 +75,7 @@ class Filter extends JsonApiParamBase {
     foreach ($this->original as $filter_index => $filter_item) {
+      $this->validateItem($filter_index, $filter_item);
       $expanded[$filter_index] = $this->expandItem($filter_index, $filter_item);
     }

Let's validate the expanded item instead so we can simplify the validation logic.

I took quite a tour d.o #2884913: Update link module for array query parameters and Link D7 #2333119: Output broken when using array parameters in query to answer my own question. Why not use parse_str to generate your arrays.

    $test = [
        'filter[foo][value]=bar',
        'filter[foo][condition][path]=foo&filter[foo][condition][value]=bar&filter[foo][condition][operator]=%23',
    ];
    
    parse_str($test[0], $input);
    parse_str($test[1], $expected);

The test code is long due to extra documentation and does not match potential reports submitting their query strings.

I cannot review this patch due to not enough experience yet with filters.

I ran into D8 #2885351: Query string duplications wondering whether we should have tests for bad patterns too.

\Drupal\Tests\link\Functional\LinkFieldTest has also some $invalid_external_entries and $invalid_internal_entries.

And related to #21 they use \parse_str in core/lib/Drupal/Core/Url.php:298.

Sorry for the noise ;-)

@Spleshka do you think you'll have time to follow up on this soon? If not, there's no problem with that, please unassign the ticket and grab it back when you're available again.

@Spleshka I'm sorry to hear that. I'm cheering for you till the end of your project! I just wanted to make sure you had all you needed for this.

This looks great @Spleshka! I wonder if we can have this kind of validation and test coverage for sort and page as well. Can you create follow up tickets for those?

I ended up doing a refactor and catching a bug with the IS NULL filters (they don't require a value).

This is committed. Thanks!